[vlc-devel] [PATCH] player: rework aout/vout callbacks

Thomas Guillem thomas at gllm.fr
Fri Aug 23 10:14:48 CEST 2019


Document that vlc_player_t functions should not be called from these callbacks.
A player function could trigger an action on the vout/aout and cause a callback
to be called => deadlock.

Remove the vlc_player_t * argument, it was never used.

Add the audio_output_t *argument for aout callbacks. It's not currently used,
it is added as symmetry with vout callbacks.
---
 include/vlc_player.h                          | 42 ++++++++-----------
 lib/media_player.c                            | 12 +++---
 modules/control/dbus/dbus.c                   | 20 ++++-----
 modules/control/rc.c                          |  4 +-
 .../gui/macosx/playlist/VLCPlayerController.m | 16 +++----
 .../gui/qt/components/audio_device_model.cpp  |  2 +-
 .../gui/qt/components/player_controller.cpp   |  8 ++--
 modules/gui/skins2/src/vlcproc.cpp            |  8 ++--
 src/player/aout.c                             | 10 +++--
 src/player/vout.c                             |  2 +-
 10 files changed, 58 insertions(+), 66 deletions(-)

diff --git a/include/vlc_player.h b/include/vlc_player.h
index 401eafd047..bf79cdba6e 100644
--- a/include/vlc_player.h
+++ b/include/vlc_player.h
@@ -2115,11 +2115,8 @@ typedef struct vlc_player_aout_listener_id vlc_player_aout_listener_id;
  *
  * Can be registered with vlc_player_aout_AddListener().
  *
- * These callbacks are *not* called with the player locked. It is safe to lock
- * the player and call any vlc_player functions from these callbacks.
- *
- * @warning To avoid deadlocks, users should never call audio_output_t
- * functions from these callbacks.
+ * @warning To avoid deadlocks, users should never call audio_output_t and
+ * vlc_player_t functions from these callbacks.
  */
 struct vlc_player_aout_cbs
 {
@@ -2128,34 +2125,34 @@ struct vlc_player_aout_cbs
      *
      * @see vlc_player_aout_SetVolume()
      *
-     * @param player unlocked player instance
+     * @param aout the main aout of the player
      * @param new_volume volume in the range [0;2.f]
      * @param data opaque pointer set by vlc_player_aout_AddListener()
      */
-    void (*on_volume_changed)(vlc_player_t *player,
-        float new_volume, void *data);
+    void (*on_volume_changed)(audio_output_t *aout, float new_volume,
+        void *data);
 
     /**
      * Called when the mute state has changed
      *
      * @see vlc_player_aout_Mute()
      *
-     * @param player unlocked player instance
+     * @param aout the main aout of the player
      * @param new_mute true if muted
      * @param data opaque pointer set by vlc_player_aout_AddListener()
      */
-    void (*on_mute_changed)(vlc_player_t *player,
-        bool new_muted, void *data);
+    void (*on_mute_changed)(audio_output_t *aout, bool new_muted,
+        void *data);
 
     /**
      * Called when the audio device has changed
      *
-     * @param player unlocked player instance
+     * @param aout the main aout of the player
      * @param device the device name
      * @param data opaque pointer set by vlc_player_aout_AddListener()
      */
-    void (*on_device_changed)(vlc_player_t *player, const char *device,
-                              void *data);
+    void (*on_device_changed)(audio_output_t *aout, const char *device,
+        void *data);
 };
 
 /**
@@ -2330,15 +2327,12 @@ enum vlc_player_vout_action
  *
  * Can be registered with vlc_player_vout_AddListener().
  *
- * These callbacks are *not* called with the player locked. It is safe to lock
- * the player and call any vlc_player functions from these callbacks.
- *
  * @note The state changed from the callbacks can be either applied on the
  * player (and all future video outputs), or on a specified video output. The
  * state is applied on the player when the vout argument is NULL.
  *
- * @warning To avoid deadlocks, users should never call vout_thread_t functions
- * from these callbacks.
+ * @warning To avoid deadlocks, users should never call vout_thread_t and
+ * vlc_player_t functions from these callbacks.
  */
 struct vlc_player_vout_cbs
 {
@@ -2347,26 +2341,24 @@ struct vlc_player_vout_cbs
      *
      * @see vlc_player_vout_SetFullscreen()
      *
-     * @param player unlocked player instance
      * @param vout cf. vlc_player_vout_cbs note
      * @param enabled true when fullscreen is enabled
      * @param data opaque pointer set by vlc_player_vout_AddListener()
      */
-    void (*on_fullscreen_changed)(vlc_player_t *player,
-        vout_thread_t *vout, bool enabled, void *data);
+    void (*on_fullscreen_changed)(vout_thread_t *vout, bool enabled,
+        void *data);
 
     /**
      * Called when the player and/or vout wallpaper mode has changed
      *
      * @see vlc_player_vout_SetWallpaperModeEnabled()
      *
-     * @param player unlocked player instance
      * @param vout cf. vlc_player_vout_cbs note
      * @param enabled true when wallpaper mode is enabled
      * @param data opaque pointer set by vlc_player_vout_AddListener()
      */
-    void (*on_wallpaper_mode_changed)(vlc_player_t *player,
-        vout_thread_t *vout, bool enabled, void *data);
+    void (*on_wallpaper_mode_changed)(vout_thread_t *vout, bool enabled,
+        void *data);
 };
 
 
