[vlc-devel] [PATCH v2 8/9] libvlc: player: make stop synchronous

[Thomas Guillem]

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" ?

> 
> >> 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