[vlc-devel] [PATCH v2 8/9] libvlc: player: make stop synchronous
Thomas Guillem
thomas at gllm.fr
Mon May 20 09:55:00 CEST 2019
On Mon, May 20, 2019, at 09:44, Steve Lhomme wrote:
> On 2019-05-17 20:22, Thomas Guillem wrote:
> >
> >
> > On Fri, May 17, 2019, at 11:32, Steve Lhomme wrote:
> >> On 2019-05-17 10:58, Romain Vimont wrote:
> >>> On Wed, May 15, 2019 at 01:31:14PM +0300, Rémi Denis-Courmont wrote:
> >>>> Hi,
> >>>>
> >>>> This looks like it'll dead lock if more than one thread calls the stop. Not sure if that worked in the current code or not.
> >>>
> >>> Yes, this is incorrect in that case.
> >>>
> >>> But since the callbacks are called with player locked, we necessarily
> >>> need to unlock the player one way or another to wait for the "stopped"
> >>> event.
> >>>
> >>> An alternative is to use a condition variable with the player mutex:
> >>>
> >>> vlc_player_Lock(player);
> >>> bool stop_async = vlc_player_Stop(player);
> >>> if (stop_async)
> >>> vlc_player_CondWait(player, &p_mi->cond_stop);
> >>> vlc_player_Unlock(player);
> >>>
> >>> But this is still not correct, because others vlc_player_Start() and
> >>> vlc_player_Stop() can be executed during vlc_player_CondWait().
> >>>
> >>> The problem is that we can't associate a specific stop request with the
> >>> right "player state change to stopped", so we would wake up all threads
> >>> waiting for stop on the first "state change to stopped".
> >>>
> >>> Since we don't want synchronous stop anymore (it was just for backward
> >>> compatibility), I suggest we just make libvlc_media_player_stop()
> >>> asynchronous.
> >>>
> >>> And we document breaking changes from libvlc 3 to libvlc 4:
> >>> - libvlc_media_player_will_play() is removed (it is meaningless with the
> >>> new player)
> >>> - libvlc_media_player callbacks are called with the same (recursive)
> >>> mutex as libvlc_media_player functions
> >>> - libvlc_media_player_stop is now asynchronous (it was the only blocking
> >>> function, it does not block anymore)
> >>
> >> IMO it's better to use a different name. You don't want old code to use
> >> the new behaviour incorrectly. Unless the signature of the function is
> >> different, then it shouldn't compile (but will wrappers in other
> >> languages notice?). IMO just remove the old behaviour and use a new name.
> >
> > +1
> > Remove stop and add stop_async.
> >
> > Is it possible with some macros to trigger an "#error use the stop_async version" ?
>
> How about :
>
> #define libvlc_media_player_stop() static_assert(false, "use the
> stop_async version" )
Nice !
>
> >>
> >>>> Le 15 mai 2019 12:53:16 GMT+03:00, Romain Vimont <rom1v at videolabs.io> a écrit :
> >>>>> To preserve the old behavior, make libvlc_media_player_stop()
> >>>>> synchronous.
> >>>>> ---
> >>>>> lib/media_player.c | 13 ++++++++++++-
> >>>>> lib/media_player_internal.h | 3 +++
> >>>>> 2 files changed, 15 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/lib/media_player.c b/lib/media_player.c
> >>>>> index 17d675e93b..e5f6fc32a7 100644
> >>>>> --- a/lib/media_player.c
> >>>>> +++ b/lib/media_player.c
> >>>>> @@ -108,6 +108,8 @@ on_state_changed(vlc_player_t *player, enum
> >>>>> vlc_player_state new_state,
> >>>>> switch (new_state) {
> >>>>> case VLC_PLAYER_STATE_STOPPED:
> >>>>> event.type = libvlc_MediaPlayerStopped;
> >>>>> + if (mp->stop_requested)
> >>>>> + vlc_sem_post(&mp->sem_stop);
> >>>>> break;
> >>>>> case VLC_PLAYER_STATE_STOPPING:
> >>>>> event.type = libvlc_MediaPlayerEndReached;
> >>>>> @@ -715,6 +717,9 @@ libvlc_media_player_new( libvlc_instance_t
> >>>>> *instance )
> >>>>>
> >>>>> vlc_player_Unlock(mp->player);
> >>>>>
> >>>>> + vlc_sem_init(&mp->sem_stop, 0);
> >>>>> + mp->stop_requested = false;
> >>>>> +
> >>>>> mp->i_refcount = 1;
> >>>>> libvlc_event_manager_init(&mp->event_manager, mp);
> >>>>>
> >>>>> @@ -796,6 +801,8 @@ static void libvlc_media_player_destroy(
> >>>>> libvlc_media_player_t *p_mi )
> >>>>> libvlc_event_manager_destroy(&p_mi->event_manager);
> >>>>> libvlc_media_release( p_mi->p_md );
> >>>>>
> >>>>> + vlc_sem_destroy(&p_mi->sem_stop);
> >>>>> +
> >>>>> vlc_http_cookie_jar_t *cookies = var_GetAddress( p_mi, "http-cookies"
> >>>>> );
> >>>>> if ( cookies )
> >>>>> {
> >>>>> @@ -972,9 +979,13 @@ void libvlc_media_player_stop(
> >>>>> libvlc_media_player_t *p_mi )
> >>>>> vlc_player_t *player = p_mi->player;
> >>>>> vlc_player_Lock(player);
> >>>>>
> >>>>> - vlc_player_Stop(player);
> >>>>> + bool stop_async = vlc_player_Stop(player);
> >>>>> + p_mi->stop_requested = stop_async;
> >>>>>
> >>>>> vlc_player_Unlock(player);
> >>>>> +
> >>>>> + if (stop_async)
> >>>>> + vlc_sem_wait(&p_mi->sem_stop);
> >>>>> }
> >>>>>
> >>>>> int libvlc_media_player_set_renderer( libvlc_media_player_t *p_mi,
> >>>>> diff --git a/lib/media_player_internal.h b/lib/media_player_internal.h
> >>>>> index c2d5348e22..a0c712780d 100644
> >>>>> --- a/lib/media_player_internal.h
> >>>>> +++ b/lib/media_player_internal.h
> >>>>> @@ -49,6 +49,9 @@ struct libvlc_media_player_t
> >>>>> struct libvlc_instance_t * p_libvlc_instance; /* Parent instance */
> >>>>> libvlc_media_t * p_md; /* current media descriptor */
> >>>>> libvlc_event_manager_t event_manager;
> >>>>> +
> >>>>> + vlc_sem_t sem_stop;
> >>>>> + bool stop_requested;
> >>>>> };
> >>>>>
> >>>>> libvlc_track_description_t * libvlc_get_track_description(
> >>>>> --
> >>>>> 2.20.1
> >>>>>
> >>>>> _______________________________________________
> >>>>> vlc-devel mailing list
> >>>>> To unsubscribe or modify your subscription options:
> >>>>> https://mailman.videolan.org/listinfo/vlc-devel
> >>>>
> >>>> --
> >>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> >>>
> >>>> _______________________________________________
> >>>> vlc-devel mailing list
> >>>> To unsubscribe or modify your subscription options:
> >>>> https://mailman.videolan.org/listinfo/vlc-devel
> >>>
> >>> _______________________________________________
> >>> vlc-devel mailing list
> >>> To unsubscribe or modify your subscription options:
> >>> https://mailman.videolan.org/listinfo/vlc-devel
> >>>
> >> _______________________________________________
> >> vlc-devel mailing list
> >> To unsubscribe or modify your subscription options:
> >> https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list