diff --git a/lib/media_player.c b/lib/media_player.c
index 199d91dbba..c4faffd34a 100644
--- a/lib/media_player.c
+++ b/lib/media_player.c
@@ -430,9 +430,9 @@ on_vout_changed(vlc_player_t *player, enum vlc_player_vout_action action,
 // player aout callbacks
 
 static void
-on_volume_changed(vlc_player_t *player, float new_volume, void *data)
+on_volume_changed(audio_output_t *aout, float new_volume, void *data)
 {
-    (void) player;
+    (void) aout;
 
     libvlc_media_player_t *mp = data;
 
@@ -444,9 +444,9 @@ on_volume_changed(vlc_player_t *player, float new_volume, void *data)
 }
 
 static void
-on_mute_changed(vlc_player_t *player, bool new_muted, void *data)
+on_mute_changed(audio_output_t *aout, bool new_muted, void *data)
 {
-    (void) player;
+    (void) aout;
 
     libvlc_media_player_t *mp = data;
 
@@ -458,9 +458,9 @@ on_mute_changed(vlc_player_t *player, bool new_muted, void *data)
 }
 
 static void
-on_audio_device_changed(vlc_player_t *player, const char *device, void *data)
+on_audio_device_changed(audio_output_t *aout, const char *device, void *data)
 {
-    (void) player;
+    (void) aout;
 
     libvlc_media_player_t *mp = data;
 
diff --git a/modules/control/dbus/dbus.c b/modules/control/dbus/dbus.c
index af6e4dc58f..ed386bdbba 100644
--- a/modules/control/dbus/dbus.c
+++ b/modules/control/dbus/dbus.c
@@ -151,11 +151,10 @@ static void player_on_position_changed(vlc_player_t *,
 static void player_on_media_meta_changed(vlc_player_t *,
                                          input_item_t *, void *);
 
-static void player_aout_on_volume_changed(vlc_player_t *, float, void *);
-static void player_aout_on_mute_changed(vlc_player_t *, bool, void *);
+static void player_aout_on_volume_changed(audio_output_t *, float, void *);
+static void player_aout_on_mute_changed(audio_output_t *, bool, void *);
 
-static void player_vout_on_fullscreen_changed(vlc_player_t *,
-                                              vout_thread_t *, bool, void *);
+static void player_vout_on_fullscreen_changed(vout_thread_t *, bool, void *);
 
 /*****************************************************************************
  * Module descriptor
@@ -1142,28 +1141,27 @@ player_on_media_meta_changed(vlc_player_t *player,
 }
 
 static void
-player_aout_on_volume_changed(vlc_player_t *player, float volume, void *data)
+player_aout_on_volume_changed(audio_output_t *aout, float volume, void *data)
 {
     add_event_signal(data,
             &(callback_info_t){ .signal = SIGNAL_VOLUME_CHANGE });
-    (void) player; (void) volume;
+    (void) aout; (void) volume;
 }
 
 static void
-player_aout_on_mute_changed(vlc_player_t *player, bool muted, void *data)
+player_aout_on_mute_changed(audio_output_t *aout, bool muted, void *data)
 {
     add_event_signal(data,
             &(callback_info_t){ .signal = SIGNAL_VOLUME_MUTED });
-    (void) player; (void) muted;
+    (void) aout; (void) muted;
 }
 
 static void
-player_vout_on_fullscreen_changed(vlc_player_t *player,
-                                  vout_thread_t *vout, bool enabled,
+player_vout_on_fullscreen_changed(vout_thread_t *vout, bool enabled,
                                   void *data)
 {
     add_event_signal(data, &(callback_info_t){ .signal = SIGNAL_FULLSCREEN });
-    (void) player; (void) vout; (void) enabled;
+    (void) vout; (void) enabled;
 }
 
 /*****************************************************************************
diff --git a/modules/control/rc.c b/modules/control/rc.c
index 29382cda56..cd85602d59 100644
--- a/modules/control/rc.c
+++ b/modules/control/rc.c
@@ -353,8 +353,8 @@ player_on_position_changed(vlc_player_t *player,
 }
 
 static void
-player_aout_on_volume_changed(vlc_player_t *player, float volume, void *data)
-{ VLC_UNUSED(player);
+player_aout_on_volume_changed(audio_output_t *aout, float volume, void *data)
+{ VLC_UNUSED(aout);
     intf_thread_t *p_intf = data;
     vlc_mutex_lock(&p_intf->p_sys->status_lock);
     msg_rc(STATUS_CHANGE "( audio volume: %ld )",
diff --git a/modules/gui/macosx/playlist/VLCPlayerController.m b/modules/gui/macosx/playlist/VLCPlayerController.m
index 874d2ea31a..e76b88ceab 100644
--- a/modules/gui/macosx/playlist/VLCPlayerController.m
+++ b/modules/gui/macosx/playlist/VLCPlayerController.m
@@ -506,18 +506,18 @@ static const struct vlc_player_cbs player_callbacks = {
 
 #pragma mark - video specific callback implementations
 
-static void cb_player_vout_fullscreen_changed(vlc_player_t *p_player, vout_thread_t *p_vout, bool isFullscreen, void *p_data)
+static void cb_player_vout_fullscreen_changed(vout_thread_t *p_vout, bool isFullscreen, void *p_data)
 {
-    VLC_UNUSED(p_player); VLC_UNUSED(p_vout);
+    VLC_UNUSED(p_vout);
     dispatch_async(dispatch_get_main_queue(), ^{
         VLCPlayerController *playerController = (__bridge VLCPlayerController *)p_data;
         [playerController fullscreenChanged:isFullscreen];
     });
 }
 
-static void cb_player_vout_wallpaper_mode_changed(vlc_player_t *p_player, vout_thread_t *p_vout,  bool wallpaperModeEnabled, void *p_data)
+static void cb_player_vout_wallpaper_mode_changed(vout_thread_t *p_vout,  bool wallpaperModeEnabled, void *p_data)
 {
-    VLC_UNUSED(p_player); VLC_UNUSED(p_vout);
+    VLC_UNUSED(p_vout);
     dispatch_async(dispatch_get_main_queue(), ^{
         VLCPlayerController *playerController = (__bridge VLCPlayerController *)p_data;
         [playerController wallpaperModeChanged:wallpaperModeEnabled];
@@ -531,18 +531,18 @@ static const struct vlc_player_vout_cbs player_vout_callbacks = {
 
 #pragma mark - video specific callback implementations
 
-static void cb_player_aout_volume_changed(vlc_player_t *p_player, float volume, void *p_data)
+static void cb_player_aout_volume_changed(audio_output_t *aout, float volume, void *p_data)
 {
-    VLC_UNUSED(p_player);
+    VLC_UNUSED(aout);
     dispatch_async(dispatch_get_main_queue(), ^{
         VLCPlayerController *playerController = (__bridge VLCPlayerController *)p_data;
         [playerController volumeChanged:volume];
     });
 }
 
-static void cb_player_aout_mute_changed(vlc_player_t *p_player, bool muted, void *p_data)
+static void cb_player_aout_mute_changed(audio_output_t *aout, bool muted, void *p_data)
 {
-    VLC_UNUSED(p_player);
+    VLC_UNUSED(aout);
     dispatch_async(dispatch_get_main_queue(), ^{
         VLCPlayerController *playerController = (__bridge VLCPlayerController *)p_data;
         [playerController muteChanged:muted];
diff --git a/modules/gui/qt/components/audio_device_model.cpp b/modules/gui/qt/components/audio_device_model.cpp
index 3c40047786..cb2b9bfdf4 100644
--- a/modules/gui/qt/components/audio_device_model.cpp
+++ b/modules/gui/qt/components/audio_device_model.cpp
@@ -20,7 +20,7 @@
 
 extern "C" {
 
-static void on_player_aout_device_changed(vlc_player_t *,const char *device, void *data)
+static void on_player_aout_device_changed(audio_output_t *,const char *device, void *data)
 {
     AudioDeviceModel* that = static_cast<AudioDeviceModel*>(data);
     QMetaObject::invokeMethod(that, [that, device=QString::fromUtf8(device)](){
diff --git a/modules/gui/qt/components/player_controller.cpp b/modules/gui/qt/components/player_controller.cpp
index e73fc7e8ab..3938b09692 100644
--- a/modules/gui/qt/components/player_controller.cpp
+++ b/modules/gui/qt/components/player_controller.cpp
@@ -790,7 +790,7 @@ static void on_player_vout_changed(vlc_player_t *player, enum vlc_player_vout_ac
 
 //player vout callbacks
 
-static void on_player_vout_fullscreen_changed(vlc_player_t *, vout_thread_t* vout, bool is_fullscreen, void *data)
+static void on_player_vout_fullscreen_changed(vout_thread_t* vout, bool is_fullscreen, void *data)
 {
     PlayerControllerPrivate* that = static_cast<PlayerControllerPrivate*>(data);
     msg_Dbg( that->p_intf, "on_player_vout_fullscreen_changed %s", is_fullscreen ? "fullscreen" : "windowed");
@@ -808,7 +808,7 @@ static void on_player_vout_fullscreen_changed(vlc_player_t *, vout_thread_t* vou
     });
 }
 
-static void on_player_vout_wallpaper_mode_changed(vlc_player_t *, vout_thread_t* vout, bool enabled, void *data)
+static void on_player_vout_wallpaper_mode_changed(vout_thread_t* vout, bool enabled, void *data)
 {
     PlayerControllerPrivate* that = static_cast<PlayerControllerPrivate*>(data);
     msg_Dbg( that->p_intf, "on_player_vout_wallpaper_mode_changed");
@@ -828,7 +828,7 @@ static void on_player_vout_wallpaper_mode_changed(vlc_player_t *, vout_thread_t*
 
 //player aout callbacks
 
-static void on_player_aout_volume_changed(vlc_player_t *, float volume, void *data)
+static void on_player_aout_volume_changed(audio_output_t *, float volume, void *data)
 {
     PlayerControllerPrivate* that = static_cast<PlayerControllerPrivate*>(data);
     msg_Dbg( that->p_intf, "on_player_aout_volume_changed");
@@ -838,7 +838,7 @@ static void on_player_aout_volume_changed(vlc_player_t *, float volume, void *da
     });
 }
 
-static void on_player_aout_mute_changed(vlc_player_t *, bool muted, void *data)
+static void on_player_aout_mute_changed(audio_output_t *, bool muted, void *data)
 {
     PlayerControllerPrivate* that = static_cast<PlayerControllerPrivate*>(data);
     msg_Dbg( that->p_intf, "on_player_aout_mute_changed");
diff --git a/modules/gui/skins2/src/vlcproc.cpp b/modules/gui/skins2/src/vlcproc.cpp
index fc276d21e5..495d32e79d 100644
--- a/modules/gui/skins2/src/vlcproc.cpp
+++ b/modules/gui/skins2/src/vlcproc.cpp
@@ -259,17 +259,17 @@ void on_player_vout_changed( vlc_player_t *player,
     VlcProc::onGenericCallback( "vout", val, data );
 }
 
-void on_player_aout_volume_changed( vlc_player_t *player,
+void on_player_aout_volume_changed( audio_output *aout,
                                     float volume, void *data )
 {
-    (void)player;
+    (void)aout;
     vlc_value_t val = { .f_float = volume };
     VlcProc::onGenericCallback( "volume", val, data );
 }
 
-void on_player_aout_mute_changed( vlc_player_t *player, bool mute, void *data )
+void on_player_aout_mute_changed( audio_output_t *aout, bool mute, void *data )
 {
-    (void)player;
+    (void)aout;
     vlc_value_t val = { .b_bool = mute};
     VlcProc::onGenericCallback( "mute", val, data );
 }
diff --git a/src/player/aout.c b/src/player/aout.c
index 2007f86e24..8d19d04ba2 100644
--- a/src/player/aout.c
+++ b/src/player/aout.c
@@ -35,7 +35,7 @@
     vlc_list_foreach(listener, &player->aout_listeners, node) \
     { \
         if (listener->cbs->event) \
-            listener->cbs->event(player, ##__VA_ARGS__, listener->cbs_data); \
+            listener->cbs->event(__VA_ARGS__, listener->cbs_data); \
     } \
     vlc_mutex_unlock(&player->aout_listeners_lock); \
 } while(0)
@@ -89,7 +89,8 @@ vlc_player_AoutCallback(vlc_object_t *this, const char *var,
     {
         if (oldval.f_float != newval.f_float)
         {
-            vlc_player_aout_SendEvent(player, on_volume_changed, newval.f_float);
+            vlc_player_aout_SendEvent(player, on_volume_changed,
+                                      (audio_output_t *)this, newval.f_float);
             vlc_player_osd_Volume(player, false);
         }
     }
@@ -97,7 +98,8 @@ vlc_player_AoutCallback(vlc_object_t *this, const char *var,
     {
         if (oldval.b_bool != newval.b_bool)
         {
-            vlc_player_aout_SendEvent(player, on_mute_changed, newval.b_bool);
+            vlc_player_aout_SendEvent(player, on_mute_changed,
+                                      (audio_output_t *)this, newval.b_bool);
             vlc_player_osd_Volume(player, true);
         }
     }
@@ -108,7 +110,7 @@ vlc_player_AoutCallback(vlc_object_t *this, const char *var,
         /* support NULL values for string comparison */
         if (old != new && (!old || !new || strcmp(old, new)))
             vlc_player_aout_SendEvent(player, on_device_changed,
-                                      newval.psz_string);
+                                      (audio_output_t *)this, newval.psz_string);
     }
     else
         vlc_assert_unreachable();
diff --git a/src/player/vout.c b/src/player/vout.c
index 9c8e55d345..b28599288f 100644
--- a/src/player/vout.c
+++ b/src/player/vout.c
@@ -34,7 +34,7 @@
     vlc_list_foreach(listener, &player->vout_listeners, node) \
     { \
         if (listener->cbs->event) \
-            listener->cbs->event(player, ##__VA_ARGS__, listener->cbs_data); \
+            listener->cbs->event(__VA_ARGS__, listener->cbs_data); \
     } \
     vlc_mutex_unlock(&player->vout_listeners_lock); \
 } while(0)
-- 
2.20.1



More information about the vlc-devel mailing list