vulkan/image: don't rely on weak-ref notifies for views

Weak refs don't quite work here correctly as there is always a race with
taking the lock between find_view() and remove_view().  If find_view()
returns a view that is going to removed by remove_view() then we have an
interesting situation.

In theory, the number and type of views for an image are relatively
constant and should not change one they've been set up which means that
it is actually practical to perform pool-like reference counting here
where the image holds a pool of different views that it can give out
as necessary.

3 files changed, 102 insertions, 25 deletions

diff --git a/gst-libs/gst/vulkan/gstvkimagememory.c b/gst-libs/gst/vulkan/gstvkimagememory.c
index 490e68606..653a59f26 100644
--- a/gst-libs/gst/vulkan/gstvkimagememory.c
+++ b/gst-libs/gst/vulkan/gstvkimagememory.c
@@ -196,6 +196,7 @@ _vk_image_mem_init (GstVulkanImageMemory * mem, GstAllocator * allocator,
   g_mutex_init (&mem->lock);
 
   mem->views = g_ptr_array_new ();
+  mem->outstanding_views = g_ptr_array_new ();
 
   GST_CAT_DEBUG (GST_CAT_VULKAN_IMAGE_MEMORY,
       "new Vulkan Image memory:%p size:%" G_GSIZE_FORMAT, mem, maxsize);
@@ -398,6 +399,12 @@ _vk_image_mem_alloc (GstAllocator * allocator, gsize size,
 }
 
 static void
+_free_view (GstVulkanImageView * view, gpointer unused)
+{
+  gst_vulkan_image_view_unref (view);
+}
+
+static void
 _vk_image_mem_free (GstAllocator * allocator, GstMemory * memory)
 {
   GstVulkanImageMemory *mem = (GstVulkanImageMemory *) memory;
@@ -405,11 +412,11 @@ _vk_image_mem_free (GstAllocator * allocator, GstMemory * memory)
   GST_CAT_TRACE (GST_CAT_VULKAN_IMAGE_MEMORY, "freeing image memory:%p "
       "id:%" G_GUINT64_FORMAT, mem, (guint64) mem->image);
 
-  if (mem->views->len > 0) {
-    g_warning ("GstVulkanImageMemory <%p> is being freed with outstanding "
-        "GstVulkanImageView's.  This usually means there is a reference "
-        "counting issue.", mem);
-  }
+  g_warn_if_fail (mem->outstanding_views > 0);
+  g_ptr_array_unref (mem->outstanding_views);
+
+  g_ptr_array_foreach (mem->views, (GFunc) _free_view, NULL);
+  g_ptr_array_unref (mem->views);
 
   if (mem->image && !mem->wrapped)
     vkDestroyImage (mem->device->device, mem->image, NULL);
@@ -529,8 +536,12 @@ find_view_index_unlocked (GstVulkanImageMemory * image,
   return (gint) index;
 }
 
-static void
-gst_vulkan_image_memory_remove_view (GstVulkanImageMemory * image,
+extern void
+gst_vulkan_image_memory_release_view (GstVulkanImageMemory * image,
+    GstVulkanImageView * view);
+
+void
+gst_vulkan_image_memory_release_view (GstVulkanImageMemory * image,
     GstVulkanImageView * view)
 {
   guint index;
@@ -538,21 +549,20 @@ gst_vulkan_image_memory_remove_view (GstVulkanImageMemory * image,
   g_return_if_fail (gst_is_vulkan_image_memory (GST_MEMORY_CAST (image)));
 
   g_mutex_lock (&image->lock);
-  if (!g_ptr_array_find (image->views, view, &index)) {
-    g_warning ("GstVulkanImageMemory:%p Attempting to remove a view %p"
+  GST_CAT_TRACE (GST_CAT_VULKAN_IMAGE_MEMORY, "image %p removing view %p",
+      image, view);
+  if (g_ptr_array_find (image->outstanding_views, view, &index)) {
+    /* really g_ptr_array_steal_index_fast() but that requires glib 2.58 */
+    g_ptr_array_remove_index_fast (image->outstanding_views, index);
+    g_ptr_array_add (image->views, view);
+  } else {
+    g_warning ("GstVulkanImageMemory:%p attempt to remove a view %p "
         "that we do not own", image, view);
-    g_assert_not_reached ();
   }
-  g_ptr_array_remove_index_fast (image->views, index);
+  gst_clear_mini_object ((GstMiniObject **) & view->image);
   g_mutex_unlock (&image->lock);
 }
 
-static void
-view_free_notify (GstVulkanImageMemory * image, GstVulkanImageView * view)
-{
-  gst_vulkan_image_memory_remove_view (image, view);
-}
-
 /**
  * gst_vulkan_image_memory_add_view:
  * @image: a #GstVulkanImageMemory
@@ -576,13 +586,37 @@ gst_vulkan_image_memory_add_view (GstVulkanImageMemory * image,
     g_mutex_unlock (&image->lock);
     return;
   }
-  g_ptr_array_add (image->views, view);
-  gst_mini_object_weak_ref (GST_MINI_OBJECT_CAST (view),
-      (GstMiniObjectNotify) view_free_notify, image);
+  g_ptr_array_add (image->outstanding_views, view);
+  GST_CAT_TRACE (GST_CAT_VULKAN_IMAGE_MEMORY, "Image %p adding view %p",
+      image, view);
 
   g_mutex_unlock (&image->lock);
 }
 
+struct view_data
+{
+  GstVulkanImageMemory *img;
+  GstVulkanImageMemoryFindViewFunc find_func;
+  gpointer find_data;
+};
+
+static gboolean
+find_view_func (GstVulkanImageView * view, gpointer user_data)
+{
+  struct view_data *data = user_data;
+  GstVulkanImageMemory *previous;
+  gboolean ret;
+
+  previous = view->image;
+  view->image = data->img;
+
+  ret = data->find_func (view, data->find_data);
+
+  view->image = previous;
+
+  return ret;
+}
+
 /**
  * gst_vulkan_image_memory_find_view:
  * @image: a #GstVulkanImageMemory
@@ -599,6 +633,7 @@ gst_vulkan_image_memory_find_view (GstVulkanImageMemory * image,
     GstVulkanImageMemoryFindViewFunc find_func, gpointer data)
 {
   GstVulkanImageView *ret = NULL;
+  struct view_data view;
   guint index;
 
   g_return_val_if_fail (gst_is_vulkan_image_memory (GST_MEMORY_CAST (image)),
@@ -606,9 +641,25 @@ gst_vulkan_image_memory_find_view (GstVulkanImageMemory * image,
   g_return_val_if_fail (find_func != NULL, NULL);
 
   g_mutex_lock (&image->lock);
-  if (g_ptr_array_find_with_equal_func (image->views, data,
-          (GEqualFunc) find_func, &index))
-    ret = gst_vulkan_image_view_ref (g_ptr_array_index (image->views, index));
+  view.img = image;
+  view.find_func = find_func;
+  view.find_data = data;
+
+  if (g_ptr_array_find_with_equal_func (image->outstanding_views, &view,
+          (GEqualFunc) find_view_func, &index)) {
+    ret =
+        gst_vulkan_image_view_ref (g_ptr_array_index (image->outstanding_views,
+            index));
+  } else if (g_ptr_array_find_with_equal_func (image->views, &view,
+          (GEqualFunc) find_view_func, &index)) {
+    /* really g_ptr_array_steal_index_fast() but that requires glib 2.58 */
+    ret = g_ptr_array_remove_index_fast (image->views, index);
+    g_ptr_array_add (image->outstanding_views, ret);
+    ret->image = (GstVulkanImageMemory *) gst_memory_ref ((GstMemory *) image);
+  }
+
+  GST_CAT_TRACE (GST_CAT_VULKAN_IMAGE_MEMORY, "Image %p found view %p",
+      image, ret);
   g_mutex_unlock (&image->lock);
 
   return ret;
diff --git a/gst-libs/gst/vulkan/gstvkimagememory.h b/gst-libs/gst/vulkan/gstvkimagememory.h
index 26461f3e6..20acffebd 100644
--- a/gst-libs/gst/vulkan/gstvkimagememory.h
+++ b/gst-libs/gst/vulkan/gstvkimagememory.h
@@ -73,6 +73,9 @@ struct _GstVulkanImageMemory
   gpointer user_data;
 
   GPtrArray *views;
+  GPtrArray *outstanding_views;
+
+  gpointer _padding[GST_PADDING];
 };
 
 /**
diff --git a/gst-libs/gst/vulkan/gstvkimageview.c b/gst-libs/gst/vulkan/gstvkimageview.c
index 1c23ce1ce..751dfa56d 100644
--- a/gst-libs/gst/vulkan/gstvkimageview.c
+++ b/gst-libs/gst/vulkan/gstvkimageview.c
@@ -49,6 +49,24 @@ init_debug (void)
   }
 }
 
+extern void
+gst_vulkan_image_memory_release_view (GstVulkanImageMemory * image,
+    GstVulkanImageView * view);
+
+static gboolean
+gst_vulkan_image_view_dispose (GstVulkanImageView * view)
+{
+  GstVulkanImageMemory *image;
+
+  if ((image = view->image) == NULL)
+    return TRUE;
+
+  gst_vulkan_image_view_ref (view);
+  gst_vulkan_image_memory_release_view (image, view);
+
+  return FALSE;
+}
+
 static void
 gst_vulkan_image_view_free (GstVulkanImageView * view)
 {
@@ -57,8 +75,9 @@ gst_vulkan_image_view_free (GstVulkanImageView * view)
   if (view->view)
     vkDestroyImageView (view->device->device, view->view, NULL);
 
-  if (view->image)
+  if (view->image) {
     gst_memory_unref (GST_MEMORY_CAST (view->image));
+  }
   view->image = NULL;
   gst_clear_object (&view->device);
 
@@ -90,7 +109,8 @@ gst_vulkan_image_view_new (GstVulkanImageMemory * image,
   view = g_new0 (GstVulkanImageView, 1);
 
   gst_mini_object_init ((GstMiniObject *) view, 0,
-      gst_vulkan_image_view_get_type (), NULL, NULL,
+      gst_vulkan_image_view_get_type (), NULL,
+      (GstMiniObjectDisposeFunction) gst_vulkan_image_view_dispose,
       (GstMiniObjectFreeFunction) gst_vulkan_image_view_free);
 
   err =
@@ -105,6 +125,9 @@ gst_vulkan_image_view_new (GstVulkanImageMemory * image,
   /* we cannot keep this as it may point to stack allocated memory */
   view->create_info.pNext = NULL;
 
+  GST_CAT_TRACE (GST_CAT_VULKAN_IMAGE_VIEW, "new image view for image: %p",
+      image);
+
   return view;
 
 vk_error: