bufferpool: Use TAG_MEMORY to check memory before releasing

Tag allocated buffers with TAG_MEMORY. When they are released later,
only add them back to the pool if the tag is still there and the memory
has not been changed, otherwise throw the buffer away.
Add unit test to check various scenarios.

Fixes https://bugzilla.gnome.org/show_bug.cgi?id=724481

diff --git a/gst/gstbufferpool.c b/gst/gstbufferpool.c
index b0cffaf..0f03753 100644 (file)
--- a/gst/gstbufferpool.c
+++ b/gst/gstbufferpool.c
@@ -270,7 +270,13 @@ do_alloc_buffer (GstBufferPool * pool, GstBuffer ** buffer,
   if (G_UNLIKELY (result != GST_FLOW_OK))
     goto alloc_failed;
 
+  /* lock all metadata and mark as pooled, we want this to remain on
+   * the buffer and we want to remove any other metadata that gets added
+   * later */
   gst_buffer_foreach_meta (*buffer, mark_meta_pooled, pool);
+  /* tag memory, this is how we expect the buffer when it is
+   * released again */
+  GST_BUFFER_FLAG_SET (*buffer, GST_BUFFER_FLAG_TAG_MEMORY);
 
   GST_LOG_OBJECT (pool, "allocated buffer %d/%d, %p", cur_buffers,
       max_buffers, buffer);
@@ -1064,7 +1070,7 @@ remove_meta_unpooled (GstBuffer * buffer, GstMeta ** meta, gpointer user_data)
 static void
 default_reset_buffer (GstBufferPool * pool, GstBuffer * buffer)
 {
-  GST_BUFFER_FLAGS (buffer) = 0;
+  GST_BUFFER_FLAGS (buffer) &= GST_BUFFER_FLAG_TAG_MEMORY;
 
   GST_BUFFER_PTS (buffer) = GST_CLOCK_TIME_NONE;
   GST_BUFFER_DTS (buffer) = GST_CLOCK_TIME_NONE;
@@ -1126,10 +1132,16 @@ gst_buffer_pool_acquire_buffer (GstBufferPool * pool, GstBuffer ** buffer,
 static void
 default_release_buffer (GstBufferPool * pool, GstBuffer * buffer)
 {
-  /* keep it around in our queue */
-  GST_LOG_OBJECT (pool, "released buffer %p", buffer);
-  gst_atomic_queue_push (pool->priv->queue, buffer);
-  gst_poll_write_control (pool->priv->poll);
+  GST_LOG_OBJECT (pool, "released buffer %p %d", buffer,
+      GST_MINI_OBJECT_FLAGS (buffer));
+
+  if (GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_TAG_MEMORY)) {
+    /* keep it around in our queue */
+    gst_atomic_queue_push (pool->priv->queue, buffer);
+    gst_poll_write_control (pool->priv->poll);
+  } else {
+    do_free_buffer (pool, buffer);
+  }
 }
 
 /**

diff --git a/gst/gstbufferpool.h b/gst/gstbufferpool.h
index 91c12f5..5d26e81 100644 (file)
--- a/gst/gstbufferpool.h
+++ b/gst/gstbufferpool.h
@@ -132,14 +132,19 @@ struct _GstBufferPool {
  *        buffers from the configured memory allocator and with the configured
  *        parameters. All metadata that is present on the allocated buffer will
  *        be marked as #GST_META_FLAG_POOLED and #GST_META_FLAG_LOCKED and will
- *        not be removed from the buffer in @reset_buffer.
+ *        not be removed from the buffer in @reset_buffer. Memory will be marked
+ *        with GST_BUFFER_FLAG_TAG_MEMORY.
  * @reset_buffer: reset the buffer to its state when it was freshly allocated.
  *        The default implementation will clear the flags, timestamps and
  *        will remove the metadata without the #GST_META_FLAG_POOLED flag (even
- *        the metadata with #GST_META_FLAG_LOCKED).
+ *        the metadata with #GST_META_FLAG_LOCKED). If the
+ *        #GST_BUFFER_FLAG_TAG_MEMORY was removed, this function can also try to
+ *        restore the memory and set the #GST_BUFFER_FLAG_TAG_MEMORY again.
  * @release_buffer: release a buffer back in the pool. The default
  *        implementation will put the buffer back in the queue and notify any
- *        blocking acquire_buffer calls.
+ *        blocking acquire_buffer calls when the #GST_BUFFER_FLAG_TAG_MEMORY
+ *        is set on the buffer. If #GST_BUFFER_FLAG_TAG_MEMORY is not set, the 
+ *        buffer will be freed with @free_buffer.
  * @free_buffer: free a buffer. The default implementation unrefs the buffer.
  *
  * The GstBufferPool class.

diff --git a/tests/check/gst/gstbufferpool.c b/tests/check/gst/gstbufferpool.c
index 7809ba2..fd201b4 100644 (file)
--- a/tests/check/gst/gstbufferpool.c
+++ b/tests/check/gst/gstbufferpool.c
@@ -125,6 +125,46 @@ GST_START_TEST (test_inactive_pool_returns_flushing)
 
 GST_END_TEST;
 
+GST_START_TEST (test_buffer_modify_discard)
+{
+
+  GstBufferPool *pool = create_pool (10, 0, 0);
+  GstBuffer *buf = NULL, *prev;
+  GstMemory *mem;
+
+  gst_buffer_pool_set_active (pool, TRUE);
+  gst_buffer_pool_acquire_buffer (pool, &buf, NULL);
+  prev = buf;
+  /* remove all memory, pool should not reuse this buffer */
+  gst_buffer_remove_all_memory (buf);
+  gst_buffer_unref (buf);
+
+  gst_buffer_pool_acquire_buffer (pool, &buf, NULL);
+  fail_if (buf == prev, "got a reused buffer instead of new one");
+  prev = buf;
+  /* do resize, pool should not reuse this buffer */
+  gst_buffer_resize (buf, 5, 2);
+  gst_buffer_unref (buf);
+
+  gst_buffer_pool_acquire_buffer (pool, &buf, NULL);
+  fail_if (buf == prev, "got a reused buffer instead of new one");
+  prev = buf;
+  /* keep ref to memory, not exclusive so pool should reuse this buffer */
+  mem = gst_buffer_get_memory (buf, 0);
+  gst_buffer_unref (buf);
+  gst_memory_unref (mem);
+
+  gst_buffer_pool_acquire_buffer (pool, &buf, NULL);
+  fail_unless (buf == prev, "got a fresh buffer instead of previous");
+  mem = gst_buffer_get_memory (buf, 0);
+
+  gst_buffer_unref (buf);
+
+  gst_buffer_pool_set_active (pool, FALSE);
+  gst_object_unref (pool);
+}
+
+GST_END_TEST;
 
 static Suite *
 gst_buffer_pool_suite (void)
@@ -140,6 +180,7 @@ gst_buffer_pool_suite (void)
   tcase_add_test (tc_chain, test_buffer_out_of_order_reuse);
   tcase_add_test (tc_chain, test_pool_config_buffer_size);
   tcase_add_test (tc_chain, test_inactive_pool_returns_flushing);
+  tcase_add_test (tc_chain, test_buffer_modify_discard);
 
   return s;
 }