[vlc-devel] [RFC] core: player: make the lock optionally reentrant

Romain Vimont rom1v at videolabs.io
Mon May 13 16:27:58 CEST 2019 

On Mon, May 13, 2019 at 05:06:45PM +0300, Rémi Denis-Courmont wrote:
> Hi,
> 
> Recursive locking only addresses backward compatibility in the corner case than the call is from the same thread. Since that was never a requirement before, that's not backward compatible.

In practice, I think this is always the case. Anyway, this is the case I
want to continue to support with this change.

So, yes, that is weaker than "backward compatible", but we can break the
API for VLC 4.0. At the same time, at least for now, we want to minimize
breaking changes to the bare minimum because we don't want to rewrite
all libvlc clients, so we could say "as backward compatible as possible"
instead.

> And if the event is blocking-dispatched to another thread from within the 
> event callback, then it will deadlock regardless of build type.

If by "blocking-dispatched", you mean dispatch the event to another
thread and wait for a response, I think we should make explicit that we
must never block/wait from a callback.

> Le 13 mai 2019 16:49:22 GMT+03:00, Romain Vimont <rom1v at videolabs.io> a écrit :
> >On Mon, May 13, 2019 at 04:25:46PM +0300, Rémi Denis-Courmont wrote:
> >> Hi,
> >> 
> >> I don't see the point in adding a parameter here. If you want to use
> >libpulse-like semantics, just make the lock recursive.
> >
> >The idea here is to use a recursive lock only for libvlc, and keep the
> >"normal" player lock otherwise.
> >
> >> But in any case, this won't provide backward compatibility.
> >
> >This will allow to call functions like libvlc_audio_get_track_count()
> >from a libvlc player callback like before.
> >
> >> 
> >> Le 13 mai 2019 15:47:36 GMT+03:00, Romain Vimont <rom1v at videolabs.io>
> >a écrit :
> >> >This will allow to implement the old libvlc API using the new
> >player.
> >> >
> >>
> >><https://mailman.videolan.org/pipermail/vlc-devel/2019-May/124461.html>
> >> >---
> >> > include/vlc_player.h  | 25 ++++++++++++++++++++++++-
> >> > src/input/player.c    | 12 ++++++++----
> >> > src/playlist/player.c |  3 ++-
> >> > 3 files changed, 34 insertions(+), 6 deletions(-)
> >> >
> >> >diff --git a/include/vlc_player.h b/include/vlc_player.h
> >> >index 1aa5f8e687..d4c49fa89d 100644
> >> >--- a/include/vlc_player.h
> >> >+++ b/include/vlc_player.h
> >> >@@ -331,6 +331,29 @@ enum vlc_player_subtitle_sync
> >> >     VLC_PLAYER_SUBTITLE_SYNC_APPLY,
> >> > };
> >> > 
> >> >+/**
> >> >+ * Player lock type (normal or reentrant)
> >> >+ */
> >> >+enum vlc_player_lock_type
> >> >+{
> >> >+    /**
> >> >+     * Normal lock
> >> >+     *
> >> >+     * If the player is already locked, subsequent calls to
> >> >vlc_player_Lock()
> >> >+     * will deadlock.
> >> >+     */
> >> >+    VLC_PLAYER_LOCK_NORMAL,
> >> >+
> >> >+    /**
> >> >+     * Reentrant lock
> >> >+     *
> >> >+     * If the player is already locked, subsequent calls to
> >> >vlc_player_Lock()
> >> >+     * will still succeed. To unlock the player, one call to
> >> >+     * vlc_player_Unlock() per vlc_player_Lock() is necessary.
> >> >+     */
> >> >+    VLC_PLAYER_LOCK_REENTRANT,
> >> >+};
> >> >+
> >> > /** Player capability: can seek */
> >> > #define VLC_PLAYER_CAP_SEEK (1<<0)
> >> > /** Player capability: can pause */
> >> >@@ -1005,7 +1028,7 @@
> >vlc_player_title_list_GetAt(vlc_player_title_list
> >> >*titles, size_t idx);
> >> >* @return a pointer to a valid player instance or NULL in case of
> >error
> >> >  */
> >> > VLC_API vlc_player_t *
> >> >-vlc_player_New(vlc_object_t *parent,
> >> >+vlc_player_New(vlc_object_t *parent, enum vlc_player_lock_type
> >> >lock_type,
> >> >                const struct vlc_player_media_provider
> >*media_provider,
> >> >                void *media_provider_data);
> >> > 
> >> >diff --git a/src/input/player.c b/src/input/player.c
> >> >index da02375a66..e01701ba65 100644
> >> >--- a/src/input/player.c
> >> >+++ b/src/input/player.c
> >> >@@ -3367,9 +3367,13 @@ vlc_player_vout_Snapshot(vlc_player_t
> >*player)
> >> > }
> >> > 
> >> > static void
> >> >-vlc_player_InitLocks(vlc_player_t *player)
> >> >+vlc_player_InitLocks(vlc_player_t *player, enum
> >vlc_player_lock_type
> >> >lock_type)
> >> > {
> >> >-    vlc_mutex_init(&player->lock);
> >> >+    if (lock_type == VLC_PLAYER_LOCK_REENTRANT)
> >> >+        vlc_mutex_init_recursive(&player->lock);
> >> >+    else
> >> >+        vlc_mutex_init(&player->lock);
> >> >+
> >> >     vlc_mutex_init(&player->vout_listeners_lock);
> >> >     vlc_mutex_init(&player->aout_listeners_lock);
> >> >     vlc_cond_init(&player->start_delay_cond);
> >> >@@ -3426,7 +3430,7 @@ vlc_player_Delete(vlc_player_t *player)
> >> > }
> >> > 
> >> > vlc_player_t *
> >> >-vlc_player_New(vlc_object_t *parent,
> >> >+vlc_player_New(vlc_object_t *parent, enum vlc_player_lock_type
> >> >lock_type,
> >> >                const struct vlc_player_media_provider
> >*media_provider,
> >> >                void *media_provider_data)
> >> > {
> >> >@@ -3514,7 +3518,7 @@ vlc_player_New(vlc_object_t *parent,
> >> >     }
> >> > 
> >> >     player->deleting = false;
> >> >-    vlc_player_InitLocks(player);
> >> >+    vlc_player_InitLocks(player, lock_type);
> >> > 
> >> >if (vlc_clone(&player->destructor.thread,
> >vlc_player_destructor_Thread,
> >> >                   player, VLC_THREAD_PRIORITY_LOW) != 0)
> >> >diff --git a/src/playlist/player.c b/src/playlist/player.c
> >> >index bbdc39ba0e..8e3d94e2e8 100644
> >> >--- a/src/playlist/player.c
> >> >+++ b/src/playlist/player.c
> >> >@@ -123,7 +123,8 @@ static const struct vlc_player_cbs
> >player_callbacks
> >> >= {
> >> > bool
> >> >vlc_playlist_PlayerInit(vlc_playlist_t *playlist, vlc_object_t
> >*parent)
> >> > {
> >> >-    playlist->player = vlc_player_New(parent,
> >&player_media_provider,
> >> >playlist);
> >> >+    playlist->player = vlc_player_New(parent,
> >VLC_PLAYER_LOCK_NORMAL,
> >> >+                                      &player_media_provider,
> >> >playlist);
> >> >     if (unlikely(!playlist->player))
> >> >         return false;
> >> > 
> >> >-- 
> >> >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
> 
> -- 
> 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

More information about the vlc-devel mailing list