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 786344 - audioaggregator: implement input conversion
audioaggregator: implement input conversion
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 773762 (view as bug list)
Depends on:
Blocks: 791218
 
 
Reported: 2017-08-16 01:33 UTC by Mathieu Duponchelle
Modified: 2018-01-27 11:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audioaggregator: implement input conversion (53.92 KB, patch)
2017-08-16 01:33 UTC, Mathieu Duponchelle
none Details | Review
audioaggregator: implement input conversion (56.71 KB, patch)
2017-09-21 20:27 UTC, Mathieu Duponchelle
none Details | Review
audioaggregator: implement input conversion (57.23 KB, patch)
2017-09-22 12:15 UTC, Mathieu Duponchelle
none Details | Review
audioaggregator: implement input conversion (61.76 KB, patch)
2017-10-19 16:14 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2017-08-16 01:33:46 UTC
See the documentation for more details :)
Comment 1 Mathieu Duponchelle 2017-08-16 01:33:51 UTC
Created attachment 357684 [details] [review]
audioaggregator: implement input conversion
Comment 2 Olivier Crête 2017-08-18 20:02:12 UTC
Review of attachment 357684 [details] [review]:

Generally looks like a good first step!

::: gst-libs/gst/audio/gstaudioaggregator.c
@@ +300,3 @@
+G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GstAudioAggregator, gst_audio_aggregator,
+    GST_TYPE_AGGREGATOR, g_type_add_class_private (g_define_type_id,
+        sizeof (GstAudioAggregatorClassPrivate)));

Any reason not to use G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE ?

@@ +504,3 @@
+/* Unref after usage */
+static GstAudioAggregatorPad *
+gst_audio_aggregator_get_first_configured_pad (GstAggregator * agg)

s/first/random (or any?)/ .. I don't thing being the first in the list means anything?

@@ +539,3 @@
+
+  if (downstream_caps && !gst_caps_is_empty (downstream_caps))
+    s2 = gst_caps_get_structure (downstream_caps, 0);

s2 gets leaked

@@ +548,3 @@
+    gst_structure_fixate_field_nearest_int (s, "rate",
+        first_configured_pad->info.rate);
+    gst_object_unref (first_configured_pad);

You will leak the pad if the first if() succeeds... You probably want to release it independantly from this.

@@ +593,3 @@
+    GST_OBJECT_UNLOCK (aaggpad);
+    gst_pad_push_event (GST_PAD (aaggpad), gst_event_new_reconfigure ());
+    GST_OBJECT_LOCK (aaggpad);

re-lock not needed, you can just unlock the mutex in the else branch too

@@ +752,3 @@
+   * format */
+  if (aaggpad->priv->buffer) {
+    GstBuffer *inbuf = gst_aggregator_pad_get_buffer (aggpad);

It's not safe to call this function with the pad's object lock held.

::: gst/audiomixer/gstaudiointerleave.c
@@ -585,3 @@
   aagg_class->aggregate_one_buffer = gst_audio_interleave_aggregate_one_buffer;
 
-

You seem to be missing the _set_convertible() in this subclass ?
Comment 3 Mathieu Duponchelle 2017-09-21 20:26:38 UTC
(In reply to Olivier Crête from comment #2)
> Review of attachment 357684 [details] [review] [review]:
> 
> Generally looks like a good first step!
> 
> ::: gst-libs/gst/audio/gstaudioaggregator.c
> @@ +300,3 @@
> +G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GstAudioAggregator, gst_audio_aggregator,
> +    GST_TYPE_AGGREGATOR, g_type_add_class_private (g_define_type_id,
> +        sizeof (GstAudioAggregatorClassPrivate)));
> 
> Any reason not to use G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE ?
> 

It's a *class* private, no macros for that :)

> @@ +504,3 @@
> +/* Unref after usage */
> +static GstAudioAggregatorPad *
> +gst_audio_aggregator_get_first_configured_pad (GstAggregator * agg)
> 
> s/first/random (or any?)/ .. I don't thing being the first in the list means
> anything?
> 

Well I'd rather the function says what it's actually doing :)

> @@ +539,3 @@
> +
> +  if (downstream_caps && !gst_caps_is_empty (downstream_caps))
> +    s2 = gst_caps_get_structure (downstream_caps, 0);
> 
> s2 gets leaked
> 

