GNOME Bugzilla – Bug 795021
Reduce amount of memory allocations when payloading fixed-size audio buffers
Last modified: 2018-11-03 12:04:52 UTC
This is a follow-up on https://bugzilla.gnome.org/show_bug.cgi?id=794544
Created attachment 370569 [details] [review] baseaudiopayload: propose bufferpool Implement a bufferpool subclass, allocating buffers large enough to prefix them with RTP headers, then resizing them at acquire_buffer time for the upstream element to write in the payload region
Created attachment 370570 [details] [review] audioencoder: try using downstream buffer pool A few audioencoder subclasses (eg alawenc) output fixed-sized buffers. If downstream provides a bufferpool with a large enough size, use it.
Review of attachment 370569 [details] [review]: ::: gst-libs/gst/rtp/gstrtpbaseaudiopayload.c @@ +96,3 @@ +gst_rtp_buffer_pool_dispose (GObject * object) +{ + G_OBJECT_CLASS (pool_parent_class)->dispose (object); This is useless @@ +133,3 @@ + + size = gst_buffer_get_sizes (buffer, &offset, NULL); + gst_buffer_resize (buffer, offset * -1, size + offset); This needs a comment, why the -1 :) We should probably also check here if the offset is as expected or that we can reset the buffer correctly? @@ +1000,3 @@ + return FALSE; + + if (gst_query_get_n_allocation_pools (query) == 0) { Why do we only add it if there is none yet? Should be safe to always add it @@ +1008,3 @@ + + structure = gst_buffer_pool_get_config (self->priv->pool); + gst_buffer_pool_config_set_params (structure, caps, size, 0, 0); Are all these things not usually set by whoever did the query to configure the buffer pool? That code might override the size and stuff
Review of attachment 370570 [details] [review]: ::: gst-libs/gst/audio/gstaudioencoder.c @@ +2790,3 @@ + + config = gst_buffer_pool_get_config (pool); + gst_buffer_pool_config_get_params (config, NULL, &enc->priv->ctx.pool_size, This should set the size. So we should probably have a property on the audio encoders to set the MTU or so, and then configure that on the pool. And also resize output buffers accordingly if less than that was written.
Created attachment 371192 [details] [review] baseaudiopayload: propose bufferpool Implement a bufferpool subclass, allocating buffers large enough to prefix them with RTP headers, then resizing them at acquire_buffer time for the upstream element to write in the payload region
Created attachment 371193 [details] [review] audioencoder: try using downstream buffer pool A few audioencoder subclasses (eg alawenc) output fixed-sized buffers. If downstream provides a bufferpool with a large enough size, use it.
Created attachment 371194 [details] [review] alwenc: expose an MTU property This in order to make use of the new AudioEncoder vmethod, get_max_output_size. If the user sets an expected maximum output buffer size, the base class can then make use of buffer pools to reduce memory allocations.
Addressed review comments, apart from : (In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 370569 [details] [review] [review]: > @@ +133,3 @@ > + > + size = gst_buffer_get_sizes (buffer, &offset, NULL); > + gst_buffer_resize (buffer, offset * -1, size + offset); > > This needs a comment, why the -1 :) We should probably also check here if > the offset is as expected or that we can reset the buffer correctly? I did add a comment, but the reset_buffer vmethod returns void, so I figured a check was pretty much useless :)
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? @@ +188,3 @@ + +static GstBufferPool * +gst_rtp_buffer_pool_new (gsize size) size is unused @@ +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
Review of attachment 371193 [details] [review]: alawenc does not output fixed-size buffers, but depending on the input buffer size, or not? ::: 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
Review of attachment 371194 [details] [review]: The encoder should enforce this, and split output buffers accordingly if needed. But nonetheless it's not clear how that would interfere with an incompatible mtu or min/max ptime in the payloader
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. @@ +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).
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 371192 [details] [review] [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] [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] [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).
Created attachment 371270 [details] [review] baseaudiopayload: propose bufferpool Implement a bufferpool subclass, allocating buffers large enough to prefix them with RTP headers, then resizing them at acquire_buffer time for the upstream element to write in the payload region
Created attachment 371271 [details] [review] audioencoder: try using downstream buffer pool A few audioencoder subclasses (eg alawenc) output fixed-sized buffers. If downstream provides a bufferpool with a large enough size, use it.
Created attachment 371273 [details] [review] bufferpool: improve buffer resizing code at reset time If an offset was set on the buffer, we have to reset it, so to ensure the resize operation will succeed.
Created attachment 371274 [details] [review] baseaudiopayload: propose bufferpool Implement a bufferpool subclass, allocating buffers large enough to prefix them with RTP headers, then resizing them at acquire_buffer time for the upstream element to write in the payload region
> (In reply to Nicolas Dufresne (stormer) from comment #12) > > Review of attachment 371192 [details] [review] [review] [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. Disregard that last part, as it turns out the resize code in the reset_buffer implementation, was doing exactly nothing, as the offset was 0 here already, so in effect we were calling gst_buffer_resize (buffer, 0, size + 0); As it turns out, we cannot resize here to pool_size + header_len anyway, because the release function runs after that and discards the buffer as it has a different size. Instead I updated the patch to simply ensure the buffer size is as expected in acquire_buffer, before the call to initialize_header.
(In reply to Mathieu Duponchelle from comment #13) > (In reply to Sebastian Dröge (slomo) from comment #9) > > Review of attachment 371192 [details] [review] [review] [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. Ok then we just go into the normal code path again. Can you add a comment about that somewhere in the code? Also I'm not entirely sure this is correct. In theory upstream could take one of our buffers, have the single reference and add memories or resize it or whatever. But that does not change buffer->pool, does it? > (In reply to Sebastian Dröge (slomo) from comment #10) > > Review of attachment 371193 [details] [review] [review] [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). How would alawenc now behave on input that is bigger than the MTU? Would it split buffers or ...? Would it do something with the buffer pool? > > ::: 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. I guess this answers my previous question :)
(In reply to Sebastian Dröge (slomo) from comment #19) > Ok then we just go into the normal code path again. Can you add a comment > about that somewhere in the code? will do :) > > Also I'm not entirely sure this is correct. In theory upstream could take > one of our buffers, have the single reference and add memories or resize it > or whatever. But that does not change buffer->pool, does it? It will not change it no. We could check if the memory tag is set, and then compare the buffer sizes with our expectations maybe ? > I guess this answers my previous question :) I think it does yes :)
(In reply to Mathieu Duponchelle from comment #20) > > > > Also I'm not entirely sure this is correct. In theory upstream could take > > one of our buffers, have the single reference and add memories or resize it > > or whatever. But that does not change buffer->pool, does it? > > It will not change it no. We could check if the memory tag is set, and then > compare the buffer sizes with our expectations maybe ? Yes and I believe you want to unset that flag if the buffer looks according to our expectations? Isn't every buffer that has the flag set discarded?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/431.