[vlc-devel] [PATCH v2 8/9] libvlc: player: make stop synchronous
Steve Lhomme
robux4 at ycbcr.xyz
Mon May 20 09:43:54 CEST 2019
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" )
>>
>>>> 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
>
More information about the vlc-devel
mailing list