gst_caps_get_structure is (transfer none) :)

> @@ +548,3 @@
> +    gst_structure_fixate_field_nearest_int (s, "rate",
> +        first_configured_pad->info.rate);
> +    gst_object_unref (first_configured_pad);
> 
> You will leak the pad if the first if() succeeds... You probably want to
> release it independantly from this.
> 

Indeed, fixed :)

> @@ +593,3 @@
> +    GST_OBJECT_UNLOCK (aaggpad);
> +    gst_pad_push_event (GST_PAD (aaggpad), gst_event_new_reconfigure ());
> +    GST_OBJECT_LOCK (aaggpad);
> 
> re-lock not needed, you can just unlock the mutex in the else branch too
> 

I actually only needed locking in the else branch anyway, updated :)

> @@ +752,3 @@
> +   * format */
> +  if (aaggpad->priv->buffer) {
> +    GstBuffer *inbuf = gst_aggregator_pad_get_buffer (aggpad);
> 
> It's not safe to call this function with the pad's object lock held.
> 

Side-stepped the issue entirely, by keeping a ref on the input buffer, as we discussed on IRC.

> ::: gst/audiomixer/gstaudiointerleave.c
> @@ -585,3 @@
>    aagg_class->aggregate_one_buffer =
> gst_audio_interleave_aggregate_one_buffer;
>  
> -
> 
> You seem to be missing the _set_convertible() in this subclass ?

I've changed the name of that class method to perform_conversion, if not called during class_init the default behaviour is to not perform conversion, as it is only checked once it doesn't really need a boolean parameter.
Comment 4 Mathieu Duponchelle 2017-09-21 20:27:29 UTC
Created attachment 360227 [details] [review]
audioaggregator: implement input conversion
Comment 5 Sebastian Dröge (slomo) 2017-09-22 07:50:53 UTC
(In reply to Mathieu Duponchelle from comment #3)
> (In reply to Olivier Crête from comment #2)
> > Review of attachment 357684 [details] [review] [review] [review]:
> > 
> > Generally looks like a good first step!
> > 
> > ::: gst-libs/gst/audio/gstaudioaggregator.c
> > @@ +300,3 @@
> > +G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GstAudioAggregator, gst_audio_aggregator,
> > +    GST_TYPE_AGGREGATOR, g_type_add_class_private (g_define_type_id,
> > +        sizeof (GstAudioAggregatorClassPrivate)));
> > 
> > Any reason not to use G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE ?
> > 
> 
> It's a *class* private, no macros for that :)

Thanks for being the first user (I'm aware of) of this feature. I added that to GObject ages ago, but before it was merged we broke GStreamer API and I didn't need it anymore ;)
Comment 6 Sebastian Dröge (slomo) 2017-09-22 08:41:17 UTC
Review of attachment 360227 [details] [review]:

::: gst-libs/gst/audio/gstaudioaggregator.c
@@ +57,3 @@
+    GST_PAD_SINK,
+    GST_PAD_REQUEST,
+    SINK_CAPS);

Subclasses might want to limit the caps in one way or another. E.g. only allow mono inputs.

@@ +135,3 @@
+      if (pad->priv->converter_config)
+        g_value_take_boxed (value,
+            gst_structure_copy (pad->priv->converter_config));

Or just g_value_set_boxed(value, config)? That copies internally then. Less noise

@@ +592,3 @@
+  /* TODO: handle different rates on sinkpads, a bit complex
+   * because offsets will have to be updated, and audio resampling
+   * has a latency to take into account

You should prevent that in get_caps() then if it's not supported yet (ignoring the race condition between getcaps and setcaps).

::: gst-libs/gst/audio/gstaudioaggregator.h
@@ +168,3 @@
 
+GST_EXPORT
+void gst_audio_aggregator_class_perform_conversion (GstAudioAggregatorClass * klass);

Should probably take a boolean so sub-subclasses can override it, and maybe a vfunc that does the conversion and can be overridden?

::: gst/audiomixer/gstaudiomixer.h
@@ -53,3 @@
-
-  /* target caps (set via property) */
-  GstCaps *filter_caps;

\o/ It's gone finally
Comment 7 Mathieu Duponchelle 2017-09-22 12:09:48 UTC
(In reply to Sebastian Dröge (slomo) from comment #6)
> Review of attachment 360227 [details] [review] [review]:
> 
> ::: gst-libs/gst/audio/gstaudioaggregator.c
> @@ +57,3 @@
> +    GST_PAD_SINK,
> +    GST_PAD_REQUEST,
> +    SINK_CAPS);
> 
> Subclasses might want to limit the caps in one way or another. E.g. only
> allow mono inputs.
> 

They can do that by overriding the static template after calling perform_conversion , does that work for you ?

> @@ +135,3 @@
> +      if (pad->priv->converter_config)
> +        g_value_take_boxed (value,
> +            gst_structure_copy (pad->priv->converter_config));
> 
> Or just g_value_set_boxed(value, config)? That copies internally then. Less
> noise
> 

Indeed, fixed

> @@ +592,3 @@
> +  /* TODO: handle different rates on sinkpads, a bit complex
> +   * because offsets will have to be updated, and audio resampling
> +   * has a latency to take into account
> 
> You should prevent that in get_caps() then if it's not supported yet
> (ignoring the race condition between getcaps and setcaps).
> 

Yeah, I'm doing that already actually :)

> ::: gst-libs/gst/audio/gstaudioaggregator.h
> @@ +168,3 @@
>  
> +GST_EXPORT
> +void gst_audio_aggregator_class_perform_conversion (GstAudioAggregatorClass
> * klass);
> 
> Should probably take a boolean so sub-subclasses can override it

Hm I see, do you figure that's an actual use case? I mean we do have liveadder, but afaiu it's mostly for backwards compatibility, I don't figure we actually want to extend the inheritance tree much further, it's already pretty deep as it is? The lack of a boolean was actually on purpose, that's also why I renamed that method from set_convertible to perform_conversion.

Related to the point above however, we may want to accept a static pad template here, to make it clearer for the subclass how to restrict the input caps, this could eventually be used by a sub-subclass to effectively disable conversion.

> , and maybe a vfunc that does the conversion and can be overridden?

That would be nice, however I would not add that preemptively, but wait till we have an actual use case to test the API with?

> ::: gst/audiomixer/gstaudiomixer.h
> @@ -53,3 @@
> -
> -  /* target caps (set via property) */
> -  GstCaps *filter_caps;
> 
> \o/ It's gone finally

Yeah, we need to be careful when we merge that however because GES uses it, I figured while we were still in bad we could break API and get rid of this :)
Comment 8 Mathieu Duponchelle 2017-09-22 12:15:22 UTC
Created attachment 360255 [details] [review]
audioaggregator: implement input conversion
Comment 9 Mathieu Duponchelle 2017-09-22 12:17:55 UTC
Updated the patch, changes:

