@ocrete thos test have been merged in gst-validate master branch

I have the impression it would be simpler to review if you just merge your patches together and propose here for review?

Yes, go for it (in -bad). That way we can give it some more testing, and the general approach of getting it closer to audiomixer definitely makes sense and was on my list since a long time. Thanks for working on this! :)

Attaching the patches, there are 4 "groups":

1. GstAggregator improvements (3 patches)
2. Splitting GstAudioAggregator from GstAudioMixer (1 patch)
3. Improvements to GstAudioAggregator 
4. Add interleave2 based on it (the last 2 patches)

One thing I'm not super happy with, the GstAudioAggregator is holding it's object lock while calling the mix_one_buffer() vmethod.

Created attachment 291387 [details] [review]
aggregator: Add convenient gst_aggregator_set_latency()

This is to allow subclasses to tell the base class about
how much latency they add.

Created attachment 291388 [details] [review]
aggregator: Handle EOS in the base class

Remove the nasty hack where subclasses return GST_FLOW_EOS to mean
sending a EOS. Instead, just check if all pads have reached EOS and
have the base class produce it directly.aggregator: Handle EOS in the
base class

Created attachment 291389 [details] [review]
aggregator: Don't loop forever if all pads are unresponsive

Created attachment 291390 [details] [review]
audioaggregator: Split base class from audiomixer

Created attachment 291391 [details] [review]
audioaggregator: Don't modify size on early buffer

The size is the size of the buffer, not of remaining part.

Created attachment 291392 [details] [review]
audioaggregator: Use the input caps when manipulating the input buffer

Also store in in the sink pad

Created attachment 291393 [details] [review]
audioaggregator: Reply to the position query in bytes too

Created attachment 291394 [details] [review]
audioaggregator: Put GAP flag on output if all inputs are GAP data

Created attachment 291395 [details] [review]
audioaggregator: Only try to clip buffer if the incoming segment is in time or samples

Created attachment 291396 [details] [review]
audioaggregator: Use incoming segment with incoming timestamp

Created attachment 291397 [details] [review]
audioaggregator: Handle non-time segments and NONE timestamps

Created attachment 291398 [details] [review]
audioaggregator: Don't reset the position when pushing out new caps

Created attachment 291399 [details] [review]
audioaggregator: Make a number of member variables private

Created attachment 291400 [details] [review]
audioaggregator: Correctly handle case where no pad has a buffer

If none of the pads have buffers that can be handled, don't claim to be EOS.

Created attachment 291401 [details] [review]
audioaggregator: Ensure proper locking

Created attachment 291402 [details] [review]
audioaggregator: Don't re-send the caps if they did not change

Created attachment 291403 [details] [review]
audioaggregator: Set latency on base class

Created attachment 291404 [details] [review]
audioaggregator: Reject caps with no channels

Created attachment 291405 [details] [review]
interleave2: Add interleave element based on audioaggregator

Created attachment 291406 [details] [review]
interleave2: Add unit tests

Almost a copy of the "interleave" unit tests, improved to support
the thread on the src pad on GstAggregator.

Minor point, but I'm not sure if we should support input in non-TIME segments. You mentioned on IRC that interleave supports that, but I think it's crack to support that and better to error out and let people fix their stuff instead.

Review of attachment 291387 [details] [review]:

Looks good, just minor comments.

::: gst-libs/gst/base/gstaggregator.c
@@ +1024,3 @@
 
+  GST_OBJECT_LOCK (self);
+  data.min += self->priv->subclass_min_latency;

min latency can not be CLOCK_TIME_NONE then? What if subclass does not know but knows the max?

