GNOME Bugzilla – Bug 746529
videoaggregator: Adding support for downstream allocation negotiation
Last modified: 2018-09-09 14:37:24 UTC
Created attachment 299947 [details] [review] [patch] Adding support for negotiating src pad bufferpool Videoaggregator and compositor don't support right now the possibility of using bufferpool from downstream. In order to do that, and therefore saving unnecessary buffer copies, it is necessary to implement decide_allocation method that performs allocation query to downstream element. This patch version configures bufferpool only once, due to a reconfiguration can provoke misbehavior of the pool.
Review of attachment 299947 [details] [review]: And there is few missing doc updates. ::: gst-libs/gst/video/gstvideoaggregator.c @@ +46,3 @@ GST_DEBUG_CATEGORY_STATIC (gst_videoaggregator_debug); #define GST_CAT_DEFAULT gst_videoaggregator_debug +#define GST_SRC_MIN_BUFFERS 16 That seems like a lot of buffer. Can you explain why an aggregator would need at least 16 buffers to operate ? @@ +388,3 @@ + static const GEnumValue videoaggregator_captureiomode[] = { + {VIDEOAGGREGATOR_CAPTUREIOMODE_IMPORT, "Import", "import"}, + {VIDEOAGGREGATOR_CAPTUREIOMODE_OWN, "Own", "own"}, I'm not sure everyone will like the V4L naming here. Maybe output-mode, or buffer-mode as we only support one direction ? @@ +445,3 @@ + /* Allocation buffers fields */ + GstBufferPool *pool; + gboolean pool_active; I would recommend using gst_buffer_pool_is_active() instead. @@ +610,3 @@ +gst_videoaggregator_set_allocation (GstVideoAggregator * vagg, + GstBufferPool * pool, GstAllocator * allocator, + GstAllocationParams * params, GstQuery * query) This function looks good. One note, should there be a special case for when the oldpool == pool ? @@ +675,3 @@ + + /* by default we remove all metadata, subclasses should implement a + * filter_meta function */ Shouldn't that code be part of a filter_meta default implementation instead ? @@ +709,3 @@ + /* no pool, we can make our own */ + GST_DEBUG_OBJECT (vagg, "no pool, making new pool"); + pool = gst_buffer_pool_new (); As a videoaggregator, it would be better to default to a video buffer pool which will add VideoMeta when enabled. Though this is slightly cosmetic. @@ +716,3 @@ + } + + /* FIXME: */ Fixme are nicer when we know what need fixing. @@ +718,3 @@ + /* FIXME: */ + if (min < GST_SRC_MIN_BUFFERS && + GST_SRC_MIN_BUFFERS < max) Coding style, I believe this fits on a single line. @@ +719,3 @@ + if (min < GST_SRC_MIN_BUFFERS && + GST_SRC_MIN_BUFFERS < max) + min = GST_SRC_MIN_BUFFERS; This seems odd. E.g. if you picked a default pool, min = 1, and max = 0 would be perfect, I don't see why you need to allocate 16 buffers here. @@ +727,3 @@ + config = gst_buffer_pool_get_config (pool); + gst_buffer_pool_config_set_params (config, outcaps, size, min, max); + gst_buffer_pool_config_set_allocator (config, allocator, ¶ms); I think it nicer to enable VideoMeta when supported. @@ +793,3 @@ + poolconfig = gst_buffer_pool_get_config (vagg->priv->pool); + + gst_buffer_pool_config_get_params(poolconfig, ¤tPoolCaps, &size, &min, Coding style. @@ +835,3 @@ + */ + if (vagg->priv->pool) { + result = gst_videoaggregator_check_src_pool(vagg, caps); Coding style, space before (. @@ +836,3 @@ + if (vagg->priv->pool) { + result = gst_videoaggregator_check_src_pool(vagg, caps); + GST_WARNING_OBJECT(vagg, "Pool already configured %s...", Coding style. @@ +859,3 @@ + * parse them */ + if (gst_query_get_n_allocation_params (query) > 0) { + gst_query_parse_nth_allocation_param (query, 0, &allocator, ¶ms); This seems to be parsed in set_allocaiton() too. @@ +2133,3 @@ + /* Check if we have a pool buffer */ + if (priv->pool) { + if (!priv->pool_active) { gst_buffer_pool_is_active() would do, but in general, just call _set_active (pool, TRUE) would do that same. Stepping back, I think it would be better to do like basesrc here, and activate the pool in decide_allocation() (or similar function). @@ +2150,3 @@ + GST_DEBUG_OBJECT (vagg, "Allocating new buffer of %d and params %" GST_PTR_FORMAT, + outsize, + ¶ms); Style error here, also, I believe we always create a pool, so this case should not exist. ::: gst-libs/gst/video/gstvideoaggregator.h @@ +56,3 @@ + * @VIDEOAGGREGATOR_CAPTUREIOMODE_OWNS: Use its own allocation buffer + * allocation method + */ Since marker missing. @@ +103,3 @@ + * @decide_allocation: Setup the allocation parameters for allocating output + * buffers. The passed in query contains the result of the + * downstream allocation query. Since marker missing.
I'm not sure why you'd ever want to not use a downstream buffer pool. I'd also like to put the decide_allocation (and caps negotiation) infrastructure into GstAggregator directly, as the same things are also useful for audio.
(In reply to Olivier Crête from comment #2) > I'm not sure why you'd ever want to not use a downstream buffer pool. I'd > also like to put the decide_allocation (and caps negotiation) infrastructure > into GstAggregator directly, as the same things are also useful for audio. About the first comment, this would happen if you use a 2D blitter for the mixing and want to export DMABUF. E.g. downstream pool is sysmem/GLTexture, but you can export DMABUF so that GL side do linux-dmabuf-import. About the second, I think something like GstBaseTransform, which handle the allocator part iirc, while the buffer pool is video/audio specific (is there buffer pool for audio ?)
Created attachment 352230 [details] [review] aggregator: Add downstream allocation query
Created attachment 352231 [details] [review] videoaggregator: Get the buffer from the pool if available
Created attachment 352232 [details] [review] glbasemixer: Use aggregator for allocation handling
Created attachment 352233 [details] [review] glbasemixer: Remove own decide_allocation, use GstAggregator's
Created attachment 352234 [details] [review] audioaggregator: Use downstream allocator and params if available
Created attachment 352235 [details] [review] videoaggregator: Create normal video pool as a fallback
Review of attachment 352230 [details] [review]: . ::: gst-libs/gst/base/gstaggregator.c @@ +2976,3 @@ + * used + * @params: (out) (allow-none) (transfer full): the + * #GstAllocationParams of @allocator erm, this isn't transfer full in the code ::: gst-libs/gst/base/gstaggregator.h @@ +209,3 @@ * Notifies subclasses what caps format has been negotiated + * @decide_allocation: Optional. + * Allows the subclass to influence the allocation choices. Extend the comment with some inspiration from GstBaseTransform::decide_allocation?
(In reply to Matthew Waters (ystreet00) from comment #10) > Review of attachment 352230 [details] [review] [review]: > > ::: gst-libs/gst/base/gstaggregator.c > @@ +2976,3 @@ > + * used > + * @params: (out) (allow-none) (transfer full): the > + * #GstAllocationParams of @allocator > > erm, this isn't transfer full in the code I copied that from BaseTransform, and it seems to work for Python.
Created attachment 352268 [details] [review] aggregator: Add downstream allocation query Added a bit more doc to decide_allocation
Pushing all the patches, thanks for the review. Attachment 352231 [details] pushed as 31bbfd6 - videoaggregator: Get the buffer from the pool if available Attachment 352232 [details] pushed as d3c2ccb - glbasemixer: Use aggregator for allocation handling Attachment 352233 [details] pushed as 9897c5c - glbasemixer: Remove own decide_allocation, use GstAggregator's Attachment 352234 [details] pushed as ea26b9a - audioaggregator: Use downstream allocator and params if available Attachment 352235 [details] pushed as 8926938 - videoaggregator: Create normal video pool as a fallback Attachment 352268 [details] pushed as 12197e4 - aggregator: Add downstream allocation query
WHile I believe it is the right thing to do it introduces a big "perf" issue in NLE/Pitivi where we try to buffer a lot so the playback is smooth later on. With these patches on "stack changes" in nlecomposition (ie. when we change the set of elements used by the composition) we end up blocking on the allocation query, until the downstream queue is drained, meaning that we won't be able to keep buffering more than 1 stack at a time. I am not sure how that should be handled, any idea?
That seems kind of suboptimal indeed and makes usage of compositor/etc rather inconvenient. In all dynamic-live scenarios you would right now have to drop the allocation query on its source pad. Can you file a blocker bug for 1.15/16 for this?
https://bugzilla.gnome.org/show_bug.cgi?id=797100
Meanwhile, specific use case can workaround this with "identity drop-allocation=1".
Hey Olivier, all the new functions in these patches are missing the locking comments you added last year. I would have believed you would have cared about this in your review.