AudioConverter only supports interleaved input / output, this means audiomixer can no longer handle non-interleaved data, is this acceptable?

Fixed a potential issue in setcaps if downstream caps were empty

Use g_value_set_boxed
Comment 10 Sebastian Dröge (slomo) 2017-09-22 14:33:57 UTC
(In reply to Mathieu Duponchelle from comment #7)
> (In reply to Sebastian Dröge (slomo) from comment #6)
> > Review of attachment 360227 [details] [review] [review] [review]:
> > 
> > ::: gst-libs/gst/audio/gstaudioaggregator.c
> > @@ +57,3 @@
> > +    GST_PAD_SINK,
> > +    GST_PAD_REQUEST,
> > +    SINK_CAPS);
> > 
> > Subclasses might want to limit the caps in one way or another. E.g. only
> > allow mono inputs.
> > 
> 
> They can do that by overriding the static template after calling
> perform_conversion , does that work for you ?

That's a bit ugly IMHO :) I'd make the pad template addition explicit in any case.

> > ::: gst-libs/gst/audio/gstaudioaggregator.h
> > @@ +168,3 @@
> >  
> > +GST_EXPORT
> > +void gst_audio_aggregator_class_perform_conversion (GstAudioAggregatorClass
> > * klass);
> > 
> > Should probably take a boolean so sub-subclasses can override it
> 
> Hm I see, do you figure that's an actual use case? I mean we do have
> liveadder, but afaiu it's mostly for backwards compatibility, I don't figure
> we actually want to extend the inheritance tree much further, it's already
> pretty deep as it is? The lack of a boolean was actually on purpose, that's
> also why I renamed that method from set_convertible to perform_conversion.
> 
> Related to the point above however, we may want to accept a static pad
> template here, to make it clearer for the subclass how to restrict the input
> caps, this could eventually be used by a sub-subclass to effectively disable
> conversion.

Consistency, in other base classes we also do these things with a boolean setting instead. Does it cause problems?

> > , and maybe a vfunc that does the conversion and can be overridden?

You might want to override the conversion if your internal code can already handle different formats for example, or if you use an external library that does that or has conversion.

In videomixer I added a vfunc IIRC, or someone, and it was needed. Adding it later always makes us lose some padding in the struct :)
Comment 11 Sebastian Dröge (slomo) 2017-09-22 14:34:52 UTC
(In reply to Mathieu Duponchelle from comment #9)
> Updated the patch, changes:
> 
> AudioConverter only supports interleaved input / output, this means
> audiomixer can no longer handle non-interleaved data, is this acceptable?

Yes, and we should just add interleaved support to audioconvert really. Internally it already supports that IIRC.
Comment 12 Mathieu Duponchelle 2017-10-19 16:14:09 UTC
Created attachment 361888 [details] [review]
audioaggregator: implement input conversion
Comment 13 Mathieu Duponchelle 2017-10-19 16:22:41 UTC
Updated the patch, changes are:

perform_conversion is gone, instead a single convert_buffer vmethod is exposed in GstAudioAggregator, if implemented we assume during caps negotiation that conversion will happen.

Conversion is now opt-out.

GstAudioAggregator no longer adds the sink template, this was indeed a bit ugly.

A new pad subclass is exposed, if the subclass uses (a subclass) of that new class as its sinkpad_type, input buffer converters will be cached instead of being recreated at each convert_buffer call.
Comment 14 Tim-Philipp Müller 2017-12-08 17:50:29 UTC
If thera re no further reviews/comments, we should get this in in order to allow for some testing in the wild.
Comment 15 Mathieu Duponchelle 2017-12-19 22:41:56 UTC
Review of attachment 361888 [details] [review]:

git bz errored out, that's ominous
Comment 16 Mathieu Duponchelle 2017-12-19 22:42:24 UTC
Well it's in, let's hope not everything crashes and burns
Comment 17 Stefan Sauer (gstreamer, gtkdoc dev) 2017-12-26 11:05:55 UTC
As discussed during the hackfest, these things where meant to be supposed to be resolved before moving:
@@ -1858,12 +1872,13 @@ gst_aggregator_query_latency_unlocked (GstAggregator * self, GstQuery * query)
 
   gst_query_parse_latency (query, &live, &min, &max);
 
+  // FIXME: this is already done by gst_query_set_latency()
   if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (min))) {
     GST_ERROR_OBJECT (self, "Invalid minimum latency %" GST_TIME_FORMAT
         ". Please file a bug at " PACKAGE_BUGREPORT ".", GST_TIME_ARGS (min));
     return FALSE;
   }
