[Bug 746529] New: videoaggregator: Adding support for downstream allocation negotiation

["GStreamer" (GNOME Bugzilla)]

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

Bug ID: 746529
Summary: videoaggregator: Adding support for downstream
allocation negotiation
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: enhancement
Priority: Normal
Component: gst-plugins-bad
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@vodalys-labs.com
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@collabora.co.uk
GNOME version: ---

Created attachment 299947
--> https://bugzilla.gnome.org/attachment.cgi?id=299947&action=edit
[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.

["GStreamer" (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <***@collabora.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #299947|none |needs-work
status| |

--- Comment #1 from Nicolas Dufresne (stormer) <***@collabora.co.uk> ---
Review of attachment 299947:
--> (https://bugzilla.gnome.org/review?bug=746529&attachment=299947)

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.

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ocrete.ca

--- Comment #2 from Olivier Crête <***@ocrete.ca> ---
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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #3 from Nicolas Dufresne (stormer) <***@collabora.co.uk> ---
(In reply to Olivier Crête from comment #2)

Post by "GStreamer" (GNOME Bugzilla)
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 ?)

["GStreamer" (GNOME Bugzilla)]

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

Jean-Michel Hautbois (jmleo) <jean-***@veo-labs.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@veo-labs.co
| |m,
| |jean-***@veo-la
| |bs.com

["GStreamer" (GNOME Bugzilla)]

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

kevin <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Depends on| |751203

["GStreamer" (GNOME Bugzilla)]

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

kevin <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Blocks| |751203
Depends on|751203 |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Depends on| |776931

["GStreamer" (GNOME Bugzilla)]

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

Nirbheek Chauhan <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=746529
Bug 746529 depends on bug 776931, which changed state.

Bug 776931 Summary: aggregator: add caps handling
https://bugzilla.gnome.org/show_bug.cgi?id=776931

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #4 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352230
--> https://bugzilla.gnome.org/attachment.cgi?id=352230&action=edit
aggregator: Add downstream allocation query

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #6 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352232
--> https://bugzilla.gnome.org/attachment.cgi?id=352232&action=edit
glbasemixer: Use aggregator for allocation handling

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #5 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352231
--> https://bugzilla.gnome.org/attachment.cgi?id=352231&action=edit
videoaggregator: Get the buffer from the pool if available

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #7 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352233
--> https://bugzilla.gnome.org/attachment.cgi?id=352233&action=edit
glbasemixer: Remove own decide_allocation, use GstAggregator's

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #8 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352234
--> https://bugzilla.gnome.org/attachment.cgi?id=352234&action=edit
audioaggregator: Use downstream allocator and params if available

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #9 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352235
--> https://bugzilla.gnome.org/attachment.cgi?id=352235&action=edit
videoaggregator: Create normal video pool as a fallback

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352230|none |needs-work
status| |

--- Comment #10 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 352230:
--> (https://bugzilla.gnome.org/review?bug=746529&attachment=352230)

.

::: 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?

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com
Attachment #352231|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352232|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352233|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352234|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352235|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #11 from Olivier Crête <***@ocrete.ca> ---
(In reply to Matthew Waters (ystreet00) from comment #10)

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/base/gstaggregator.c
@@ +2976,3 @@
+ * used
erm, this isn't transfer full in the code

I copied that from BaseTransform, and it seems to work for Python.

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352230|needs-work |none
status| |
Attachment #352230|0 |1
is obsolete| |

--- Comment #12 from Olivier Crête <***@ocrete.ca> ---
Created attachment 352268
--> https://bugzilla.gnome.org/attachment.cgi?id=352268&action=edit
aggregator: Add downstream allocation query

Added a bit more doc to decide_allocation

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352268|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #299947|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #13 from Olivier Crête <***@ocrete.ca> ---
Pushing all the patches, thanks for the review.

Attachment 352231 pushed as 31bbfd6 - videoaggregator: Get the buffer from the
pool if available
Attachment 352232 pushed as d3c2ccb - glbasemixer: Use aggregator for
allocation handling
Attachment 352233 pushed as 9897c5c - glbasemixer: Remove own
decide_allocation, use GstAggregator's
Attachment 352234 pushed as ea26b9a - audioaggregator: Use downstream allocator
and params if available
Attachment 352235 pushed as 8926938 - videoaggregator: Create normal video pool
as a fallback
Attachment 352268 pushed as 12197e4 - aggregator: Add downstream allocation
query

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352231|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352232|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352233|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352234|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352235|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Olivier Crête <***@ocrete.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #352268|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net
Target Milestone|git master |1.13.1

["GStreamer" (GNOME Bugzilla)]

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

Thibault Saunier <***@gnome.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gnome.org

--- Comment #14 from Thibault Saunier <***@gnome.org> ---
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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #15 from Sebastian Dröge (slomo) <***@coaxion.net> ---
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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #16 from Sebastian Dröge (slomo) <***@coaxion.net> ---
https://bugzilla.gnome.org/show_bug.cgi?id=797100

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #17 from Nicolas Dufresne (ndufresne) <***@ndufresne.ca> ---
Meanwhile, specific use case can workaround this with "identity
drop-allocation=1".

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #18 from Nicolas Dufresne (ndufresne) <***@ndufresne.ca> ---
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.