gstbuffer: Add parent meta when a copy shares memory with parent

When copying a buffer, for example with gst_buffer_make_writable(), the
new buffer might reference the same GstMemory as the src buffer,
making those memories not writable. If the src buffer gets disposed
first it should return to its buffer pool, but since some of its
memories are not writable it gets discarded and new buffer/memory gets
allocated.

Solves this by making the new buffer keep a reference to the src buffer,
that ensures that by the time the src buffer gets disposed no other
buffer are referencing its memories and it can thus return safely to its
pool.

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

diff --git a/subprojects/gst-devtools/validate/gst/validate/flow/formatting.c b/subprojects/gst-devtools/validate/gst/validate/flow/formatting.c
index 7736118..c9f8c78 100644 (file)
--- a/subprojects/gst-devtools/validate/gst/validate/flow/formatting.c
+++ b/subprojects/gst-devtools/validate/gst/validate/flow/formatting.c
@@ -252,6 +252,12 @@ buffer_get_meta_string (GstBuffer * buffer)
   while ((meta = gst_buffer_iterate_meta (buffer, &state))) {
     const gchar *desc = g_type_name (meta->info->type);
 
+    if (meta->info->api == GST_PARENT_BUFFER_META_API_TYPE) {
+      /* The parent buffer meta is added automatically every time a buffer gets
+       * copied, it is not useful to track them. */
+      continue;
+    }
+
     if (s == NULL)
       s = g_string_new (NULL);
     else

diff --git a/subprojects/gstreamer/gst/gstbuffer.c b/subprojects/gstreamer/gst/gstbuffer.c
index 441fc82..a2fff0c 100644 (file)
--- a/subprojects/gstreamer/gst/gstbuffer.c
+++ b/subprojects/gstreamer/gst/gstbuffer.c
@@ -546,6 +546,7 @@ gst_buffer_copy_into (GstBuffer * dest, GstBuffer * src,
   GstMetaItem *walk;
   gsize bufsize;
   gboolean region = FALSE;
+  gboolean sharing_mem = FALSE;
 
   g_return_val_if_fail (dest != NULL, FALSE);
   g_return_val_if_fail (src != NULL, FALSE);
@@ -649,6 +650,9 @@ gst_buffer_copy_into (GstBuffer * dest, GstBuffer * src,
           return FALSE;
         }
 
+        /* Indicates if dest references any of src memories. */
+        sharing_mem |= (newmem == mem);
+
         _memory_add (dest, -1, newmem);
         left -= tocopy;
       }
@@ -662,6 +666,10 @@ gst_buffer_copy_into (GstBuffer * dest, GstBuffer * src,
         gst_buffer_remove_memory_range (dest, dest_len, -1);
         return FALSE;
       }
+
+      /* If we were sharing memory and the merge is no-op, we are still sharing. */
+      sharing_mem &= (mem == GST_BUFFER_MEM_PTR (dest, 0));
+
       _replace_memory (dest, len, 0, len, mem);
     }
   }
@@ -710,6 +718,14 @@ gst_buffer_copy_into (GstBuffer * dest, GstBuffer * src,
     }
   }
 
+  if (sharing_mem && src->pool != NULL) {
+    /* The new buffer references some of src's memories. We have to ensure that
+     * src buffer does not return to its buffer pool as long as its memories are
+     * used by other buffers. That would cause the buffer to be discarted by the
+     * pool because its memories are not writable. */
+    gst_buffer_add_parent_buffer_meta (dest, src);
+  }
+
   return TRUE;
 }
 

diff --git a/subprojects/gstreamer/tests/check/gst/gstbufferpool.c b/subprojects/gstreamer/tests/check/gst/gstbufferpool.c
index d1d87cd..737dc24 100644 (file)
--- a/subprojects/gstreamer/tests/check/gst/gstbufferpool.c
+++ b/subprojects/gstreamer/tests/check/gst/gstbufferpool.c
@@ -388,6 +388,62 @@ GST_START_TEST (test_parent_meta)
 
 GST_END_TEST;
 
+GST_START_TEST (test_make_writable_parent_meta)
+{
+  GstBufferPool *pool;
+  GstBuffer *buf1, *buf2, *buf3;
+  GstMemory *mem1, *mem2;
+  gint buf1_dcount = 0;
+  gint buf2_dcount = 0;
+
+  pool = create_pool (1, 0, 0);
+  fail_unless (pool);
+  gst_buffer_pool_set_active (pool, TRUE);
+
+  fail_unless (gst_buffer_pool_acquire_buffer (pool, &buf1,
+          NULL) == GST_FLOW_OK);
+  buffer_track_destroy (buf1, &buf1_dcount);
+
+  /* Make buf1 not writable and copy it */
+  gst_buffer_ref (buf1);
+  buf2 = gst_buffer_make_writable (buf1);
+  buffer_track_destroy (buf2, &buf2_dcount);
+  fail_unless (buf1 != buf2);
+  fail_unless (gst_buffer_is_writable (buf2));
+
+  /* buf1 and buf2 should be sharing the same memory */
+  mem1 = gst_buffer_peek_memory (buf1, 0);
+  mem2 = gst_buffer_peek_memory (buf2, 0);
+  fail_unless_equals_pointer (mem1, mem2);
+
+  /* buf1 is still reffed by the meta */
+  gst_buffer_unref (buf1);
+  fail_unless_equals_int (buf1_dcount, 0);
+  fail_unless_equals_int (buf2_dcount, 0);
+
+  /* buf2 gets destroyed and buf1 returns to pool */
+  gst_buffer_unref (buf2);
+  fail_unless_equals_int (buf1_dcount, 0);
+  fail_unless_equals_int (buf2_dcount, 1);
+
+  /* buf1 should be recycled with the same memory */
+  fail_unless (gst_buffer_pool_acquire_buffer (pool, &buf3,
+          NULL) == GST_FLOW_OK);
+  fail_unless_equals_pointer (buf1, buf3);
+  fail_unless_equals_pointer (mem1, gst_buffer_peek_memory (buf3, 0));
+
+  gst_buffer_unref (buf3);
+  fail_unless_equals_int (buf1_dcount, 0);
+  fail_unless_equals_int (buf2_dcount, 1);
+
+  gst_buffer_pool_set_active (pool, FALSE);
+  gst_object_unref (pool);
+  fail_unless_equals_int (buf1_dcount, 1);
+  fail_unless_equals_int (buf2_dcount, 1);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_buffer_pool_suite (void)
 {
@@ -408,6 +464,7 @@ gst_buffer_pool_suite (void)
   tcase_add_test (tc_chain, test_flushing_pool_returns_flushing);
   tcase_add_test (tc_chain, test_no_deadlock_for_buffer_discard);
   tcase_add_test (tc_chain, test_parent_meta);
+  tcase_add_test (tc_chain, test_make_writable_parent_meta);
 
   return s;
 }