[Bug 776931] aggregator: add caps handling

[GStreamer (GNOME Bugzilla)]

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

Olivier Crête <olivier.crete at ocrete.ca> changed:

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

--- Comment #7 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 347899:
 --> (https://bugzilla.gnome.org/review?bug=776931&attachment=347899)

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.