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 773611844e783fe3794c0d342fedd6b2bb75fa8c..c9f8c782d91b68e6d126517c57f8c89676497086 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 441fc82710039e6a691aa97cf1760b3693241c35..a2fff0c855ff6334ac7cf8ededfd0aa0d78fcc36 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 d1d87cddae83b241b1d07fac0457369e6adc216e..737dc24f426253df19c8d34b5367f621a86aeb60 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;
 }