From: Xavier Claessens Date: Wed, 15 Mar 2023 13:11:51 +0000 (-0400) Subject: gstbuffer: Add parent meta when a copy shares memory with parent X-Git-Tag: 1.22.8~29 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d809406dfce5b46b4259495783f7c84662ed69a6;p=platform%2Fupstream%2Fgstreamer.git 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: --- diff --git a/subprojects/gst-devtools/validate/gst/validate/flow/formatting.c b/subprojects/gst-devtools/validate/gst/validate/flow/formatting.c index 773611844e..c9f8c782d9 100644 --- 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 441fc82710..a2fff0c855 100644 --- 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 d1d87cddae..737dc24f42 100644 --- 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; }