@@ +1881,3 @@
+
+/**
+ * gst_aggregator_set_latency:

Please add that to section.txt too.

Review of attachment 291388 [details] [review]:

I do not understand why you would say it is a hack at all? The subclass can have information about the fact it is EOS that baseclass does not know, and we need to let the subclass tell us if it is the case, for example here in videoaggregator, in the case we have output_end_time == output_start_time, we *know* at that point that we are EOS, we have not received any EOS event on the pad but we do not care at that point.

Review of attachment 291388 [details] [review]:

I do not understand why you would say it is a hack at all? The subclass can have information about the fact it is EOS that baseclass does not know, and we need to let the subclass tell us if it is the case, for example here in videoaggregator, in the case we have output_end_time == output_start_time, we *know* at that point that we are EOS, we have not received any EOS event on the pad but we do not care at that point.

Review of attachment 291389 [details] [review]:

I am not sure I understand what the behaviour is after that patch?

Review of attachment 291390 [details] [review]:

Looks generally good, have not gone into details though.

::: gst/audiomixer/gstaudioaggregator.h
@@ +66,3 @@
+ * GstAudioAggregatorPad:
+ * @buffer: currently queued buffer.
+ * @segment: last segment received.

Can't see those 2 fields here :)

Review of attachment 291393 [details] [review]:

I am not sure we are supposed to answer to BYTE position with decoded data like that.

Review of attachment 291394 [details] [review]:

Looks OK

Review of attachment 291395 [details] [review]:

Do we support other format types? And what would happen in the other cases?

> Open questions:
> - We should probably rename GstAudioAggregator and GstVideoAggregator into
> GstRaw...Aggregator as they only handle raw and raw-like formats.

I do not expect to have any subclass working handling encoded audio/video soon, I would just keep it short and make it a GstEncodedVideoAggregator in the case it is needed at some point. 

> - The various members of GstAggregator have unclear locking rules, in
> particular, it's not clear what the rules are for the "segment" member.

I will go over them and document that, it is indeed clearly missing documentation, and now I am not sure how segment is supposed to be protected.

> - I'm unhappy about how the "unresponsive" pads are handled (but I made that
> argument on bug #739010 already)
> - The .._set_src_pad() delaying should probably happen in the base class (which
> should handle the mecanism of bufferpool and allocator negotiation), this would
> make it easier for GstAudioAggregator to use a downstream GstAllocator if
> possible (for example, when using shmsink).

That would be a nice feature to add indeed :)

Great work you've put in, I haven't reviewed the patch set yet but going in that direction makes me happy :)

> Open questions:
> - We should probably rename GstAudioAggregator and GstVideoAggregator into
> GstRaw...Aggregator as they only handle raw and raw-like formats.


I don't think we want to do that, we don't prefix with raw* everything that deals with raw formats :)

First, thanks for your thoughtful review

(In reply to comment #26)
> Review of attachment 291387 [details] [review]:
> ::: gst-libs/gst/base/gstaggregator.c
> @@ +1024,3 @@
> 
> +  GST_OBJECT_LOCK (self);
> +  data.min += self->priv->subclass_min_latency;
> 
> min latency can not be CLOCK_TIME_NONE then? What if subclass does not know but
> knows the max?

I think if you don't know, then we've lost, live pipelines (with live src & live sink) can't really work without a known latency, so you may as well return 0. For the max, GST_CLOCK_TIME_NONE is really GST_CLOCK_TIME_MAX, ie, it has no limit on how much it can buffer.

(In reply to comment #28)
> Review of attachment 291388 [details] [review]:
> 
> I do not understand why you would say it is a hack at all? The subclass can
> have information about the fact it is EOS that baseclass does not know, and we
> need to let the subclass tell us if it is the case, for example here in
> videoaggregator, in the case we have output_end_time == output_start_time, we
> *know* at that point that we are EOS, we have not received any EOS event on the
> pad but we do not care at that point.

Normally, returning GST_FLOW_EOS means that you tried to push a buffer after a pad has reached eos. Ie, it is a fatal error resulting from a programming error. I guess we could add a gst_aggregator_push_eos() for that case. Although it does seem a strange one.


(In reply to comment #29)
> Review of attachment 291389 [details] [review]:
> 
> I am not sure I understand what the behaviour is after that patch?

Before this patch, it all pads go unresponsive, GstVideoAggregator based elements would call the aggregate function in a loop forever (and AudioAggregator just sent EOS). After the patch, the aggregate function will be called once (which allows the subclass to send downstream any data it still has), then it will wait for another buffer to arrive.

(In reply to comment #31)
> Review of attachment 291393 [details] [review]:
> 
> I am not sure we are supposed to answer to BYTE position with decoded data like
> that.

This is directly taken from "interleave", I assume it had the right behavior.

(In reply to comment #33)
> Review of attachment 291395 [details] [review]:
> 
> Do we support other format types? And what would happen in the other cases?

In the other format types, you just can't clip, and you can't really synchronize either.

> Normally, returning GST_FLOW_EOS means that you tried to push a buffer after a
> pad has reached eos. Ie, it is a fatal error resulting from a programming
> error. I guess we could add a gst_aggregator_push_eos() for that case. Although
> it does seem a strange one.

That case does happen, and I think using GST_FLOW_EOS to tell subclass that we are EOS is legit.

And thanks for those patches! I will try to keep reviewing the remaining one soon (no courage right now! :))

Also, it would be very nice if you could also run the gst-validate testsuite for GES.

For that you will just need to run:

$ gst-validate-launcher ges --sync
 
I tried yesterday and we can see that this patchset introduces regressions in that testsuite.

(Also you will need to get latest gst-uninstalled, I just pushed a patch to make sure the GES testsuite is avalaible by default in that env)

(and you probably wan to pass --mute, to $ gst-validate-launcher ges --sync --mute so that you can keep using your computer :))

(In reply to comment #36)

> > Review of attachment 291388 [details] [review] [details]:
> > 
> > I do not understand why you would say it is a hack at all? The subclass can
> > have information about the fact it is EOS that baseclass does not know, and we
> > need to let the subclass tell us if it is the case, for example here in
> > videoaggregator, in the case we have output_end_time == output_start_time, we
> > *know* at that point that we are EOS, we have not received any EOS event on the
> > pad but we do not care at that point.
> 
> Normally, returning GST_FLOW_EOS means that you tried to push a buffer after a
> pad has reached eos. Ie, it is a fatal error resulting from a programming
> error. I guess we could add a gst_aggregator_push_eos() for that case. Although
> it does seem a strange one.

This is not true. It can be an error, but it can also be completely expected behaviour. Downstream can return EOS when it decides that it is at the end of stream now. Upstream might not know that this is the case, e.g. because downstream has a different segment configured or because downstream has additional knowledge about when the stream ends (think container format with a known byte size and then a lot of garbage at the end after the container).

Created attachment 292028 [details] [review]
interleave2: Add interleave element based on audioaggregator

With one deadlock less than the previous version.

(In reply to comment #40)
> This is not true. It can be an error, but it can also be completely expected
> behaviour. Downstream can return EOS when it decides that it is at the end of
> stream now. Upstream might not know that this is the case, e.g. because
> downstream has a different segment configured or because downstream has
> additional knowledge about when the stream ends (think container format with a
> known byte size and then a lot of garbage at the end after the container).

Thanks for the information, I didn't know that.. I updated my git branch to do both, so handle eos from the base class if all pads have received EOS and have no pending data, but also expect GST_FLOW_EOS from the subclass and push the EOS event downstream in this case.

I also fixed the documentation mixups, next is figuring out why the GES regressions.

Created attachment 297469 [details] [review]
audioaggregator: Split base class from audiomixer

Merged most of the patches splitting an GstAudioAggregator base class
from the audiomixer element.

I have only one problem with the current code, the GstAudioAggregator
holds the object locks of both the GstElement and the GstPad when
calling "aggregate_one_buffer", it makes the implementation of the
GstAudioAggretor much easier, as it means pads can't appear or
disappear, but it's a bit odd for a base class.

I ran lots and lots of iterations of unit tests and validate over this.


Also:
-  Don't modify size on early buffer
   The size is the size of the buffer, not of remaining part.
- Use the input caps when manipulating the input buffer
   Also store in in the sink pad
- Reply to the position query in bytes too
- Put GAP flag on output if all inputs are GAP data
- Only try to clip buffer if the incoming segment is in time or samples
- Use incoming segment with incoming timestamp
   Handle non-time segments and NONE timestamps
- Don't reset the position when pushing out new caps
- Make a number of member variables private
- Correctly handle case where no pad has a buffer
  If none of the pads have buffers that can be handled, don't claim to be EOS.
- Ensure proper locking

Created attachment 297470 [details] [review]
audioaggregator: Don't re-send the caps if they did not change

Created attachment 297471 [details] [review]
audioaggregator: Reject caps with no channels

Created attachment 297472 [details] [review]
interleave2: Add interleave element based on audioaggregator

Created attachment 297473 [details] [review]
interleave2: Add unit tests

Almost a copy of the "interleave" unit tests, improved to support
the thread on the src pad on GstAggregator.

It sounds like some of the items in your "Also:" list were recently fixed in audiomixer already. When updating the patch 4 days ago, did you rebase everything against all the changes done to audiomixer recently?

@slomo: Yes, I "swallowed" all of the recent patches, most of the code in GstAudioAggregator is as-is from audiomixer. That said, I'd greatly appreciate a review.

Review of attachment 297470 [details] [review]:

::: gst/audiomixer/gstaudioaggregator.c
@@ +617,3 @@
   GST_OBJECT_LOCK (aagg);
 
+  if (memcmp (&info, &aagg->info, sizeof (info))) {

Why not gst_audio_info_is_equal()?

Review of attachment 297471 [details] [review]:

::: gst/audiomixer/gstaudioaggregator.c
@@ +614,3 @@
   }
 
+  g_return_val_if_fail (info.channels > 0, FALSE);

Shouldn't gst_audio_info_from_caps() just fail for incomplete caps? The channels field is not optional! And the code also looks like it would.

Also, how about calling them audiointerleave/deinterleave instead of deinterleave2 etc.?

Review of attachment 297469 [details] [review]:

Needs to be resynced against latest audiomixer, but generally looks good. Please push afterwards :)

::: gst/audiomixer/gstaudioaggregator.c
@@ +699,3 @@
+  GST_OBJECT_LOCK (bpad);
+  if (bpad->segment.format == GST_FORMAT_TIME ||
+      bpad->segment.format == GST_FORMAT_DEFAULT)

I don't think we should work at all in non-TIME/DEFAULT segment formats

@@ +711,3 @@
+ */
+static gboolean
+gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,

