[vlc-devel] [PATCH 1/8] Fix Metadata marshalling when sending the PropertiesChanged signal

[mirsal]

Comments inline :)

On Mon, 2013-01-28 at 11:54 +0000, Alex Merry wrote:
> VLC was getting kicked from the D-Bus when a track was stopped, because
> it was generating invalid data on the wire when sending the
> PropertiesChanged signal for the Metadata property.
> 
> The issue was that if there was now no current track, GetInputMeta would
> never be called and the the variant would never be populated with the
> "a{sv}" structure that the call to dbus_message_iter_open_container
> claimed it would be.
> 
> We now share the code that GetProperties used, which dealt with this
> correctly, although now both use CurrentInput (which is what the signal
> previously used) instead of CurrentPlayingItem (which is what
> GetProperties previously used).
> ---
>  modules/control/dbus/dbus_player.c | 58 ++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/modules/control/dbus/dbus_player.c b/modules/control/dbus/dbus_player.c
> index 5d24af1..ef40688 100644
> --- a/modules/control/dbus/dbus_player.c
> +++ b/modules/control/dbus/dbus_player.c
> @@ -4,10 +4,12 @@
>   * Copyright © 2006-2011 Rafaël Carré
>   * Copyright © 2007-2011 Mirsal Ennaime
>   * Copyright © 2009-2011 The VideoLAN team
> + * Copyright © 2013      Alex Merry
>   * $Id$
>   *
>   * Authors:    Mirsal Ennaime <mirsal at mirsal fr>
>   *             Rafaël Carré <funman at videolanorg>
> + *             Alex Merry <dev at randomguy3 me uk>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -673,30 +675,46 @@ DBUS_METHOD( LoopStatusSet )
>      REPLY_SEND;
>  }
>  
> +static int
> +MarshalMetadata( intf_thread_t *p_intf, DBusMessageIter *container )
> +{
> +    DBusMessageIter a;
> +    playlist_t *p_playlist = p_intf->p_sys->p_playlist;
> +    input_item_t *p_item = 0;
> +
> +    input_thread_t *p_input;
> +    if( ( p_input = playlist_CurrentInput( p_intf->p_sys->p_playlist ) ) ) {
> +        PL_LOCK;
> +        p_item = input_GetItem( p_input );
> +        if( p_item )
> +            GetInputMeta( p_item, container );
> +        PL_UNLOCK;

This does not look correct to me, the playlist lock should not be used
to synchronize with an input thread.

Removing the PL_LOCK/UNLOCK calls should be fine here.

> +        vlc_object_release( (vlc_object_t*) p_input );
> +    }
> +    if (!p_item) {
> +        // avoid breaking the type marshalling
> +        if( !dbus_message_iter_open_container( container, DBUS_TYPE_ARRAY, "{sv}", &a ) ||
> +              !dbus_message_iter_close_container( container, &a ) ) {
> +            PL_UNLOCK;
> +            return DBUS_HANDLER_RESULT_NEED_MEMORY;

Use VLC's error constants

> +        }
> +    }
> +
> +    return VLC_SUCCESS;
> +}
> +
>  DBUS_METHOD( Metadata )
>  {
>      REPLY_INIT;
>      OUT_ARGUMENTS;
>  
> -    DBusMessageIter v, a;
> -    playlist_t *p_playlist = PL;
> +    DBusMessageIter v;
>  
>      if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT,
>                                             "a{sv}", &v ) )
>          return DBUS_HANDLER_RESULT_NEED_MEMORY;
>  
> -    PL_LOCK;
> -    playlist_item_t* p_item =  playlist_CurrentPlayingItem( p_playlist );
> -
> -    if( p_item )
> -        GetInputMeta( p_item->p_input, &v );
> -
> -    PL_UNLOCK;
> -
> -    if( ( !p_item &&
> -        ( !dbus_message_iter_open_container( &v, DBUS_TYPE_ARRAY, "{sv}", &a ) ||
> -          !dbus_message_iter_close_container( &v, &a ) ) ) ||
> -
> +    if( MarshalMetadata( p_this, &v ) != VLC_SUCCESS ||
>          !dbus_message_iter_close_container( &args, &v ) ) {
>          return DBUS_HANDLER_RESULT_NEED_MEMORY;
>      }
> @@ -888,20 +906,10 @@ PropertiesChangedSignal( intf_thread_t    *p_intf,
>  
>          if( !strcmp( ppsz_properties[i], "Metadata" ) )
>          {
> -            input_thread_t *p_input;
> -            p_input = playlist_CurrentInput( p_intf->p_sys->p_playlist );
> -
>              dbus_message_iter_open_container( &entry,
>                                                DBUS_TYPE_VARIANT, "a{sv}",
>                                                &variant );
> -
> -            if( p_input )
> -            {
> -                input_item_t *p_item = input_GetItem( p_input );
> -                GetInputMeta( p_item, &variant );
> -                vlc_object_release( p_input );
> -            }
> -
> +            MarshalMetadata( p_intf, &variant );

Please check the marshaller's return value

>              dbus_message_iter_close_container( &entry, &variant );
>          }
>          else if( !strcmp( ppsz_properties[i], "PlaybackStatus" ) )

Looks good otherwise, thanks !

-- 
mirsal 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130128/1aa61be7/attachment.sig>