[Bug 795021] Reduce amount of memory allocations when payloading fixed-size audio buffers

[GStreamer (GNOME Bugzilla)]

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

--- Comment #12 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 371192:
 --> (https://bugzilla.gnome.org/review?bug=795021&attachment=371192)

::: gst-libs/gst/rtp/gstrtpbaseaudiopayload.c
@@ +130,3 @@
+   * released to the pool again, see the resize code in
+   * gst_rtp_buffer_pool_acquire_buffer */
+  gst_buffer_resize (buffer, offset * -1, size + offset);

You implemented that in GstBufferPool:
  gst_buffer_resize (buffer, 0, pool->priv->size);

Which now that I think of is incorrect, and now your are hiding this bug here.
You should drop this reset code and fix GstBufferPool instead.

@@ +1025,3 @@
+
+  if (!gst_buffer_pool_set_config (self->priv->pool, structure))
+    goto config_failed;

I have done that on purpose in many places, because some of the video pool I
offer don't accept any other caps then the one being passed in the allocation
query. This way the pool can save the caps and fail properly. But I don't think
it applies here indeed.

@@ +1027,3 @@
+    goto config_failed;
+
+  gst_query_add_allocation_pool (query, self->priv->pool, 0, 0, 0);

Ground rule is that downstream should account of in-flight buffer, so min
should be 1 here. This is just to reduce run-time allocation (if possible).
Note sure it will make any difference here though.

@@ +1034,3 @@
+config_failed:
+  {
+    GST_ERROR_OBJECT (self, "failed to set config");

This is supposed to always be reached, because min == 1 is invalid (it's
usually a bug if you ended with a min == 0, and a pool without buffers is
useless). Maybe it only fails in video pools, would be nice to fix
GstBufferPool.

Though, set_config() will update the proposed config to something valid (as per
documentation). So calling set_config() to pre-configure a pool isn't a no-op.
The benefit here is that you can pre-configure a pool in a way that if upstream
element so set_active() without configuring it first, it will fail (not
configured yet).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.