[vlc-devel] [PATCH] Open Sound Control support using liblo

[Nicholas J Humfrey]

Hello,

Thank you very much for reviewing my patch. Many of that issues you  
have raised are as a result of copying code from elsewhere in VLC.


> +dnl
> +dnl liblo based Open Sound Control plugin
> +dnl
> +AC_ARG_ENABLE(osc,
> +  [  --enable-osc            Open Sound Control plugin (default
> disabled)])
> +if test "${enable_osc}" = "yes"; then
> +  PKG_CHECK_MODULES(LIBLO, liblo >= 0.26,
> +    [AC_DEFINE(HAVE_LIBLO, 1, [Define if you have the liblo library])
>
> Why is this AC_DEFINE? You don't seem to use HAVE_LIBIO anywhere, so
> don't bother (with) <config.h>.

Yes, changed.


> +     VLC_ADD_PLUGIN([osc])
> +     VLC_ADD_LIBS([osc],[$LIBLO_LIBS])
> +     VLC_ADD_CFLAGS([osc],[$LIBLO_CFLAGS])],
> +    [AC_MSG_WARN(liblo library not found)])
>
> If --enable-osc was explicitly specified but libio is not found, you
> should probably fail hard.

Yes, good idea.


> +
> +#include <fcntl.h>
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
>
> Generally, I recommend including fcntl.h _after_ config.h, so you get
> defines that may affect system headers (e.g. _GNU_SOURCE).

It turns out that fcntl.h isn't needed there at all. It was a bad copy/ 
paste from lirc.c.


> +
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_interface.h>
> +#include <vlc_playlist.h>
> +
> +#include <lo/lo.h>
> +
> +
> +#define OSC_PORT_TEXT N_( "Port Number to listen for OSC messages." )
> +#define OSC_PORT_LONGTEXT N_( \
> +    "Set the port number that VLC should listen for OSC messages  
> on. "
> \
> +    "By default it chooses a random port number." )
> +
> +/* Thread-local variable to store object running the current thread  
> */
> +static vlc_threadvar_t thread_object_key;
> +static int instance_count = 0;
>
> unsigned.

Changed.


> +/ 
> *****************************************************************************
> + * DeckHandler: callback for /deck/ * messages
> +
> *****************************************************************************/
> +static int DeckHandler( const char *path, const char *types, lo_arg
> **argv,
> +    int argc, lo_message msg, void *user_data )
> +{
> +    vlc_object_t *p_this = (vlc_object_t*)user_data;
> +    char* psz_from =  
> lo_address_get_url( lo_message_get_source(msg) );
> +    playlist_t *p_playlist = NULL;
>
> Assignment is not used.

Removed.

> +    int i_ret = 0;
> +
> +    msg_Info( p_this, "Recieved '%s' from '%s'.", path, psz_from );
> +    free(psz_from);
> +
> +    p_playlist = pl_Hold( (vlc_object_t*) p_this );
>
> Casting should not be needed here.

Changed.


> +/ 
> *****************************************************************************
> + * Run: main loop
> +
> *****************************************************************************/
> +static void Run( intf_thread_t *p_intf )
> +{
> +    intf_sys_t *p_sys = p_intf->p_sys;
> +    int i_cancel, i_msg_available;
>
> Is it so that ErrorHandler cannot be called from this thread?
> I would have expected a vlc_threadvar_set() call here.

Yes, changed. It should definitely have a reference to the current  
thread.


> +
> +    for( ;; )
> +    {
> +        /* Wait for a message / check if scheduled message needs
> processing */
> +        i_msg_available = lo_server_wait( p_sys->p_server, 100 );
> +        if (i_msg_available<=0) continue;
>
> Is 100 some kind of timeout value? If so, why can you not use an
> infinite (or really long timeout)?

Ugh, sadly there still needs to be some polling. With OSC you able to  
send a message and then schedule it to be acted on at a specified  
time. The polling is required incase an event gets added while you are  
waiting. But perhaps this is a bug in liblo that needs sorting out.

How long is really long?


> Convincing the release manager (not me!) that this can violate the
> feature freeze, or wait for the VLC 1.1 defrost.

Would be great if it made it into 1.0 - it isn't a big changeset and  
isn't enabled by default. But understandable if it doesn't.



nick.