-
+  // FIXME: should this be done by gst_query_set_latency() too?
   if (min > max && GST_CLOCK_TIME_IS_VALID (max)) {
     GST_ELEMENT_WARNING (self, CORE, CLOCK, (NULL),
         ("Impossible to configure latency: max %" GST_TIME_FORMAT " < min %"
@@ -2001,6 +2016,7 @@ gst_aggregator_default_src_query (GstAggregator * self, GstQuery * query)
 
       /* don't pass it along as some (file)sink might claim it does
        * whereas with a collectpads in between that will not likely work */
+      // FIXME: wat?    ^^^^^^^^^^^
       gst_query_parse_seeking (query, &format, NULL, NULL, NULL);
       gst_query_set_seeking (query, format, FALSE, 0, -1);
       res = TRUE;


So please exterminate all mentions of collectpads. Those are leftovers, since the initial code started as a close of collectpads.
Comment 18 Mathieu Duponchelle 2017-12-26 14:31:31 UTC
Wrong bug ?

https://bugzilla.gnome.org/show_bug.cgi?id=739010 is probably the one you wanted to comment on :)
Comment 19 Stefan Sauer (gstreamer, gtkdoc dev) 2017-12-26 20:17:07 UTC
Sorry, had too many tabs open. Added the comment to the right bug.
Comment 20 Tim-Philipp Müller 2018-01-27 11:31:16 UTC
*** Bug 773762 has been marked as a duplicate of this bug. ***