[vlc-devel] [PATCH] Updated mrpis to spec 2.2
mirsal
mirsal at mirsal.fr
Fri Feb 10 22:47:56 CET 2012
Hey kevin,
On Fri, 2012-02-10 at 16:00 -0500, Kevin Anthony wrote:
> On Thu, Feb 9, 2012 at 9:05 AM, mirsal <mirsal at mirsal.fr> wrote:
>
> > Hi Kevin, and thanks for the patch !
> > Comments inline.
> >
> > On Wed, 2012-02-08 at 21:54 -0500, Kevin Anthony wrote:
> > > Added Fullscreen and CanSetFullscreen properties,
> > > including the underlying methods to set properties in dbus_root
> > > ---
> > > modules/control/dbus/dbus.c | 13 ++-
> > > modules/control/dbus/dbus_common.h | 3 +-
> > > modules/control/dbus/dbus_introspect.h | 2 +
> > > modules/control/dbus/dbus_root.c | 175
> > ++++++++++++++++++++++++++++++++
> > > modules/control/dbus/dbus_root.h | 2 +
> > > 5 files changed, 193 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/modules/control/dbus/dbus.c b/modules/control/dbus/dbus.c
> > > index 21945e2..e238870 100644
> > > --- a/modules/control/dbus/dbus.c
> > > +++ b/modules/control/dbus/dbus.c
> > > @@ -531,9 +531,10 @@ static void ProcessEvents( intf_thread_t *p_intf,
> > > playlist_t *p_playlist = p_intf->p_sys->p_playlist;
> > > bool b_can_play = p_intf->p_sys->b_can_play;
> > >
> > > - vlc_dictionary_t player_properties, tracklist_properties;
> > > + vlc_dictionary_t player_properties, tracklist_properties,
> > root_properties;
> > > vlc_dictionary_init( &player_properties, 0 );
> > > vlc_dictionary_init( &tracklist_properties, 0 );
> > > + vlc_dictionary_init( &root_properties, 0 );
> > >
> > > for( int i = 0; i < i_events; i++ )
> > > {
> > > @@ -565,6 +566,9 @@ static void ProcessEvents( intf_thread_t *p_intf,
> > > case SIGNAL_RANDOM:
> > > vlc_dictionary_insert( &player_properties, "Shuffle", NULL
> > );
> > > break;
> > > + case SIGNAL_FULLSCREEN:
> > > + vlc_dictionary_insert( &root_properties, "Fullscreen", NULL
> > );
> > > + break;
> > > case SIGNAL_REPEAT:
> > > case SIGNAL_LOOP:
> > > vlc_dictionary_insert( &player_properties, "LoopStatus",
> > NULL );
> > > @@ -623,8 +627,12 @@ static void ProcessEvents( intf_thread_t *p_intf,
> > > if( vlc_dictionary_keys_count( &tracklist_properties ) )
> > > TrackListPropertiesChangedEmit( p_intf, &tracklist_properties );
> > >
> > > + if( vlc_dictionary_keys_count( &root_properties ) )
> > > + RootPropertiesChangedEmit( p_intf, &root_properties );
> > > +
> > > vlc_dictionary_clear( &player_properties, NULL, NULL );
> > > vlc_dictionary_clear( &tracklist_properties, NULL, NULL );
> > > + vlc_dictionary_clear( &root_properties, NULL, NULL );
> > > }
> > >
> > > /**
> > > @@ -1049,6 +1057,9 @@ static int AllCallback( vlc_object_t *p_this,
> > const char *psz_var,
> > > else if( !strcmp( "random", psz_var ) )
> > > info->signal = SIGNAL_RANDOM;
> > >
> > > + else if( !strcmp( "fullscreen", psz_var ) )
> > > + info->signal = SIGNAL_FULLSCREEN;
> > > +
> >
> > The AllCallback function is not connected to any variable change event
> > with that name. This will never be executed unless you add a proper call
> > to AddCallback when initializing the module, which should be done in
> > dbus.c, in the Open() function.
> >
> > > else if( !strcmp( "repeat", psz_var ) )
> > > info->signal = SIGNAL_REPEAT;
> > >
> > > diff --git a/modules/control/dbus/dbus_common.h
> > b/modules/control/dbus/dbus_common.h
> > > index c977138..30d6e2c 100644
> > > --- a/modules/control/dbus/dbus_common.h
> > > +++ b/modules/control/dbus/dbus_common.h
> > > @@ -121,7 +121,8 @@ enum
> > > SIGNAL_CAN_SEEK,
> > > SIGNAL_CAN_PAUSE,
> > > SIGNAL_VOLUME_CHANGE,
> > > - SIGNAL_VOLUME_MUTED
> > > + SIGNAL_VOLUME_MUTED,
> > > + SIGNAL_FULLSCREEN
> > > };
> > >
> > > enum
> > > diff --git a/modules/control/dbus/dbus_introspect.h
> > b/modules/control/dbus/dbus_introspect.h
> > > index 5056c9b..cc53e45 100644
> > > --- a/modules/control/dbus/dbus_introspect.h
> > > +++ b/modules/control/dbus/dbus_introspect.h
> > > @@ -63,6 +63,8 @@ static const char* psz_introspection_xml =
> > > " <property name=\"SupportedUriSchemes\" type=\"as\" access=\"read\"
> > />\n"
> > > " <property name=\"HasTrackList\" type=\"b\" access=\"read\" />\n"
> > > " <property name=\"CanQuit\" type=\"b\" access=\"read\" />\n"
> > > +" <property name=\"CanSetFullscreen\" type=\"b\" access=\"read\"
> > />\n"
> > > +" <property name=\"Fullscreen\" type=\"b\" access=\"readwrite\" />\n"
> > > " <property name=\"CanRaise\" type=\"b\" access=\"read\" />\n"
> > > " <method name=\"Quit\" />\n"
> > > " <method name=\"Raise\" />\n"
> > > diff --git a/modules/control/dbus/dbus_root.c
> > b/modules/control/dbus/dbus_root.c
> > > index 5de8af6..1b92463 100644
> > > --- a/modules/control/dbus/dbus_root.c
> > > +++ b/modules/control/dbus/dbus_root.c
> > > @@ -30,6 +30,10 @@
> > >
> > > #include <vlc_common.h>
> > > #include <vlc_interface.h>
> > > +#include <vlc_input.h>
> > > +#include <vlc_vout.h>
> > > +#include <vlc_plugin.h>
> > > +#include <vlc_playlist.h>
> > >
> > > #include <unistd.h>
> > > #include <limits.h>
> > > @@ -95,6 +99,79 @@ DBUS_METHOD( Identity )
> > > }
> > >
> > > static int
> > > +MarshalCanSetFullscreen( intf_thread_t *p_intf, DBusMessageIter
> > *container )
> > > +{
> > > + VLC_UNUSED( p_intf );
> > > + const dbus_bool_t b_ret = TRUE;
> >
> > I am not sure that VLC's fullscreen state can be changed at all time.
> >
> > > +
> > > + dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN,
> > &b_ret );
> > > + return VLC_SUCCESS;
> > > +}
> > > +
> > > +DBUS_METHOD( CanSetFullscreen )
> > > +{
> > > + REPLY_INIT;
> > > + OUT_ARGUMENTS;
> > > +
> > > + DBusMessageIter v;
> > > + dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "b", &v
> > );
> > > +
> > > + MarshalCanSetFullscreen( p_this, &v );
> > > +
> > > + if( !dbus_message_iter_close_container( &args, &v ) )
> > > + return DBUS_HANDLER_RESULT_NEED_MEMORY;
> > > +
> > > + REPLY_SEND;
> > > +}
> > > +
> > > +static void
> > > +MarshalFullscreen( intf_thread_t *p_intf, DBusMessageIter *container )
> > > +{
> > > + vout_thread_t *p_vout = (vout_thread_t *)p_intf;
> > > + dbus_bool_t b_fullscreen;
> > > + if ( p_vout ) {
> > > + b_fullscreen = var_GetBool( p_vout , "fullscreen" );
> > > + } else {
> > > + b_fullscreen = FALSE;
> > > + }
> >
> > Coding style (have a look at: http://wiki.videolan.org/Code_Conventions)
Plus you should get the value from the playlist's "fullscreen" variable,
there is no need to access any data owned by the vout thread.
see comments below about FullscreenSet
> >
> > > + dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN,
> > &b_fullscreen );
> > > +}
> > > +
> > > +DBUS_METHOD( FullscreenGet )
> > > +{
> > > + REPLY_INIT;
> > > + OUT_ARGUMENTS;
> > > +
> > > + DBusMessageIter v;
> > > +
> > > + if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT,
> > "b", &v ) )
> > > + return DBUS_HANDLER_RESULT_NEED_MEMORY;
> > > +
> > > + MarshalFullscreen( p_this, &v );
> > > +
> > > + if( !dbus_message_iter_close_container( &args, &v ) )
> > > + return DBUS_HANDLER_RESULT_NEED_MEMORY;
> > > +
> > > + REPLY_SEND;
> > > +}
> > > +
> > > +DBUS_METHOD( FullscreenSet )
> > > +{
> > > + REPLY_INIT;
> > > + dbus_bool_t b_fullscreen;
> > > +
> > > + if( VLC_SUCCESS != DemarshalSetPropertyValue( p_from, &b_fullscreen
> > ) )
> > > + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> > > +
> > > + vout_thread_t *p_vout = (vout_thread_t *)p_this;
> >
> > This is not the right way to access the vout thread (it is done by
> > calling input_GetVout with an input thread as an argument) and anyway
> > you should rather use the "fullscreen" variable provided by the playlist
> > object, which nicely hides the details of manipulating the input and
> > vout threads away.
>
> When i set the var_SetBool( p_playlist, "fullscreen", ( b_fullscreen ==
> TRUE ) );
> although the variable does change, vlc doesn't actually fullscreen. when i
> looked in other files it seems when you want to set fullscreen, you need to
> do it via p_vout.
> unless there is another way, which i can't seem to find.
You need to set it through the vout indeed, my bad. That comment was
actually meant for the code which reads the fullscreen variable value.
So, set it on both the playlist and the vout, and get it from the
playlist. Setting fullscreen should look like something like that:
---
if( INTF->p_sys->p_input )
{
p_input = (input_thread_t*) vlc_object_hold( INTF->p_sys->p_input );
p_vout = input_GetVout( p_input );
vlc_object_release( p_input );
if( p_vout )
{
var_SetBool( p_vout, "fullscreen", ( b_fullscreen == TRUE ) );
vlc_object_release( p_vout );
}
if( PL )
var_SetBool( PL, "fullscreen", ( b_fullscreen == TRUE ) );
}
---
> > > +
> > > + if ( p_vout ){
> > > + var_SetBool( p_vout, "fullscreen", ( b_fullscreen == TRUE ) );
> > > + }
> >
> > Coding style, and see the comment above about using the wrong variable.
> >
> > > + REPLY_SEND;
> > > +}
> > > +
> > > +static int
> > > MarshalCanQuit( intf_thread_t *p_intf, DBusMessageIter *container )
> > > {
> > > VLC_UNUSED( p_intf );
> > > @@ -343,10 +420,38 @@ DBUS_METHOD( GetProperty )
> > > PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "SupportedUriSchemes",
> > SupportedUriSchemes )
> > > PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "HasTrackList",
> > HasTrackList )
> > > PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "CanQuit",
> > CanQuit )
> > > + PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "CanSetFullscreen",
> > CanSetFullscreen )
> > > + PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "Fullscreen",
> > FullscreenGet )
> > > PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "CanRaise",
> > CanRaise )
> > > PROPERTY_MAPPING_END
> > > }
> > >
> > > +DBUS_METHOD( SetProperty )
> > > +{
> > > + DBusError error;
> > > +
> > > + char *psz_interface_name = NULL;
> > > + char *psz_property_name = NULL;
> > > +
> > > + dbus_error_init( &error );
> > > + dbus_message_get_args( p_from, &error,
> > > + DBUS_TYPE_STRING, &psz_interface_name,
> > > + DBUS_TYPE_STRING, &psz_property_name,
> > > + DBUS_TYPE_INVALID );
> > > +
> > > + if( dbus_error_is_set( &error ) )
> > > + {
> > > + msg_Err( (vlc_object_t*) p_this, "D-Bus message reading : %s",
> > > + error.message );
> > > + dbus_error_free( &error );
> > > + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> > > + }
> > > +
> > > + PROPERTY_MAPPING_BEGIN
> > > + PROPERTY_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "Fullscreen",
> > FullscreenSet )
> > > + PROPERTY_MAPPING_END
> > > +}
> > > +
> > > #undef PROPERTY_MAPPING_BEGIN
> > > #undef PROPERTY_GET_FUNC
> > > #undef PROPERTY_MAPPING_END
> > > @@ -425,6 +530,7 @@ DBUS_METHOD( GetAllProperties )
> > > ADD_PROPERTY( SupportedUriSchemes, "as" );
> > > ADD_PROPERTY( HasTrackList, "b" );
> > > ADD_PROPERTY( CanQuit, "b" );
> > > + ADD_PROPERTY( CanSetFullscreen, "b" );
> > > ADD_PROPERTY( CanRaise, "b" );
> > >
> > > dbus_message_iter_close_container( &args, &dict );
> > > @@ -444,6 +550,7 @@ handle_root ( DBusConnection *p_conn, DBusMessage
> > *p_from, void *p_this )
> > > {
> > > METHOD_MAPPING_BEGIN
> > > METHOD_FUNC( DBUS_INTERFACE_PROPERTIES, "Get", GetProperty
> > );
> > > + METHOD_FUNC( DBUS_INTERFACE_PROPERTIES, "Set", SetProperty
> > );
> > > METHOD_FUNC( DBUS_INTERFACE_PROPERTIES, "GetAll",
> > GetAllProperties );
> > > METHOD_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "Quit", Quit );
> > > METHOD_FUNC( DBUS_MPRIS_ROOT_INTERFACE, "Raise", Raise );
> > > @@ -453,3 +560,71 @@ handle_root ( DBusConnection *p_conn, DBusMessage
> > *p_from, void *p_this )
> > > #undef METHOD_MAPPING_BEGIN
> > > #undef METHOD_FUNC
> > > #undef METHOD_MAPPING_END
> > > +/**
> > > + * PropertiesChangedSignal() synthetizes and sends the
> > > + * org.freedesktop.DBus.Properties.PropertiesChanged signal
> > > + */
> > > +
> > > +static DBusHandlerResult
> > > +PropertiesChangedSignal( intf_thread_t *p_intf,
> > > + vlc_dictionary_t *p_changed_properties )
> > > +{
> > > + DBusConnection *p_conn = p_intf->p_sys->p_conn;
> > > + DBusMessageIter changed_properties, invalidated_properties, entry,
> > variant;
> > > + const char *psz_interface_name = DBUS_MPRIS_ROOT_INTERFACE;
> > > + char **ppsz_properties = NULL;
> > > + int i_properties = 0;
> > > +
> > > + SIGNAL_INIT( DBUS_INTERFACE_PROPERTIES,
> > > + DBUS_MPRIS_OBJECT_PATH,
> > > + "PropertiesChanged" );
> > > +
> > > + OUT_ARGUMENTS;
> > > + ADD_STRING( &psz_interface_name );
> > > + dbus_message_iter_open_container( &args, DBUS_TYPE_ARRAY, "{sv}",
> > > + &changed_properties );
> > > +
> > > + i_properties = vlc_dictionary_keys_count( p_changed_properties );
> > > + ppsz_properties = vlc_dictionary_all_keys( p_changed_properties );
> > > +
> > > + for( int i = 0; i < i_properties; i++ )
> > > + {
> > > + dbus_message_iter_open_container( &changed_properties,
> > > + DBUS_TYPE_DICT_ENTRY, NULL,
> > > + &entry );
> > > +
> > > + dbus_message_iter_append_basic( &entry, DBUS_TYPE_STRING,
> > > + &ppsz_properties[i] );
> > > +
> > > + if( !strcmp( ppsz_properties[i], "Fullscreen" ) )
> > > + {
> > > + dbus_message_iter_open_container( &entry,
> > > + DBUS_TYPE_VARIANT, "b",
> > > + &variant );
> > > + MarshalFullscreen( p_intf, &variant );
> > > + dbus_message_iter_close_container( &entry, &variant );
> > > + }
> > > + dbus_message_iter_close_container( &changed_properties, &entry
> > );
> > > + free( ppsz_properties[i] );
> > > + }
> > > + dbus_message_iter_close_container( &args, &changed_properties );
> > > + dbus_message_iter_open_container( &args, DBUS_TYPE_ARRAY, "s",
> > > + &invalidated_properties );
> > > + dbus_message_iter_close_container( &args, &invalidated_properties );
> > > + free( ppsz_properties );
> > > +
> > > + SIGNAL_SEND;
> > > +}
> > > +
> > >
> > +/*****************************************************************************
> > > + * PropertiesChangedEmitRoot: Emits the Seeked signal
> > > +
> > *****************************************************************************/
> >
> > Wrong function name ;)
> >
> > > +int RootPropertiesChangedEmit( intf_thread_t * p_intf,
> > > + vlc_dictionary_t *
> > p_changed_properties )
> > > +{
> > > + if( p_intf->p_sys->b_dead )
> > > + return VLC_SUCCESS;
> > > +
> > > + PropertiesChangedSignal( p_intf, p_changed_properties );
> > > + return VLC_SUCCESS;
> > > +}
> > > diff --git a/modules/control/dbus/dbus_root.h
> > b/modules/control/dbus/dbus_root.h
> > > index 38267ee..86adaed 100644
> > > --- a/modules/control/dbus/dbus_root.h
> > > +++ b/modules/control/dbus/dbus_root.h
> > > @@ -36,5 +36,7 @@
> > > DBusHandlerResult handle_root ( DBusConnection *p_conn,
> > > DBusMessage *p_from,
> > > void *p_this );
> > > +int RootPropertiesChangedEmit ( intf_thread_t *,
> >
> > Trailing whitespace
> >
> > > + vlc_dictionary_t * );
> > >
> > > #endif //dbus-root.h
> >
> > These are trivial changes, so I can just amend your patch myself if you
> > want.
> >
> > Thanks !
> > All the best,
--
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/20120210/ea1d08c1/attachment.sig>
More information about the vlc-devel
mailing list