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 776931 - aggregator: add caps handling
aggregator: add caps handling
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:
Blocks: 746529
Reported: 2017-01-06 05:06 UTC by Matthew Waters (ystreet00)
Modified: 2017-05-20 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---

aggregator: add simple support src caps handling (45.30 KB, patch)
2017-01-06 05:14 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
aggregator: add simple support src caps handling (44.97 KB, patch)
2017-03-14 04:22 UTC, Matthew Waters (ystreet00)
needs-work Details | Review

Description Matthew Waters (ystreet00) 2017-01-06 05:06:57 UTC
Caps handling is generic enough that is should be placed into aggregator (or at least some support for it should be).  Also required as a prerequisite for handling allocation queries inside aggregator in bug 746529.
Comment 1 Matthew Waters (ystreet00) 2017-01-06 05:14:28 UTC
Created attachment 343000 [details] [review]
aggregator: add simple support src caps handling

This is a first pass (RFC) at adding src caps handling to aggregator modelled after videoaggregator.

It seems to work well enough for gst-validate (haven't tried ges's validate) and the unittests.

I strongly suspect that there are locking problems with the caps handling here as now the sink setting caps (in the subclasses) uses a different lock to the src setting/updating the caps.  Not sure if it matters yet though.
Comment 2 Olivier Crête 2017-01-06 20:33:51 UTC
Review of attachment 343000 [details] [review]:

::: ext/gl/gstopengl.c
@@ -239,0 +240,5 @@
+  if (!gst_element_register (plugin, "glmodelrender",
... 2 more ...

That seems unrelated
Comment 3 Olivier Crête 2017-01-06 20:45:53 UTC
Review of attachment 343000 [details] [review]:

Somewhat related to this, I had also written patches to delay processing of serialized events (like caps) to the output thread, to simplify locking, etc.

The rest of your patch feels like it's in the right direction.

::: gst-libs/gst/base/gstaggregator.c
@@ +830,3 @@
+  GstFlowReturn ret = GST_FLOW_OK;
+  downstream_caps = gst_pad_get_allowed_caps (self->srcpad);

I'm a bit concerned about using this without a filter, it may produce huge caps. By this point, we may already have information about what we receive from upstream, like for audio*. We'll already know the upstream format.

@@ +844,3 @@
+  }
+  peercaps = gst_pad_peer_query_caps (self->srcpad, NULL);

This is redundant with the gst_pad_get_allowed_caps () call? And again no filter.. I think we somehow need to let the subclass do a bit more.
Comment 4 Matthew Waters (ystreet00) 2017-01-09 03:19:17 UTC
Yea I was wondering about making caps/event processing occur on a single thread.  One could also expand/generalize that to N-M elements having a worker thread to simplify the locking requirements.

gst_pad_get_allowed_caps() only retrieves the pad templates.
gst_pad_peer_query_caps() does the query and could return massive caps.  One can pass the gst_pad_get_allowed_caps() result to the gst_pad_peer_query_caps() as the filter.  There isn't really much else generic caps-handling code could do.

These two calls are still much less than what basetransform does when negotiating ;).
Comment 5 Olivier Crête 2017-01-11 18:25:26 UTC
gst_pad_get_allowed_caps() calls gst_pad_peer_query() already!

I wonder if we should let the peer querying to the subclass as they will have more caps to intersect with (like the caps from all of the sink pads.
Comment 6 Matthew Waters (ystreet00) 2017-03-14 04:22:44 UTC
Created attachment 347899 [details] [review]
aggregator: add simple support src caps handling

Addresses most of the review comments.

Only one caps query is performed with the pad template caps as filter.
Comment 7 Olivier Crête 2017-03-17 22:53:13 UTC
Review of attachment 347899 [details] [review]:

Generally looks good, just a couple little details below:

There is probably a gst_pad_mark_reconfigure() missing in gst_audio_aggregator_set_sink_caps().

I wonder if there shouldnt be something for the sink setcaps function too in the base class? But I guess that would be a further patch, that would depend on the work on bug #779945.

::: gst-libs/gst/base/gstaggregator.c
@@ +800,3 @@
+  if (filter) {
+    *ret = gst_caps_intersect (caps, filter);

Should this be with GST_CAPS_INTERSECT_FIRST ? with the filter first?

@@ +834,3 @@
+  if (!downstream_caps || gst_caps_is_any (downstream_caps)) {
+    GST_DEBUG_OBJECT (self, "No negotiation necessary");

If downstream caps is ANY, you still need to aggregate the caps from multiple upstreams.

@@ +839,3 @@
+  if (gst_caps_is_empty (downstream_caps)) {
+    GST_INFO_OBJECT (self, "No downstream caps found %"
+        GST_PTR_FORMAT, downstream_caps);

This is not "no downstream caps found".. this is downstream elements not compatible with pad templates!

@@ +850,3 @@
+  GST_DEBUG_OBJECT (self, "       with filter %" GST_PTR_FORMAT, template_caps);
+  ret =
+      agg_klass->update_src_caps (self, downstream_caps, template_caps, &caps);

Why do you pass the template here if it has already been applied?

Can't the APIU only be update_src_caps(GstAggregator *self, GstCaps *downstream_filtered, GstCaps **res) ?

@@ +869,3 @@
+    g_assert (agg_klass->fixate_src_caps);
+    caps = gst_caps_make_writable (caps);

The header file mentions that this is not guaranteed to be writable, so either fix the doc string or remove this line.

@@ +918,3 @@
+    if (gst_pad_check_reconfigure (GST_AGGREGATOR_SRC_PAD (self))) {

I wonder if there arent more places where it needs to set reconfigure on the src pad, such as when a caps event is received on the sink pads?

::: gst-libs/gst/base/gstaggregator.h
@@ +124,2 @@

I wonder if those shouldn't be GST_AGGREGATOR_FLOW_...

::: gst/audiomixer/gstaudiointerleave.c
@@ +512,2 @@
+  if (self->sinkcaps == NULL || self->channels == 0)
+    return GST_FLOW_NEED_DATA;

This is NOT_NEGOTIATED, not need more data.
Comment 8 Sebastian Dröge (slomo) 2017-05-09 12:50:08 UTC
Also related to

Can we get this all updated and merged? :)
Comment 9 Olivier Crête 2017-05-20 14:22:29 UTC
Pushed an updated version as:

commit 7c53043386b359b5c07df2fd0394f3e6717e0590
Author: Matthew Waters <>
Date:   Sat May 20 14:24:57 2017 +0200

    aggregator: add simple support for caps handling
    Modelled off the videoaggregator caps handling as that seems the most
    mature aggregtor-using implementation that has caps handling there is.