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

[Steve Lhomme]

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.

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