[vlc-devel] [PATCH 05/16] qt: don't release vout window when the interface has already been destroyed
Rémi Denis-Courmont
remi at remlab.net
Thu Aug 6 20:30:12 CEST 2020
Le torstaina 6. elokuuta 2020, 10.43.51 EEST Pierre Lamot a écrit :
> ---
> modules/gui/qt/maininterface/compositor.hpp | 3 +-
> .../gui/qt/maininterface/compositor_dcomp.cpp | 41 ++++++++++++++-----
> .../gui/qt/maininterface/compositor_dcomp.hpp | 2 +-
> .../gui/qt/maininterface/compositor_dummy.cpp | 2 +-
> .../gui/qt/maininterface/compositor_dummy.hpp | 2 +-
> modules/gui/qt/qt.cpp | 25 ++++++++++-
> 6 files changed, 60 insertions(+), 15 deletions(-)
>
> diff --git a/modules/gui/qt/maininterface/compositor.hpp
> b/modules/gui/qt/maininterface/compositor.hpp index aef4e11a32..7e74ffb593
> 100644
> --- a/modules/gui/qt/maininterface/compositor.hpp
> +++ b/modules/gui/qt/maininterface/compositor.hpp
> @@ -34,13 +34,14 @@ namespace vlc {
>
> class Compositor {
> public:
> + typedef bool (*VoutDestroyCb)(vout_window_t *p_wnd);
>
> virtual ~Compositor() = default;
>
> virtual MainInterface* makeMainInterface() = 0;
> virtual void destroyMainInterface() = 0;
>
> - virtual bool setupVoutWindow(vout_window_t *p_wnd) = 0;
> + virtual bool setupVoutWindow(vout_window_t *p_wnd, VoutDestroyCb
> destroyCb) = 0;
>
> //factory
> static Compositor* createCompositor(intf_thread_t *p_intf);
> diff --git a/modules/gui/qt/maininterface/compositor_dcomp.cpp
> b/modules/gui/qt/maininterface/compositor_dcomp.cpp index
> 23b0612a9e..b3b95aa8f1 100644
> --- a/modules/gui/qt/maininterface/compositor_dcomp.cpp
> +++ b/modules/gui/qt/maininterface/compositor_dcomp.cpp
> @@ -40,6 +40,11 @@
>
> namespace vlc {
>
> +struct VoutWindowPriv {
> + CompositorDirectComposition* that = nullptr;
> + CompositorDirectComposition::VoutDestroyCb voutDestroyCb = nullptr;
> +};
> +
> using namespace Microsoft::WRL;
>
> //Signature for DCompositionCreateDevice
> @@ -47,7 +52,8 @@ typedef HRESULT (*DCompositionCreateDeviceFun)(IDXGIDevice
> *dxgiDevice, REFIID i
>
> int CompositorDirectComposition::window_enable(struct vout_window_t *
> p_wnd, const vout_window_cfg_t *) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> msg_Dbg(that->m_intf, "window_enable");
> if (!that->m_videoVisual)
> {
> @@ -71,7 +77,8 @@ int CompositorDirectComposition::window_enable(struct
> vout_window_t * p_wnd, con
>
> void CompositorDirectComposition::window_disable(struct vout_window_t *
> p_wnd) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> try
> {
> that->m_qmlVideoSurfaceProvider->disable();
> @@ -89,14 +96,20 @@ void CompositorDirectComposition::window_disable(struct
> vout_window_t * p_wnd)
>
> void CompositorDirectComposition::window_resize(struct vout_window_t *
> p_wnd, unsigned width, unsigned height) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> msg_Dbg(that->m_intf, "window_resize %ux%u", width, height);
> that->m_rootWindow->requestResizeVideo(width, height);
> }
>
> void CompositorDirectComposition::window_destroy(struct vout_window_t *
> p_wnd) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> + bool canRelease = sys->voutDestroyCb(p_wnd);
> + delete sys;
> + if (!canRelease)
> + return;
> msg_Dbg(that->m_intf, "window_destroy");
> that->m_window = nullptr;
> that->m_videoVisual.Reset();
> @@ -104,21 +117,24 @@ void
> CompositorDirectComposition::window_destroy(struct vout_window_t * p_wnd)
>
> void CompositorDirectComposition::window_set_state(struct vout_window_t *
> p_wnd, unsigned state) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> msg_Dbg(that->m_intf, "window_set_state");
>
> that->m_rootWindow->requestVideoState(static_cast<vout_window_state>(state)
> ); }
>
> void CompositorDirectComposition::window_unset_fullscreen(struct
> vout_window_t * p_wnd) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> msg_Dbg(that->m_intf, "window_unset_fullscreen");
> that->m_rootWindow->requestVideoWindowed();
> }
>
> void CompositorDirectComposition::window_set_fullscreen(struct
> vout_window_t * p_wnd, const char *id) {
> - CompositorDirectComposition* that =
> static_cast<CompositorDirectComposition*>(p_wnd->sys); + VoutWindowPriv*
> sys = static_cast<VoutWindowPriv*>(p_wnd->sys); +
> CompositorDirectComposition* that = sys->that;
> msg_Dbg(that->m_intf, "window_set_fullscreen");
> that->m_rootWindow->requestVideoFullScreen(id);
> }
> @@ -263,7 +279,7 @@ void CompositorDirectComposition::destroyMainInterface()
> }
> }
>
> -bool CompositorDirectComposition::setupVoutWindow(vout_window_t *p_wnd)
> +bool CompositorDirectComposition::setupVoutWindow(vout_window_t *p_wnd,
> VoutDestroyCb destroyCb) {
> //Only the first video is embedded
> if (m_videoVisual.Get())
> @@ -275,7 +291,6 @@ bool
> CompositorDirectComposition::setupVoutWindow(vout_window_t *p_wnd)
> msg_Err(p_wnd, "create to create DComp video visual");
> return false;
> }
> -
> static const struct vout_window_operations ops = {
> CompositorDirectComposition::window_enable,
> CompositorDirectComposition::window_disable,
> @@ -286,7 +301,13 @@ bool
> CompositorDirectComposition::setupVoutWindow(vout_window_t *p_wnd)
> CompositorDirectComposition::window_set_fullscreen,
> nullptr, //window_set_title
> };
> - p_wnd->sys = this;
> +
> + VoutWindowPriv* sys = new VoutWindowPriv();
> + //destructor callback
> + sys->voutDestroyCb = destroyCb;
> + sys->that = this;
> +
> + p_wnd->sys = sys;
> p_wnd->type = VOUT_WINDOW_TYPE_DCOMP;
> p_wnd->display.dcomp_device = m_dcompDevice.Get();
> p_wnd->handle.dcomp_visual = m_videoVisual.Get();
> diff --git a/modules/gui/qt/maininterface/compositor_dcomp.hpp
> b/modules/gui/qt/maininterface/compositor_dcomp.hpp index
> 7deaf92127..41d5678bea 100644
> --- a/modules/gui/qt/maininterface/compositor_dcomp.hpp
> +++ b/modules/gui/qt/maininterface/compositor_dcomp.hpp
> @@ -48,7 +48,7 @@ public:
> MainInterface *makeMainInterface() override;
> void destroyMainInterface() override;
>
> - bool setupVoutWindow(vout_window_t *p_wnd) override;
> + bool setupVoutWindow(vout_window_t *p_wnd, VoutDestroyCb destroyCb)
> override;
>
> private:
> static int window_enable(struct vout_window_t *, const
> vout_window_cfg_t *); diff --git
> a/modules/gui/qt/maininterface/compositor_dummy.cpp
> b/modules/gui/qt/maininterface/compositor_dummy.cpp index
> 16012e1c34..c645552bf7 100644
> --- a/modules/gui/qt/maininterface/compositor_dummy.cpp
> +++ b/modules/gui/qt/maininterface/compositor_dummy.cpp
> @@ -52,7 +52,7 @@ void CompositorDummy::destroyMainInterface()
> }
> }
>
> -bool CompositorDummy::setupVoutWindow(vout_window_t*)
> +bool CompositorDummy::setupVoutWindow(vout_window_t*, VoutDestroyCb )
> {
> //dummy compositor doesn't handle window intergration
> return false;
> diff --git a/modules/gui/qt/maininterface/compositor_dummy.hpp
> b/modules/gui/qt/maininterface/compositor_dummy.hpp index
> bcaa9672a0..521700418d 100644
> --- a/modules/gui/qt/maininterface/compositor_dummy.hpp
> +++ b/modules/gui/qt/maininterface/compositor_dummy.hpp
> @@ -39,7 +39,7 @@ public:
> MainInterface *makeMainInterface() override;
> virtual void destroyMainInterface() override;
>
> - bool setupVoutWindow(vout_window_t *p_wnd) override;
> + bool setupVoutWindow(vout_window_t *p_wnd, VoutDestroyCb destroyCb)
> override;
>
> private:
>
> diff --git a/modules/gui/qt/qt.cpp b/modules/gui/qt/qt.cpp
> index 804a8a9408..f0d361c9ee 100644
> --- a/modules/gui/qt/qt.cpp
> +++ b/modules/gui/qt/qt.cpp
> @@ -796,6 +796,29 @@ static void ShowDialog( intf_thread_t *p_intf, int
> i_dialog_event, int i_arg, QApplication::postEvent( THEDP, event );
> }
>
> +static bool WindowClose( vout_window_t *p_wnd )
> +{
> + MainInterface *p_mi = (MainInterface *)p_wnd->sys;
> + vlc::threads::mutex_locker locker (lock);
> +
> + /* Normally, the interface terminates after the video. In the contrary,
> the
> + * Qt main loop is gone, so we cannot send any event to the user
> interface
> + * widgets. Ideally, we would keep the Qt main loop running
> until after
> + * the video window is released. But it is far simpler to
> just have the Qt
> + * thread destroy the window early, and to turn this
> function into a stub.
> + *
> + * That assumes the video output will behave sanely if it window is
> + * destroyed asynchronously.
> + * XCB and Xlib-XCB are fine with that. Plain Xlib wouldn't, */
It would certainly be far simpler, if it worked, but as the second paragraph
states, it does *not* work. You _cannot_ do that. The window handle MUST be
valid until the vout_twindow_t.disable callback is invoked. You MUST deal with
that.
Otherwise, nothing except XCB works: GL crashes or locks up, Xlib aborts, and
Vulkan probably crashes or locks up like GL. I can only guess that DX and
WinGL don't handle it well either. Nowadays, VLC rarely ever uses XCB, meaning
XCB/RENDER or plain XCB outputs.
-1
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list