[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 #13 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #9)
> Review of attachment 371192 [details] [review]:
> 
> ::: gst-libs/gst/rtp/gstrtpbaseaudiopayload.c
> @@ +160,3 @@
> +    goto wrong_config;
> +
> +  self->size = size;
> 
> What if this size is bigger than our MTU here, or does not comply with the
> min/max ptime?
> 

The input buffers are pushed to an adapter, then chopped up into new buffers,
which we don't resize in prepare_output_buffer () (buffer->pool != self->pool).

As these new buffers are taken with take_fast, the memories are likely to be
shared / resized further, causing the memory flag to be set and the buffers
being discarded when released. The behaviour of the payloader is unaffected, as
shown by:

gst-launch-1.0 audiotestsrc volume=0.05 samplesperbuffer=100 ! alawenc mtu=200
! rtppcmapay mtu=50 ! rtppcmadepay ! alawdec ! autoaudiosink

> @@ +188,3 @@
> +
> +static GstBufferPool *
> +gst_rtp_buffer_pool_new (gsize size)
> 
> size is unused

Indeed, fixed

> 
> @@ +1025,3 @@
> +
> +  if (!gst_buffer_pool_set_config (self->priv->pool, structure))
> +    goto config_failed;
> 
> This is supposed to be done by the consumer of the pool

Right, leftover from previous code, fixed.

(In reply to Sebastian Dröge (slomo) from comment #10)
> Review of attachment 371193 [details] [review]:
> 
> alawenc does not output fixed-size buffers, but depending on the input
> buffer size, or not?
> 

That is correct, it is the reason why I documented the new MTU property as "Set
the expected maximum size of output buffers". If the input buffers end up
resulting in different-from-MTU buffers being output, the buffers are either
resized to a lower size, causing the buffers to be released back to the buffer
pool, or the pool is not used (with my latest audioencoder patch :P).


> ::: gst-libs/gst/audio/gstaudioencoder.c
> @@ +2953,3 @@
> +  if (enc->priv->ctx.pool) {
> +    GstFlowReturn ret =
> +        gst_buffer_pool_acquire_buffer (enc->priv->ctx.pool, &buffer, NULL);
> 
> Should probably sanity-check that size <= max_size here

I've actually updated this to only acquire buffers from the pool if size <=
self->pool_size.

(In reply to Nicolas Dufresne (stormer) from comment #12)
> Review of attachment 371192 [details] [review]:
> 
> ::: 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.

I actually didn't implement the patch you were thinking of as discussed on IRC,
I'll propose my amendment to that patch here, you are right that we shouldn't
need to manipulate the offset here. The size however will still need to be
manipulated, as at this point we are resetting the buffer to the state that
acquire_buffer expects, which is the same as after alloc_buffer.

> 
> @@ +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.
> 

Hm, not sure what you mean, checking the return value of set_config seems like
the right thing to do ? :)

> @@ +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.
> 

Fixed

> @@ +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).

I ended up removing the set_config call in baseaudiopayload altogether, see my
latest patch (attaching shortly).

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