[vlc-devel] [PATCH] libvlc: fix MediaPlayerESSelected events not sent
Filip Roséen
filip at atch.se
Sun Apr 2 23:09:11 CEST 2017
Hi Thomas,
On 2017-03-31 15:25, Thomas Guillem wrote:
> This event was sent only when an ES was changed by libvlc. It was not sent when
> an ES was changed by the input thread. To fix this issue, listen to the
> INPUT_EVENT_ES input event (that is sent when an ES is added/deleted/selected)
> to detect an ES selection change.
>
> We don't want to trigger callbacks for "video-es"/"audio-es"/"spu-es" variables
> from the input_thread since it's also listening to these callbacks and we want
> to avoid a selecting busy loop.
> ---
> lib/media_player.c | 65 ++++++++++++++++++++++++---------------------
> lib/media_player_internal.h | 1 +
> 2 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/lib/media_player.c b/lib/media_player.c
> index 2c5aa727fb..6ab47254f7 100644
> --- a/lib/media_player.c
> +++ b/lib/media_player.c
> @@ -65,11 +65,6 @@ input_es_changed( vlc_object_t * p_this, char const * psz_cmd,
> void *p_userdata);
>
> static int
> -input_es_selected( vlc_object_t * p_this, char const * psz_cmd,
> - vlc_value_t oldval, vlc_value_t newval,
> - void * p_userdata );
> -
> -static int
> corks_changed(vlc_object_t *obj, const char *name, vlc_value_t old,
> vlc_value_t cur, void *opaque);
>
> @@ -411,6 +406,39 @@ input_event_changed( vlc_object_t * p_this, char const * psz_cmd,
> event.u.media_player_chapter_changed.new_chapter = var_GetInteger( p_input, "chapter" );
> libvlc_event_send( p_mi->p_event_manager, &event );
> }
> + else if ( newval.i_int == INPUT_EVENT_ES )
> + {
> + /* Send ESSelected events from this callback ("intf-event") and not
> + * from "audio-es", "video-es", "spu-es" callbacks. Indeed, these
> + * callbacks are not triggered when the input_thread changes an ES
> + * while this callback is. */
> +
> + int new_es[3] = { -2, -2, -2 }; /* -1 is reserved for deselect */
> + static const char *es_names[3] = { "audio-es", "video-es", "spu-es" };
> + static const int es_types[3] = { libvlc_track_audio, libvlc_track_video,
> + libvlc_track_text };
As these two arrays are tightly coupled, why not turn them into one
array of unnamed structs having both `const char*` and `const int` as
*data-members*?
> + /* Check if an es selection changed */
> + lock( p_mi );
> + for( int i = 0; i < 3; ++i ) /* for audio, video, spu */
> + {
> + int current_es = var_GetInteger( p_input, es_names[i] );
> + if( current_es != p_mi->selected_es[i] )
> + new_es[i] = p_mi->selected_es[i] = current_es;
> + }
> + unlock( p_mi );
> +
> + /* Send the ESSelected event for each ES that were newly selected */
> + for( int i = 0; i < 3; ++i ) /* for audio, video, spu */
> + {
> + if( new_es[i] != -2 )
> + {
Honestly I am not a super big fan of using `-2` as a magic number, but
my soul would rest a little easier if it was at least denoted by a
named constant (to hide the mischief).
> + event.type = libvlc_MediaPlayerESSelected;
> + event.u.media_player_es_changed.i_type = es_types[i];
> + event.u.media_player_es_changed.i_id = new_es[i];
> + libvlc_event_send( p_mi->p_event_manager, &event );
> + }
> + }
> + }
>
> return VLC_SUCCESS;
> }
> @@ -475,25 +503,6 @@ static int input_es_changed( vlc_object_t *p_this,
> return VLC_SUCCESS;
> }
>
> -static int
> -input_es_selected( vlc_object_t * p_this, char const * psz_cmd,
> - vlc_value_t oldval, vlc_value_t newval,
> - void * p_userdata )
> -{
> - VLC_UNUSED(p_this);
> - VLC_UNUSED(oldval);
> - libvlc_media_player_t *mp = p_userdata;
> - libvlc_event_t event;
> -
> - event.type = libvlc_MediaPlayerESSelected;
> - event.u.media_player_es_changed.i_type = track_type_from_name(psz_cmd);
> - event.u.media_player_es_changed.i_id = newval.i_int;
> -
> - libvlc_event_send(mp->p_event_manager, &event);
> -
> - return VLC_SUCCESS;
> -}
> -
> /**************************************************************************
> * Snapshot Taken Event.
> *
> @@ -916,9 +925,6 @@ static void add_es_callbacks( input_thread_t *p_input_thread, libvlc_media_playe
> var_AddListCallback( p_input_thread, "video-es", input_es_changed, p_mi );
> var_AddListCallback( p_input_thread, "audio-es", input_es_changed, p_mi );
> var_AddListCallback( p_input_thread, "spu-es", input_es_changed, p_mi );
> - var_AddCallback( p_input_thread, "video-es", input_es_selected, p_mi );
> - var_AddCallback( p_input_thread, "audio-es", input_es_selected, p_mi );
> - var_AddCallback( p_input_thread, "spu-es", input_es_selected, p_mi );
> }
>
> static void del_es_callbacks( input_thread_t *p_input_thread, libvlc_media_player_t *p_mi )
> @@ -926,9 +932,6 @@ static void del_es_callbacks( input_thread_t *p_input_thread, libvlc_media_playe
> var_DelListCallback( p_input_thread, "video-es", input_es_changed, p_mi );
> var_DelListCallback( p_input_thread, "audio-es", input_es_changed, p_mi );
> var_DelListCallback( p_input_thread, "spu-es", input_es_changed, p_mi );
> - var_DelCallback( p_input_thread, "video-es", input_es_selected, p_mi );
> - var_DelCallback( p_input_thread, "audio-es", input_es_selected, p_mi );
> - var_DelCallback( p_input_thread, "spu-es", input_es_selected, p_mi );
> }
>
> /**************************************************************************
> @@ -958,6 +961,8 @@ int libvlc_media_player_play( libvlc_media_player_t *p_mi )
> return -1;
> }
>
> + memset( p_mi->selected_es, -2, sizeof(p_mi->selected_es) );
> +
The above is not correct as it will set every *byte* in
`p_mi->selected_is` to `-2`, it will not set each of the individual
elements in the array to `-2` (as `sizeof p_mi->selected_es[0] > 1`).
> media_attach_preparsed_event( p_mi->p_md );
>
> p_input_thread = input_Create( p_mi, p_mi->p_md->p_input_item, NULL,
> diff --git a/lib/media_player_internal.h b/lib/media_player_internal.h
> index 1eca3c3f76..a5fa20dc0a 100644
> --- a/lib/media_player_internal.h
> +++ b/lib/media_player_internal.h
> @@ -55,6 +55,7 @@ struct libvlc_media_player_t
> libvlc_event_manager_t * p_event_manager;
> libvlc_state_t state;
> vlc_viewpoint_t viewpoint;
> + int selected_es[3];
> };
>
> /* Media player - audio, video */
> --
> 2.11.0
I do not understand why *libvlc* must "cache" the selected ES to
prevent a selection event of the same ES bubbling up twice. If we have
code that spawns an "ES selected" event for an ES that is already
selected (without explicit interaction), that code should be fixed.
If all event-posting is correct in the core, why is it not enough to
simply "forward" these events as they come in (instead of having a
cache filtering out duplicates)?
Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170402/4dcee9d0/attachment.html>
More information about the vlc-devel
mailing list