[vlc-commits] [Git][videolan/vlc][master] skins2: skin_main: fix window closing after interface
Steve Lhomme (@robUx4)
gitlab at videolan.org
Fri May 20 06:23:55 UTC 2022
Steve Lhomme pushed to branch master at VideoLAN / VLC
Commits:
a928eb17 by Alexandre Janniaux at 2022-05-20T06:03:01+00:00
skins2: skin_main: fix window closing after interface
The interface cannot be closed before the vout_window_t object is
disabled, since the playlist/player is running from outside the
interfaces.
When it happens, a warning was raised from skins2 and the OpenGL code
would stay stuck waiting for xcb events, preventing the full closing of
the interface resources and delaying the close of the application itself.
This commit ensures that no skins2 window is enabled before leaving the
Close function from the interface, and no new skins2 window will be
enabled in a racy pattern while or after the Close function from the
interface is running.
The issue was highlighted in previous commit, including the commit
04409ebd456c67bfe4f5827c2116359f955b596e which required in its commit
description that interface modules should outlive the vout window
instances they provide, but didn't provide a mechanism to do so.
The added mutex was removed in 6083d3f103d31bafffeb99538f88229024eb6d6b
because it was useless as-is. This commit adds a reference count of
opened window, and a wait for this reference count to reach zero before
ending the interface Close() function, which justify the use of a mutex.
Note also that this patch uses the Enable/Disable lifecycle of the
window to manage the reference count and skins2 resources, because
there is an inversion between the playlist destruction and the
interface destruction.
We cannot wait for the vout_window_t object to be destroyed while we're
waiting in the interface Close(), since the vout is potentially reused
up to the playlist destruction (or more precisely, input resource
destruction) which can only happen after every interface is destroyed.
The implementation here is a bit different to what has been done for
other interfaces like Qt. The Qt interface is creating a new libvlc
object and stashes the interface state there, while refcounting it.
It means that the interface implementation itself will continue to exist
even if the interface object is dead. It has particular benefit when
the interface is not the usual interface but a dialog provider one.
The submitted approach here tries to keep the lifecycle of the
interface tied together with its object, to simplify the use of its
object for variable or logging, and avoid unnecessary object allocation.
- - - - -
1 changed file:
- modules/gui/skins2/src/skin_main.cpp
Changes:
=====================================
modules/gui/skins2/src/skin_main.cpp
=====================================
@@ -33,6 +33,7 @@
#include <vlc_playlist.h>
#include <vlc_threads.h>
#include <vlc_window.h>
+#include <vlc_cxx_helpers.hpp>
#include "dialogs.hpp"
#include "os_factory.hpp"
@@ -64,6 +65,9 @@ static void Close ( vlc_object_t * );
static void *Run ( void * );
static std::atomic<intf_thread_t *> skin_load_intf;
+static vlc::threads::mutex skin_load_lock;
+static vlc::threads::condition_variable skin_load_wait;
+static uintptr_t skin_load_rc = 0;
//---------------------------------------------------------------------------
// Open: initialize interface
@@ -110,6 +114,7 @@ static int Open( vlc_object_t *p_this )
return VLC_EGENERIC;
}
+ vlc::threads::mutex_locker guard {skin_load_lock};
skin_load_intf = p_intf;
return VLC_SUCCESS;
}
@@ -129,7 +134,15 @@ static void Close( vlc_object_t *p_this )
vlc_playlist_Stop ( playlist );
vlc_playlist_Unlock( playlist );
- skin_load_intf = NULL;
+ {
+ vlc::threads::mutex_locker guard {skin_load_lock};
+ while (skin_load_rc != 0)
+ skin_load_wait.wait(skin_load_lock);
+
+ /* We don't need the skin_load_lock anymore since the interface
+ * will signaled that it doesn't exist anymore below. */
+ skin_load_intf = NULL;
+ }
AsyncQueue *pQueue = p_intf->p_sys->p_queue;
if( pQueue )
@@ -311,7 +324,15 @@ static void WindowSetFullscreen( vlc_window_t *pWnd, const char * );
static int WindowEnable( vlc_window_t *pWnd, const vlc_window_cfg_t *cfg )
{
vout_window_skins_t* sys = static_cast<vout_window_skins_t*>(pWnd->sys);
- intf_thread_t *pIntf = sys->pIntf;
+
+ /* Protect from races against the main interface closing. */
+ vlc::threads::mutex_locker guard {skin_load_lock};
+
+ /* If the interface has already been closed, we cannot enable the window
+ * anymore and we need to report that. */
+ if (skin_load_intf == NULL)
+ return VLC_EGENERIC;
+ intf_thread_t *pIntf = skin_load_intf;
sys->cfg = *cfg;
@@ -328,24 +349,17 @@ static int WindowEnable( vlc_window_t *pWnd, const vlc_window_cfg_t *cfg )
if (cfg->is_fullscreen)
WindowSetFullscreen( pWnd, NULL );
+
+ /* Prevent the interface from closing before we get disabled. */
+ skin_load_rc++;
return VLC_SUCCESS;
}
static void WindowDisable( vlc_window_t *pWnd )
{
- // vout_window_skins_t* sys = (vout_window_skins_t *)pWnd->sys;
-
- // Design issue
- // In the process of quitting vlc, the interfaces are destroyed first,
- // then comes the playlist along with the player and possible vouts.
- // problem: the interface is no longer active to properly deallocate
- // resources allocated as a vout window submodule.
+ /* Protect from races against the main interface closing. */
+ vlc::threads::mutex_locker guard {skin_load_lock};
intf_thread_t *pIntf = skin_load_intf;
- if( pIntf == NULL )
- {
- msg_Err( pWnd, "Design issue: the interface no longer exists !!!!" );
- return;
- }
// force execution in the skins2 thread context
CmdExecuteBlock* cmd = new CmdExecuteBlock( pIntf, VLC_OBJECT( pWnd ),
@@ -353,6 +367,10 @@ static void WindowDisable( vlc_window_t *pWnd )
CmdExecuteBlock::executeWait( CmdGenericPtr( cmd ) );
pWnd->type = VLC_WINDOW_TYPE_DUMMY;
+
+ /* Now the interface can be closed. */
+ skin_load_rc--;
+ skin_load_wait.signal();
}
static void WindowResize( vlc_window_t *pWnd,
@@ -424,11 +442,13 @@ static int WindowOpen( vlc_window_t *pWnd )
vout_window_skins_t* sys;
- intf_thread_t *pIntf = skin_load_intf;
-
- if( pIntf == NULL )
+ /* Prevent the interface from closing behind our back. */
+ vlc::threads::mutex_locker guard {skin_load_lock};
+ if (skin_load_intf == NULL)
return VLC_EGENERIC;
+ intf_thread_t *pIntf = skin_load_intf;
+
if( !var_InheritBool( pIntf, "skinned-video") )
return VLC_EGENERIC;
View it on GitLab: https://code.videolan.org/videolan/vlc/-/commit/a928eb1712e5968c6eb6527fced380730f7f6e48
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/commit/a928eb1712e5968c6eb6527fced380730f7f6e48
You're receiving this email because of your account on code.videolan.org.
VideoLAN code repository instance
More information about the vlc-commits
mailing list