GNOME Bugzilla – Bug 786344
audioaggregator: implement input conversion
Last modified: 2018-01-27 11:31:16 UTC
See the documentation for more details :)
Created attachment 357684 [details] [review] audioaggregator: implement input conversion
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 ?
(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.
Created attachment 360227 [details] [review] audioaggregator: implement input conversion
(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 ;)
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
(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 :)
Created attachment 360255 [details] [review] audioaggregator: implement input conversion
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
(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 :)
(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.
Created attachment 361888 [details] [review] audioaggregator: implement input conversion
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.
If thera re no further reviews/comments, we should get this in in order to allow for some testing in the wild.
Review of attachment 361888 [details] [review]: git bz errored out, that's ominous
Well it's in, let's hope not everything crashes and burns
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.
Wrong bug ? https://bugzilla.gnome.org/show_bug.cgi?id=739010 is probably the one you wanted to comment on :)
Sorry, had too many tabs open. Added the comment to the right bug.
*** Bug 773762 has been marked as a duplicate of this bug. ***