[vlc-devel] [PATCH] libvlc: fix MediaPlayerESSelected events not sent
Thomas Guillem
thomas at gllm.fr
Mon Apr 3 09:53:55 CEST 2017
On Sun, Apr 2, 2017, at 23:09, Filip Roséen wrote:
> 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*?
OK,
>> + /* 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).
Then I have to declare a define / const for the whole file, I
don't know.
>> + 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).
Oops, yes, will fix.
>> 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.
This commit is more a hack for 3.0. We may want to rewrite event
handling for 4.0 or 5.0. We need to "cache" because this event is not
sent only for selected es events.
> 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)?
Unfortunately, no, see patch comments and commit log.
> Best Regards,
> Filip
> _________________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170403/48145ecb/attachment.html>
More information about the vlc-devel
mailing list