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.

Review of attachment 343000 [details] [review]:

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

That seems unrelated

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.

https://git.collabora.com/cgit/user/tester/gst-plugins-bad.git/log/?h=agg-neg-2

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.

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 ;).

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.

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.

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 @@
       continue;
 
+    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 @@
 #define GST_FLOW_NOT_HANDLED           GST_FLOW_CUSTOM_SUCCESS
+#define GST_FLOW_NEED_DATA             GST_FLOW_CUSTOM_ERROR

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.

Also related to https://bugzilla.gnome.org/show_bug.cgi?id=781673

Can we get this all updated and merged? :)

Pushed an updated version as:

commit 7c53043386b359b5c07df2fd0394f3e6717e0590
Author: Matthew Waters <matthew@centricular.com>
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.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776931