d3d11videosink: fix race conditions in win32 window

One race condition is the fact that the window object
can be destroyed while running some routine in the UI
thread (such as resizing). To avoid that situation we make
UI thread hold a reference on the window object while it's
running.
Other probpematic case is when the window handle is reused:
if we stop and start the pipeline very fast,
so the sink creates a new window object that is going to use
the same window handle as the previous one.
And finally the case when the pipeline is stopped immediatelly
right after starting, this one is also handled in this commit.

NOTE: a unit test that reproduces this cases have been added
in the previous commit.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4417>

diff --git a/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11videosink.cpp b/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11videosink.cpp
index 2d3ba79..7a9755c 100644 (file)
--- a/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11videosink.cpp
+++ b/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11videosink.cpp
@@ -834,6 +834,7 @@ gst_d3d11_video_sink_update_window (GstD3D11VideoSink * self, GstCaps * caps)
     if (ret == GST_FLOW_FLUSHING) {
       GstD3D11CSLockGuard lk (&self->lock);
       GST_WARNING_OBJECT (self, "Couldn't prepare window but we are flushing");
+      gst_d3d11_window_unprepare (self->window);
       gst_clear_object (&self->window);
       gst_object_unref (window);
 

diff --git a/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11window_win32.cpp b/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11window_win32.cpp
index 73196e0..c4db036 100644 (file)
--- a/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11window_win32.cpp
+++ b/subprojects/gst-plugins-bad/sys/d3d11/gstd3d11window_win32.cpp
@@ -35,6 +35,7 @@ GST_DEBUG_CATEGORY_EXTERN (gst_d3d11_window_debug);
 #define GST_CAT_DEFAULT gst_d3d11_window_debug
 
 G_LOCK_DEFINE_STATIC (create_lock);
+G_LOCK_DEFINE_STATIC (get_instance_lock);
 
 #define EXTERNAL_PROC_PROP_NAME "d3d11_window_external_proc"
 #define D3D11_WINDOW_PROP_NAME "gst_d3d11_window_win32_object"
@@ -121,7 +122,8 @@ static gpointer gst_d3d11_window_win32_thread_func (gpointer data);
 static gboolean
 gst_d3d11_window_win32_create_internal_window (GstD3D11WindowWin32 * self);
 static void gst_d3d11_window_win32_destroy_internal_window (HWND hwnd);
-static void gst_d3d11_window_win32_release_external_handle (HWND hwnd);
+static void
+gst_d3d11_window_win32_release_external_handle (GstD3D11WindowWin32 * self);
 static void
 gst_d3d11_window_win32_on_resize (GstD3D11Window * window,
     guint width, guint height);
@@ -137,7 +139,8 @@ static void gst_d3d11_window_win32_set_title (GstD3D11Window * window,
 static gboolean gst_d3d11_window_win32_unlock (GstD3D11Window * window);
 static gboolean gst_d3d11_window_win32_unlock_stop (GstD3D11Window * window);
 static GstFlowReturn
-gst_d3d11_window_win32_set_external_handle (GstD3D11WindowWin32 * self);
+gst_d3d11_window_win32_set_external_handle (GstD3D11WindowWin32 * self,
+    HWND hwnd);
 
 static void
 gst_d3d11_window_win32_class_init (GstD3D11WindowWin32Class * klass)
@@ -204,8 +207,8 @@ done:
 static void
 gst_d3d11_window_win32_dispose (GObject * object)
 {
+  GST_DEBUG_OBJECT (object, "dispose");
   gst_d3d11_window_win32_unprepare (GST_D3D11_WINDOW (object));
-
   G_OBJECT_CLASS (parent_class)->dispose (object);
 }
 
@@ -230,11 +233,8 @@ gst_d3d11_window_win32_prepare (GstD3D11Window * window, guint display_width,
     return GST_FLOW_ERROR;
   }
 
-  self->overlay_state = GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_NONE;
-  self->external_hwnd = hwnd;
-
   GST_DEBUG_OBJECT (self, "Preparing external handle");
-  ret = gst_d3d11_window_win32_set_external_handle (self);
+  ret = gst_d3d11_window_win32_set_external_handle (self, hwnd);
   if (ret != GST_FLOW_OK) {
     gst_structure_free (config);
     if (ret == GST_FLOW_FLUSHING) {
@@ -261,9 +261,13 @@ gst_d3d11_window_win32_unprepare (GstD3D11Window * window)
 {
   GstD3D11WindowWin32 *self = GST_D3D11_WINDOW_WIN32 (window);
 
+  GST_DEBUG_OBJECT (self, "unprepare");
+
   if (self->external_hwnd) {
-    gst_d3d11_window_win32_release_external_handle (self->external_hwnd);
+    G_LOCK (get_instance_lock);
+    gst_d3d11_window_win32_release_external_handle (self);
     RemovePropA (self->internal_hwnd, D3D11_WINDOW_PROP_NAME);
+    G_UNLOCK (get_instance_lock);
 
     if (self->internal_hwnd_thread == g_thread_self ()) {
       /* State changing thread is identical to internal window thread.
@@ -271,10 +275,10 @@ gst_d3d11_window_win32_unprepare (GstD3D11Window * window)
 
       GST_INFO_OBJECT (self, "Closing internal window immediately");
       gst_d3d11_window_win32_destroy_internal_window (self->internal_hwnd);
-    } else {
+    } else if (self->internal_hwnd) {
       /* We cannot destroy internal window from non-window thread.
        * and we cannot use synchronously SendMessage() method at this point
-       * since window thread might be wait for current thread and SendMessage()
+       * since window thread might be waiting for current thread and SendMessage()
        * will be blocked until it's called from window thread.
        * Instead, posts message so that it can be closed from window thread
        * asynchronously */
@@ -308,6 +312,19 @@ gst_d3d11_window_win32_unprepare (GstD3D11Window * window)
   }
 }
 
+static GstD3D11WindowWin32 *
+gst_d3d11_window_win32_hwnd_get_instance (HWND hwnd)
+{
+  HANDLE handle;
+  G_LOCK (get_instance_lock);
+  handle = GetPropA (hwnd, D3D11_WINDOW_PROP_NAME);
+  if (handle)
+    handle = gst_object_ref (handle);
+  G_UNLOCK (get_instance_lock);
+
+  return (GstD3D11WindowWin32 *) handle;
+}
+
 static void
 gst_d3d11_window_win32_set_render_rectangle (GstD3D11Window * window,
     const GstVideoRectangle * rect)
@@ -465,41 +482,47 @@ gst_d3d11_window_win32_destroy_internal_window (HWND hwnd)
   if (!hwnd)
     return;
 
+  ShowWindow (hwnd, SW_HIDE);
   SetParent (hwnd, NULL);
 
   GST_INFO ("Destroying internal window %" G_GUINTPTR_FORMAT, (guintptr) hwnd);
 
   if (!DestroyWindow (hwnd))
-    GST_WARNING ("failed to destroy window %" G_GUINTPTR_FORMAT
+    g_critical ("failed to destroy window %" G_GUINTPTR_FORMAT
         ", 0x%x", (guintptr) hwnd, (guint) GetLastError ());
 }
 
 static GstFlowReturn
-gst_d3d11_window_win32_set_external_handle (GstD3D11WindowWin32 * self)
+gst_d3d11_window_win32_set_external_handle (GstD3D11WindowWin32 * self,
+    HWND hwnd)
 {
   WNDPROC external_window_proc;
   GstFlowReturn ret = GST_FLOW_OK;
 
-  external_window_proc =
-      (WNDPROC) GetWindowLongPtrA (self->external_hwnd, GWLP_WNDPROC);
+  GstD3D11SRWLockGuard lk (&self->lock);
+  self->overlay_state = GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_NONE;
+  self->external_hwnd = hwnd;
+
+  G_LOCK (get_instance_lock);
+  external_window_proc = (WNDPROC) GetWindowLongPtrA (hwnd, GWLP_WNDPROC);
 
   GST_DEBUG_OBJECT (self, "set external window %" G_GUINTPTR_FORMAT
-      ", original window procedure %p", (guintptr) self->external_hwnd,
-      external_window_proc);
+      ", original window procedure %p", (guintptr) hwnd, external_window_proc);
 
-  SetPropA (self->external_hwnd, EXTERNAL_PROC_PROP_NAME,
-      (HANDLE) external_window_proc);
-  SetPropA (self->external_hwnd, D3D11_WINDOW_PROP_NAME, self);
-  SetWindowLongPtrA (self->external_hwnd, GWLP_WNDPROC,
-      (LONG_PTR) sub_class_proc);
+  g_assert (external_window_proc != sub_class_proc);
+  g_warn_if_fail (GetPropA (hwnd, EXTERNAL_PROC_PROP_NAME) == NULL);
+  g_warn_if_fail (GetPropA (hwnd, D3D11_WINDOW_PROP_NAME) == NULL);
+
+  SetPropA (hwnd, EXTERNAL_PROC_PROP_NAME, (HANDLE) external_window_proc);
+  SetPropA (hwnd, D3D11_WINDOW_PROP_NAME, self);
+
+  SetWindowLongPtrA (hwnd, GWLP_WNDPROC, (LONG_PTR) sub_class_proc);
+  G_UNLOCK (get_instance_lock);
 
   /* SendMessage() may cause deadlock if parent window thread is busy
    * for changing pipeline's state. Post our message instead, and wait for
    * the parent window's thread or flushing */
-  PostMessageA (self->external_hwnd, WM_GST_D3D11_CONSTRUCT_INTERNAL_WINDOW,
-      0, 0);
-
-  GstD3D11SRWLockGuard lk (&self->lock);
+  PostMessageA (hwnd, WM_GST_D3D11_CONSTRUCT_INTERNAL_WINDOW, 0, 0);
   while (self->external_hwnd &&
       self->overlay_state == GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_NONE &&
       !self->flushing) {
@@ -517,27 +540,29 @@ gst_d3d11_window_win32_set_external_handle (GstD3D11WindowWin32 * self)
 }
 
 static void
-gst_d3d11_window_win32_release_external_handle (HWND hwnd)
+gst_d3d11_window_win32_release_external_handle (GstD3D11WindowWin32 * self)
 {
   WNDPROC external_proc;
+  HWND hwnd = self->external_hwnd;
 
   if (!hwnd)
     return;
 
+  self->external_hwnd = NULL;
   external_proc = (WNDPROC) GetPropA (hwnd, EXTERNAL_PROC_PROP_NAME);
   if (!external_proc) {
-    GST_WARNING ("Failed to get original window procedure");
+    GST_WARNING_OBJECT (self, "Failed to get original window procedure");
     return;
   }
 
-  GST_DEBUG ("release external window %" G_GUINTPTR_FORMAT
+  GST_DEBUG_OBJECT (self, "release external window %" G_GUINTPTR_FORMAT
       ", original window procedure %p", (guintptr) hwnd, external_proc);
 
   RemovePropA (hwnd, EXTERNAL_PROC_PROP_NAME);
   RemovePropA (hwnd, D3D11_WINDOW_PROP_NAME);
 
   if (!SetWindowLongPtrA (hwnd, GWLP_WNDPROC, (LONG_PTR) external_proc))
-    GST_WARNING ("Couldn't restore original window procedure");
+    GST_WARNING_OBJECT (self, "Couldn't restore original window procedure");
 }
 
 static gboolean
@@ -763,8 +788,8 @@ gst_d3d11_window_win32_handle_window_proc (GstD3D11WindowWin32 * self,
     case WM_CLOSE:
       if (self->internal_hwnd) {
         RemovePropA (self->internal_hwnd, D3D11_WINDOW_PROP_NAME);
-        ShowWindow (self->internal_hwnd, SW_HIDE);
-        gst_d3d11_window_win32_destroy_internal_window (self->internal_hwnd);
+        gst_d3d11_window_win32_destroy_internal_window (hWnd);
+        self->overlay_state = GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_CLOSED;
         self->internal_hwnd = NULL;
         self->internal_hwnd_thread = NULL;
       }
@@ -840,6 +865,12 @@ window_proc (HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
 {
   GstD3D11WindowWin32 *self;
 
+  if (uMsg == WM_GST_D3D11_DESTROY_INTERNAL_WINDOW) {
+    GST_INFO ("Handle destroy window message");
+    gst_d3d11_window_win32_destroy_internal_window (hWnd);
+    return 0;
+  }
+
   if (uMsg == WM_CREATE) {
     self = GST_D3D11_WINDOW_WIN32 (((LPCREATESTRUCT) lParam)->lpCreateParams);
 
@@ -852,16 +883,7 @@ window_proc (HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
     ReleaseDC (hWnd, self->device_handle);
 
     SetPropA (hWnd, D3D11_WINDOW_PROP_NAME, self);
-  } else if (GetPropA (hWnd, D3D11_WINDOW_PROP_NAME)) {
-    HANDLE handle = GetPropA (hWnd, D3D11_WINDOW_PROP_NAME);
-
-    if (!GST_IS_D3D11_WINDOW_WIN32 (handle)) {
-      GST_WARNING ("%p is not d3d11window object", handle);
-      return DefWindowProcA (hWnd, uMsg, wParam, lParam);
-    }
-
-    self = GST_D3D11_WINDOW_WIN32 (handle);
-
+  } else if ((self = gst_d3d11_window_win32_hwnd_get_instance (hWnd))) {
     g_assert (self->internal_hwnd == hWnd);
 
     gst_d3d11_window_win32_handle_window_proc (self, hWnd, uMsg, wParam,
@@ -870,23 +892,22 @@ window_proc (HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
     switch (uMsg) {
       case WM_SIZE:
         /* We handled this event already */
+        gst_object_unref (self);
         return 0;
       case WM_NCHITTEST:
         /* To passthrough mouse event if external window is used.
          * Only hit-test succeeded window can receive/handle some mouse events
          * and we want such events to be handled by parent (application) window
          */
-        if (self->external_hwnd)
+        if (self->external_hwnd) {
+          gst_object_unref (self);
           return (LRESULT) HTTRANSPARENT;
+        }
         break;
       default:
         break;
     }
-  } else if (uMsg == WM_GST_D3D11_DESTROY_INTERNAL_WINDOW) {
-    GST_INFO ("Handle destroy window message");
-    gst_d3d11_window_win32_destroy_internal_window (hWnd);
-
-    return 0;
+    gst_object_unref (self);
   }
 
   return DefWindowProcA (hWnd, uMsg, wParam, lParam);
@@ -897,49 +918,73 @@ sub_class_proc (HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
 {
   WNDPROC external_window_proc =
       (WNDPROC) GetPropA (hWnd, EXTERNAL_PROC_PROP_NAME);
-  GstD3D11WindowWin32 *self =
-      (GstD3D11WindowWin32 *) GetPropA (hWnd, D3D11_WINDOW_PROP_NAME);
+  GstD3D11WindowWin32 *self = gst_d3d11_window_win32_hwnd_get_instance (hWnd);
 
-  if (uMsg == WM_GST_D3D11_CONSTRUCT_INTERNAL_WINDOW) {
-    GstD3D11Window *window = GST_D3D11_WINDOW (self);
-    RECT rect;
+  if (self == NULL || self->flushing) {
+    GST_DEBUG ("No object attached to the window, chain up to default");
+    gst_clear_object (&self);
+    return CallWindowProcA (external_window_proc, hWnd, uMsg, wParam, lParam);
+  }
 
-    GST_DEBUG_OBJECT (self, "Create internal window");
+  switch (uMsg) {
+    case WM_GST_D3D11_CONSTRUCT_INTERNAL_WINDOW:{
+      GstD3D11Window *window = GST_D3D11_WINDOW (self);
+      RECT rect;
 
-    window->initialized = gst_d3d11_window_win32_create_internal_window (self);
+      GST_DEBUG_OBJECT (self, "Create internal window");
 
-    SetWindowLongPtrA (self->internal_hwnd, GWL_STYLE, WS_CHILD | WS_MAXIMIZE);
-    SetParent (self->internal_hwnd, self->external_hwnd);
+      GstD3D11SRWLockGuard lk (&self->lock);
+      if (self->internal_hwnd) {
+        GST_WARNING_OBJECT (self,
+            "Window already created, probably we have received 2 creation messages");
+        g_warn_if_fail (self->overlay_state ==
+            GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_OPENED);
+        gst_object_unref (self);
+        return 0;
+      }
 
-    /* take changes into account: SWP_FRAMECHANGED */
-    GetClientRect (self->external_hwnd, &rect);
-    SetWindowPos (self->internal_hwnd, HWND_TOP, rect.left, rect.top,
-        rect.right, rect.bottom,
-        SWP_ASYNCWINDOWPOS | SWP_NOMOVE | SWP_NOSIZE | SWP_NOZORDER |
-        SWP_FRAMECHANGED | SWP_NOACTIVATE);
-    MoveWindow (self->internal_hwnd, rect.left, rect.top, rect.right,
-        rect.bottom, FALSE);
+      if (self->flushing) {
+        GST_DEBUG_OBJECT (self, "Flushing");
+        gst_object_unref (self);
+        return 0;
+      }
 
-    GstD3D11SRWLockGuard lk (&self->lock);
-    self->overlay_state = GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_OPENED;
-    WakeAllConditionVariable (&self->cond);
+      window->initialized =
+          gst_d3d11_window_win32_create_internal_window (self);
 
-    /* don't need to be chained up to parent window procedure,
-     * as this is our custom message */
-    return 0;
-  } else if (self) {
-    if (uMsg == WM_SIZE) {
+      SetWindowLongPtrA (self->internal_hwnd, GWL_STYLE,
+          WS_CHILD | WS_MAXIMIZE);
+      SetParent (self->internal_hwnd, self->external_hwnd);
+
+      /* take changes into account: SWP_FRAMECHANGED */
+      GetClientRect (self->external_hwnd, &rect);
+      SetWindowPos (self->internal_hwnd, HWND_TOP, rect.left, rect.top,
+          rect.right, rect.bottom,
+          SWP_ASYNCWINDOWPOS | SWP_NOMOVE | SWP_NOSIZE | SWP_NOZORDER |
+          SWP_FRAMECHANGED | SWP_NOACTIVATE);
+      MoveWindow (self->internal_hwnd, rect.left, rect.top, rect.right,
+          rect.bottom, FALSE);
+
+      self->overlay_state = GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_OPENED;
+      WakeAllConditionVariable (&self->cond);
+
+      /* don't need to be chained up to parent window procedure,
+       * as this is our custom message */
+      gst_object_unref (self);
+      return 0;
+    }
+    case WM_SIZE:
       MoveWindow (self->internal_hwnd, 0, 0, LOWORD (lParam), HIWORD (lParam),
           FALSE);
-    } else if (uMsg == WM_CLOSE || uMsg == WM_DESTROY) {
+      break;
+    case WM_CLOSE:
+    case WM_DESTROY:{
       GstD3D11SRWLockGuard lk (&self->lock);
       GST_WARNING_OBJECT (self, "external window is closing");
-      gst_d3d11_window_win32_release_external_handle (self->external_hwnd);
-      self->external_hwnd = NULL;
+      gst_d3d11_window_win32_release_external_handle (self);
 
       if (self->internal_hwnd) {
         RemovePropA (self->internal_hwnd, D3D11_WINDOW_PROP_NAME);
-        ShowWindow (self->internal_hwnd, SW_HIDE);
         gst_d3d11_window_win32_destroy_internal_window (self->internal_hwnd);
       }
       self->internal_hwnd = NULL;
@@ -947,12 +992,15 @@ sub_class_proc (HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
 
       self->overlay_state = GST_D3D11_WINDOW_WIN32_OVERLAY_STATE_CLOSED;
       WakeAllConditionVariable (&self->cond);
-    } else {
+      break;
+    }
+    default:
       gst_d3d11_window_win32_handle_window_proc (self, hWnd, uMsg, wParam,
           lParam);
-    }
+      break;
   }
 
+  gst_object_unref (self);
   return CallWindowProcA (external_window_proc, hWnd, uMsg, wParam, lParam);
 }