[vlc-devel] [PATCH v2 8/9] libvlc: player: make stop synchronous
Rémi Denis-Courmont
remi at remlab.net
Mon May 20 14:09:46 CEST 2019
Hi,
It is possible with proprietary attributes, and we already have examples, but not with standard C/C++.
I think you can just remove the function though. Nowadays most compilers and IDE can suggest close identifier matches.
Le 17 mai 2019 21:22:03 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>
>
>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
>_______________________________________________
>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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20190520/9f6bd921/attachment.html>
More information about the vlc-devel
mailing list