Please check this complete function against the current audiomixer code again :)

@@ +731,3 @@
+
+  if (!GST_BUFFER_PTS_IS_VALID (inbuf) ||
+      aggpad->segment.format != GST_FORMAT_TIME) {

We should require properly timestamps input buffers

@@ +880,3 @@
+
+static void
+gst_audio_aggregator_mix_buffer (GstAudioAggregator * aagg,

And also this function should be compared against the latest audiomixer code

::: gst/audiomixer/gstaudioaggregator.h
@@ +161,3 @@
+void
+gst_audio_aggregator_set_sink_caps (GstAudioAggregator * aagg,
+    GstAudioAggregatorPad * pad, GstCaps * caps);

Why should subclasses be able to set caps on the sinkpad?

::: gst/audiomixer/gstaudiomixer.c
@@ +393,2 @@
+  if (ret)
+    gst_audio_aggregator_set_sink_caps (aagg, GST_AUDIO_AGGREGATOR_PAD (pad),

Why? Shouldn't that just be the default behaviour for setcaps()?

Review of attachment 297472 [details] [review]:

I don't like the name interleave2 :) Maybe we can call it audiointerleave, and then also register deinterleave with the name "audiodeinterleave" and then deprecate old "interleave"?

Otherwise seems good to go :)

::: gst/audiomixer/gstinterleave2.c
@@ +4,3 @@
+ *                    2005 Wim Taymans <wim@fluendo.com>
+ *                    2007 Andy Wingo <wingo at pobox.com>
+ *                    2008 Sebastian Dröge <slomo@circular-chaos.rg>

Typo ;)

I'll merge the new audiomixer changes in and update the patch, also based on your comments, thank you for the review

(In reply to Tim-Philipp Müller from comment #52)
> Also, how about calling them audiointerleave/deinterleave instead of
> deinterleave2 etc.?

The plan was to rename it to interleave once aggregator/audioaggregator go into -base and we can replace the existing interleave which is in good.

(In reply to Sebastian Dröge (slomo) from comment #53)
> I don't think we should work at all in non-TIME/DEFAULT segment formats

> We should require properly timestamps input buffers

The current "interleave" element works fine with byte segments.

(In reply to Sebastian Dröge (slomo) from comment #53)
> @@ +393,2 @@
> +  if (ret)
> +    gst_audio_aggregator_set_sink_caps (aagg, GST_AUDIO_AGGREGATOR_PAD
> (pad),
> 
> Why? Shouldn't that just be the default behaviour for setcaps()?

This probably should just be done like in videoaggregator

(In reply to Olivier Crête from comment #55)

> (In reply to Sebastian Dröge (slomo) from comment #53)
> > I don't think we should work at all in non-TIME/DEFAULT segment formats
> 
> > We should require properly timestamps input buffers
> 
> The current "interleave" element works fine with byte segments.

Ok, but that doesn't seem very useful to support and just makes everything more complex :)

I don't see a valid use case for supporting byte segments in the current interleave even. That's just wrong. We should just remove that behaviour and error out instead.

(In reply to Sebastian Dröge (slomo) from comment #53)
> @@ +711,3 @@
> + */
> +static gboolean
> +gst_audio_aggregator_fill_buffer (GstAudioAggregator * aagg,
> 
> Please check this complete function against the current audiomixer code
> again :)
> 
> @@ +731,3 @@
> +
> +  if (!GST_BUFFER_PTS_IS_VALID (inbuf) ||
> +      aggpad->segment.format != GST_FORMAT_TIME) {
> 
> We should require properly timestamps input buffers

In live pipeline, it's convenient to be able to send out buffers with a GST_CLOCK_TIME_NONE pts, to be mixed asap, for thing like sound events and stuff.
 

> ::: gst/audiomixer/gstaudioaggregator.h
> @@ +161,3 @@
> +void
> +gst_audio_aggregator_set_sink_caps (GstAudioAggregator * aagg,
> +    GstAudioAggregatorPad * pad, GstCaps * caps);
> 
> Why should subclasses be able to set caps on the sinkpad?
> 
> ::: gst/audiomixer/gstaudiomixer.c
> @@ +393,2 @@
> +  if (ret)
> +    gst_audio_aggregator_set_sink_caps (aagg, GST_AUDIO_AGGREGATOR_PAD
> (pad),
> 
> Why? Shouldn't that just be the default behaviour for setcaps()?

The base class doesn't have a setcaps at all, it just lets the subclass handle the caps event.

(In reply to Olivier Crête from comment #59)
> > @@ +731,3 @@
> > +
> > +  if (!GST_BUFFER_PTS_IS_VALID (inbuf) ||
> > +      aggpad->segment.format != GST_FORMAT_TIME) {
> > 
> > We should require properly timestamps input buffers
> 
> In live pipeline, it's convenient to be able to send out buffers with a
> GST_CLOCK_TIME_NONE pts, to be mixed asap, for thing like sound events and
> stuff.

The meaning being that it perfectly aligns with the samples of the previous buffers? Sounds reasonable, yes.

But don't support non-TIME segments please :)

> > ::: gst/audiomixer/gstaudioaggregator.h
> > ::: gst/audiomixer/gstaudiomixer.c
> > @@ +393,2 @@
> > +  if (ret)
> > +    gst_audio_aggregator_set_sink_caps (aagg, GST_AUDIO_AGGREGATOR_PAD
> > (pad),
> > 
> > Why? Shouldn't that just be the default behaviour for setcaps()?
> 
> The base class doesn't have a setcaps at all, it just lets the subclass
> handle the caps event.

I see. Maybe that should be changed then :) Looking at videoaggregator, it is also a bit special there. We should make it sensible in both and also the same way. But don't block merging this patch on that, let's just get it in before more changes are added to audiomixer.

Committed everything here! Let's hope I didn't break stuff!

Note that I changed pad->position/pad->size to be in frames (sample size * num channels) instead of bytes to have the same unit everywhere.

commit 84eff5cca93b669a29acbd4db57a79ca4e911dff
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Thu Nov 13 20:39:11 2014 -0500

    audiointerleave: Add unit tests
    
    Almost a copy of the "interleave" unit tests, improved to support
    the thread on the src pad on GstAggregator.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740236

commit 224f14a299ed5cae77354c41dc16d3f2d9234f6d
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Fri Mar 6 13:49:48 2015 -0500

    audiointerleave: Set src caps in aggregate
    
    This prevents races between the setcaps of the sink pads
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740236

commit b08b01895fc0606fec7477084332130f12345996
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Thu Nov 13 15:40:15 2014 -0500

    audiointerleave: Add interleave element based on audioaggregator
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740236

commit c56587799124bd37638ddbfeffdfb9dff9b61fe8
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Fri Mar 6 16:51:12 2015 -0500

    audioaggregator: Print a message when a buffer is late
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740236

commit 01520c7e4781c3d5bff3d1b2a5e123ad085e4bd7
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Sat Nov 15 17:54:51 2014 -0500

    audioaggregator: Don't re-send the caps if they did not change
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740236

commit 959f8e4a3e57c0185a3fa826efffb6065ae7e080
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Thu Nov 6 17:15:17 2014 -0500

    audioaggregator: Split base class from audiomixer
    
    Also:
    -  Don't modify size on early buffer
       The size is the size of the buffer, not of remaining part.
    - Use the input caps when manipulating the input buffer
       Also store in in the sink pad
    - Reply to the position query in bytes too
    - Put GAP flag on output if all inputs are GAP data
    - Only try to clip buffer if the incoming segment is in time or samples
    - Use incoming segment with incoming timestamp
       Handle non-time segments and NONE timestamps
    - Don't reset the position when pushing out new caps
    - Make a number of member variables private
    - Correctly handle case where no pad has a buffer
      If none of the pads have buffers that can be handled, don't claim to be EOS.
    - Ensure proper locking
    - Only support time segments
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740236