After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 746529 - videoaggregator: Adding support for downstream allocation negotiation
videoaggregator: Adding support for downstream allocation negotiation
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 776931
Blocks: 751203
 
 
Reported: 2015-03-20 13:34 UTC by Pablo Anton
Modified: 2018-09-09 14:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[patch] Adding support for negotiating src pad bufferpool (17.64 KB, patch)
2015-03-20 13:34 UTC, Pablo Anton
needs-work Details | Review
aggregator: Add downstream allocation query (8.58 KB, patch)
2017-05-20 17:01 UTC, Olivier Crête
none Details | Review
videoaggregator: Get the buffer from the pool if available (6.44 KB, patch)
2017-05-20 17:01 UTC, Olivier Crête
committed Details | Review
glbasemixer: Use aggregator for allocation handling (5.73 KB, patch)
2017-05-20 17:01 UTC, Olivier Crête
committed Details | Review
glbasemixer: Remove own decide_allocation, use GstAggregator's (3.55 KB, patch)
2017-05-20 17:01 UTC, Olivier Crête
committed Details | Review
audioaggregator: Use downstream allocator and params if available (1.40 KB, patch)
2017-05-20 17:01 UTC, Olivier Crête
committed Details | Review
videoaggregator: Create normal video pool as a fallback (2.51 KB, patch)
2017-05-20 17:01 UTC, Olivier Crête
committed Details | Review
aggregator: Add downstream allocation query (8.79 KB, patch)
2017-05-21 11:25 UTC, Olivier Crête
committed Details | Review

Description Pablo Anton 2015-03-20 13:34:10 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.
Comment 1 Nicolas Dufresne (ndufresne) 2015-07-06 13:39:20 UTC
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, &params);

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, &currentPoolCaps, &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, &params);

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,
+        &params);

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.
Comment 2 Olivier Crête 2015-07-06 17:23:38 UTC
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.
Comment 3 Nicolas Dufresne (ndufresne) 2015-07-08 19:07:16 UTC
(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 ?)
Comment 4 Olivier Crête 2017-05-20 17:01:22 UTC
Created attachment 352230 [details] [review]
aggregator: Add downstream allocation query
Comment 5 Olivier Crête 2017-05-20 17:01:28 UTC
Created attachment 352231 [details] [review]
videoaggregator: Get the buffer from the pool if available
Comment 6 Olivier Crête 2017-05-20 17:01:33 UTC
Created attachment 352232 [details] [review]
glbasemixer: Use aggregator for allocation handling
Comment 7 Olivier Crête 2017-05-20 17:01:39 UTC
Created attachment 352233 [details] [review]
glbasemixer: Remove own decide_allocation, use GstAggregator's
Comment 8 Olivier Crête 2017-05-20 17:01:45 UTC
Created attachment 352234 [details] [review]
audioaggregator: Use downstream allocator and params if available
Comment 9 Olivier Crête 2017-05-20 17:01:50 UTC
Created attachment 352235 [details] [review]
videoaggregator: Create normal video pool as a fallback
Comment 10 Matthew Waters (ystreet00) 2017-05-20 17:11:12 UTC
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?
Comment 11 Olivier Crête 2017-05-21 11:25:10 UTC
(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.
Comment 12 Olivier Crête 2017-05-21 11:25:34 UTC
Created attachment 352268 [details] [review]
aggregator: Add downstream allocation query

Added a bit more doc to decide_allocation
Comment 13 Olivier Crête 2017-05-21 12:36:27 UTC
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
Comment 14 Thibault Saunier 2018-09-08 16:02:02 UTC
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?
Comment 15 Sebastian Dröge (slomo) 2018-09-08 16:15:38 UTC
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?
Comment 16 Sebastian Dröge (slomo) 2018-09-09 07:44:17 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=797100
Comment 17 Nicolas Dufresne (ndufresne) 2018-09-09 14:19:31 UTC
Meanwhile, specific use case can workaround this with "identity drop-allocation=1".
Comment 18 Nicolas Dufresne (ndufresne) 2018-09-09 14:37:24 UTC
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.