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 740236 - New audiointerleave based on GstAggregator and create GstAudioAggregator from audiomixer
New audiointerleave based on GstAggregator and create GstAudioAggregator from...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: High critical
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-11-17 05:09 UTC by Olivier Crête
Modified: 2015-03-16 20:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
aggregator: Add convenient gst_aggregator_set_latency() (3.49 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
reviewed Details | Review
aggregator: Handle EOS in the base class (4.42 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
rejected Details | Review
aggregator: Don't loop forever if all pads are unresponsive (2.80 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Split base class from audiomixer (86.93 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
reviewed Details | Review
audioaggregator: Don't modify size on early buffer (1.11 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Use the input caps when manipulating the input buffer (4.46 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Reply to the position query in bytes too (1.16 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
reviewed Details | Review
audioaggregator: Put GAP flag on output if all inputs are GAP data (4.70 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
reviewed Details | Review
audioaggregator: Only try to clip buffer if the incoming segment is in time or samples (1.06 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Use incoming segment with incoming timestamp (1.62 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Handle non-time segments and NONE timestamps (2.25 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Don't reset the position when pushing out new caps (1.23 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Make a number of member variables private (17.43 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Correctly handle case where no pad has a buffer (2.65 KB, patch)
2014-11-24 17:50 UTC, Olivier Crête
none Details | Review
audioaggregator: Ensure proper locking (10.86 KB, patch)
2014-11-24 17:51 UTC, Olivier Crête
none Details | Review
audioaggregator: Don't re-send the caps if they did not change (1.30 KB, patch)
2014-11-24 17:51 UTC, Olivier Crête
none Details | Review
audioaggregator: Set latency on base class (1.36 KB, patch)
2014-11-24 17:51 UTC, Olivier Crête
none Details | Review
audioaggregator: Reject caps with no channels (828 bytes, patch)
2014-11-24 17:51 UTC, Olivier Crête
none Details | Review
interleave2: Add interleave element based on audioaggregator (32.99 KB, patch)
2014-11-24 17:51 UTC, Olivier Crête
none Details | Review
interleave2: Add unit tests (30.80 KB, patch)
2014-11-24 17:51 UTC, Olivier Crête
none Details | Review
interleave2: Add interleave element based on audioaggregator (33.01 KB, patch)
2014-12-02 21:35 UTC, Olivier Crête
none Details | Review
audioaggregator: Split base class from audiomixer (94.38 KB, patch)
2015-02-20 23:47 UTC, Olivier Crête
committed Details | Review
audioaggregator: Don't re-send the caps if they did not change (1.30 KB, patch)
2015-02-20 23:48 UTC, Olivier Crête
committed Details | Review
audioaggregator: Reject caps with no channels (828 bytes, patch)
2015-02-20 23:49 UTC, Olivier Crête
rejected Details | Review
interleave2: Add interleave element based on audioaggregator (32.96 KB, patch)
2015-02-20 23:49 UTC, Olivier Crête
committed Details | Review
interleave2: Add unit tests (30.80 KB, patch)
2015-02-20 23:49 UTC, Olivier Crête
committed Details | Review

Description Olivier Crête 2014-11-17 05:09:57 UTC
First, I've split audiomixer into a GstAudioAggregator.

I've left the patches quite split up to ease review, but I would squash all of the "split audiomixer into a baseclass" patches into one before merging.

I've then added an interleave2 element based on the "interleave" element, I did my best to keep the same behavior/API, down to not fixing bug #708988 as I'm not sure how to do it without breaking API. So that when GstAggregator moves into core, we can replace "interleave" with interleave2 directly. Although the addition of a task on the srcpad does change the behaviour a bit.

I've included an only sligthly modified version of the "interleave" unit tests, they all pass. I've only made a gst-validate test based on Thibault's branch which has audiomixer tests. Both still pass.


Improvements in GstAudioAggregator over the current audiomixer code:
- Put the GAP flag on buffers created solely from GAP buffers
- Drop (don't push) buffers where nothing was mixed into, ideally, we'd push a GAP event instead, but for now we get a discont on the next buffer.
- Handle incoming streams with non-TIME segments (interleave did it, just ignore sync)
- Handle incoming buffers with GST_CLOCK_TIME_NONE (interleave worked fine, also ignore sync)
- Implemented latency query response (it increases the pipeline latency by up to "blocksize" samples).

Improvements to GstAggregator:

- added a gst_aggregator_set_latency() for the sub classes' benefit.
- handle EOS in the base class, remove hack where where the subclass would tell the base class about getting to eos by overloading the meaning of GST_FLOW_EOS.

Open questions:
- We should probably rename GstAudioAggregator and GstVideoAggregator into GstRaw...Aggregator as they only handle raw and raw-like formats.
- The various members of GstAggregator have unclear locking rules, in particular, it's not clear what the rules are for the "segment" member.
- 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).
- Handling of incoming GAP events (they're currently ignored).

git branch:
http://cgit.collabora.com/git/user/tester/gst-plugins-bad.git/log/?h=audio-aggregator

validate git branch:
http://cgit.collabora.com/git/user/tester/gst-devtools.git/log/?h=audiomixer
Comment 1 Thibault Saunier 2014-11-22 15:06:29 UTC
@ocrete thos test have been merged in gst-validate master branch
Comment 2 Thibault Saunier 2014-11-22 15:07:55 UTC
I have the impression it would be simpler to review if you just merge your patches together and propose here for review?
Comment 3 Sebastian Dröge (slomo) 2014-11-24 10:59:08 UTC
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! :)
Comment 4 Olivier Crête 2014-11-24 17:49:38 UTC
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.
Comment 5 Olivier Crête 2014-11-24 17:50:01 UTC
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.
Comment 6 Olivier Crête 2014-11-24 17:50:05 UTC
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
Comment 7 Olivier Crête 2014-11-24 17:50:08 UTC
Created attachment 291389 [details] [review]
aggregator: Don't loop forever if all pads are unresponsive
Comment 8 Olivier Crête 2014-11-24 17:50:13 UTC
Created attachment 291390 [details] [review]
audioaggregator: Split base class from audiomixer
Comment 9 Olivier Crête 2014-11-24 17:50:17 UTC
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.
Comment 10 Olivier Crête 2014-11-24 17:50:21 UTC
Created attachment 291392 [details] [review]
audioaggregator: Use the input caps when manipulating the input buffer

Also store in in the sink pad
Comment 11 Olivier Crête 2014-11-24 17:50:24 UTC
Created attachment 291393 [details] [review]
audioaggregator: Reply to the position query in bytes too
Comment 12 Olivier Crête 2014-11-24 17:50:29 UTC
Created attachment 291394 [details] [review]
audioaggregator: Put GAP flag on output if all inputs are GAP data
Comment 13 Olivier Crête 2014-11-24 17:50:34 UTC
Created attachment 291395 [details] [review]
audioaggregator: Only try to clip buffer if the incoming segment is in time or samples
Comment 14 Olivier Crête 2014-11-24 17:50:38 UTC
Created attachment 291396 [details] [review]
audioaggregator: Use incoming segment with incoming timestamp
Comment 15 Olivier Crête 2014-11-24 17:50:42 UTC
Created attachment 291397 [details] [review]
audioaggregator: Handle non-time segments and NONE timestamps
Comment 16 Olivier Crête 2014-11-24 17:50:46 UTC
Created attachment 291398 [details] [review]
audioaggregator: Don't reset the position when pushing out new caps
Comment 17 Olivier Crête 2014-11-24 17:50:51 UTC
Created attachment 291399 [details] [review]
audioaggregator: Make a number of member variables private
Comment 18 Olivier Crête 2014-11-24 17:50:55 UTC
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.
Comment 19 Olivier Crête 2014-11-24 17:51:00 UTC
Created attachment 291401 [details] [review]
audioaggregator: Ensure proper locking
Comment 20 Olivier Crête 2014-11-24 17:51:04 UTC
Created attachment 291402 [details] [review]
audioaggregator: Don't re-send the caps if they did not change
Comment 21 Olivier Crête 2014-11-24 17:51:09 UTC
Created attachment 291403 [details] [review]
audioaggregator: Set latency on base class
Comment 22 Olivier Crête 2014-11-24 17:51:13 UTC
Created attachment 291404 [details] [review]
audioaggregator: Reject caps with no channels
Comment 23 Olivier Crête 2014-11-24 17:51:18 UTC
Created attachment 291405 [details] [review]
interleave2: Add interleave element based on audioaggregator
Comment 24 Olivier Crête 2014-11-24 17:51:23 UTC
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.
Comment 25 Tim-Philipp Müller 2014-11-24 18:13:29 UTC
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.
Comment 26 Thibault Saunier 2014-11-25 20:53:02 UTC
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.
Comment 27 Thibault Saunier 2014-11-25 21:08:01 UTC
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.
Comment 28 Thibault Saunier 2014-11-25 21:08:01 UTC
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.
Comment 29 Thibault Saunier 2014-11-25 21:12:56 UTC
Review of attachment 291389 [details] [review]:

I am not sure I understand what the behaviour is after that patch?
Comment 30 Thibault Saunier 2014-11-25 21:19:12 UTC
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 :)
Comment 31 Thibault Saunier 2014-11-25 21:29:47 UTC
Review of attachment 291393 [details] [review]:

I am not sure we are supposed to answer to BYTE position with decoded data like that.
Comment 32 Thibault Saunier 2014-11-25 21:30:42 UTC
Review of attachment 291394 [details] [review]:

Looks OK
Comment 33 Thibault Saunier 2014-11-25 21:34:43 UTC
Review of attachment 291395 [details] [review]:

Do we support other format types? And what would happen in the other cases?
Comment 34 Thibault Saunier 2014-11-25 21:54:44 UTC
> 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 :)
Comment 35 Mathieu Duponchelle 2014-11-25 22:20:00 UTC
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 :)
Comment 36 Olivier Crête 2014-11-26 21:20:12 UTC
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.
Comment 37 Thibault Saunier 2014-11-26 21:50:06 UTC
> 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.
Comment 38 Thibault Saunier 2014-11-26 22:01:12 UTC
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)
Comment 39 Thibault Saunier 2014-11-26 22:06:47 UTC
(and you probably wan to pass --mute, to $ gst-validate-launcher ges --sync --mute so that you can keep using your computer :))
Comment 40 Sebastian Dröge (slomo) 2014-11-27 09:30:03 UTC
(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).
Comment 41 Olivier Crête 2014-12-02 21:35:13 UTC
Created attachment 292028 [details] [review]
interleave2: Add interleave element based on audioaggregator

With one deadlock less than the previous version.
Comment 42 Olivier Crête 2014-12-02 21:53:17 UTC
(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.
Comment 43 Olivier Crête 2015-02-20 23:47:54 UTC
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
Comment 44 Olivier Crête 2015-02-20 23:48:10 UTC
Created attachment 297470 [details] [review]
audioaggregator: Don't re-send the caps if they did not change
Comment 45 Olivier Crête 2015-02-20 23:49:03 UTC
Created attachment 297471 [details] [review]
audioaggregator: Reject caps with no channels
Comment 46 Olivier Crête 2015-02-20 23:49:12 UTC
Created attachment 297472 [details] [review]
interleave2: Add interleave element based on audioaggregator
Comment 47 Olivier Crête 2015-02-20 23:49:25 UTC
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.
Comment 48 Sebastian Dröge (slomo) 2015-02-24 08:54:49 UTC
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?
Comment 49 Olivier Crête 2015-02-24 19:19:47 UTC
@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.
Comment 50 Sebastian Dröge (slomo) 2015-03-13 15:48:14 UTC
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()?
Comment 51 Sebastian Dröge (slomo) 2015-03-13 15:49:43 UTC
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.
Comment 52 Tim-Philipp Müller 2015-03-13 15:58:34 UTC
Also, how about calling them audiointerleave/deinterleave instead of deinterleave2 etc.?
Comment 53 Sebastian Dröge (slomo) 2015-03-13 16:01:39 UTC
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()?
Comment 54 Sebastian Dröge (slomo) 2015-03-13 16:04:18 UTC
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 ;)
Comment 55 Olivier Crête 2015-03-13 16:08:36 UTC
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.
Comment 56 Sebastian Dröge (slomo) 2015-03-13 16:11:47 UTC
(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
Comment 57 Sebastian Dröge (slomo) 2015-03-13 16:12:32 UTC
(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 :)
Comment 58 Tim-Philipp Müller 2015-03-13 16:17:16 UTC
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.
Comment 59 Olivier Crête 2015-03-14 20:07:13 UTC
(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.
Comment 60 Sebastian Dröge (slomo) 2015-03-15 12:20:09 UTC
(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.
Comment 61 Olivier Crête 2015-03-16 20:53:22 UTC
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