Simplifications of the mixing algorithm.

Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.

NOTRY=True

Committed: https://crrev.com/652ac89c09adba4be85a27b0be8c2fc2395d16f1
Cr-Commit-Position: refs/heads/master@{#14110}

[aleloi, 2016-08-31 11:22:31]

Patchset #2 (id:20001) has been deleted

[aleloi, 2016-08-31 11:22:40]

Patchset #1 (id:1) has been deleted

[aleloi, 2016-08-31 11:34:28]

aleloi@webrtc.org changed reviewers:
+ ivoc@webrtc.org, kwiberg@webrtc.org

[aleloi, 2016-08-31 11:34:29]

Have another one! It's part 2 of the original mess I sent out earlier. This CL
touches the structure of the mixing algorithm.

I've tried to make the anonymous / non-anonymous mixing more consistent.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:82: *frame >>= 1;
MixFrames is now done in MixFromList.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:428: continue;
Replaced this with a RTC_DCHECK_EQ in
MixFromList. Frames should never be empty.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:506: // TODO(andrew): consolidate
this function with MixFromList.
Done! It took only five years or so:)

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:49: 
Differs from the above constructor by not calculating energy. A structure with
source, frame and previous mixed status was needed for |Ramp()|. I reused
|SourceFrame|.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:94: }
Ramping in/out should be done both for anonymous and ordinary
participants. I broke it out to a function to avoid code duplication.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:358: std::vector<SourceFrame>
ramp_list;
See comment about 2:nd SourceFrame constructor.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (left):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.h:35: typedef
std::list<FrameAndMuteInfo> AudioFrameList;
Mixing structure is a little simplified. Mute information is only used
inside of Get*AnonymousAudio. Muted frames are filtered out one step
earlier. Therefore a |muted| field is no longer needed here, and the
FrameAndMuteInfo struct can be replaced by a single frame.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.h:103:
EXCLUSIVE_LOCKS_REQUIRED(crit_);
Made naming and signature more consistent between the Get*Audio
methods.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.h:128: 
Merged with MixFromList

[ivoc, 2016-08-31 15:01:06]

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:490: uint32_t position = 0;
It looks like this is not used anywhere, if so, please remove it.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:514: *frame >>= 1;
Should we try to come up with a better approach here (something less vulnerable
to overflow), or do we postpone that until we switch to float?

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (right):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.h:29: typedef std::list<AudioFrame*>
AudioFrameList;
I think list is in many cases less efficient than vector (mostly due to
caching), are we sure that list is the right choice here? If you're interested
there's an interesting talk by Bjarne Stroustrup on the topic:
https://www.youtube.com/watch?v=YQs6IC-vgmo

[aleloi, 2016-08-31 15:25:34]

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:490: uint32_t position = 0;
On 2016/08/31 15:01:06, ivoc wrote:
> It looks like this is not used anywhere, if so, please remove it.

Removed in dependent CL, https://codereview.webrtc.org/2294263002/

I tried to keep this minimal and only concern the mixing structure changes.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:514: *frame >>= 1;
On 2016/08/31 15:01:06, ivoc wrote:
> Should we try to come up with a better approach here (something less
vulnerable
> to overflow), or do we postpone that until we switch to float?

I think that was the decision. I am going to take a look at how the limiter
works in the coming weeks, and then probably get back to this.

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (right):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.h:29: typedef std::list<AudioFrame*>
AudioFrameList;
On 2016/08/31 15:01:06, ivoc wrote:
> I think list is in many cases less efficient than vector (mostly due to
> caching), are we sure that list is the right choice here? If you're interested
> there's an interesting talk by Bjarne Stroustrup on the topic:
> https://www.youtube.com/watch?v=YQs6IC-vgmo

Will watch!

I think the right way is to make a performance test with lots of participants
and see what performs better. In addition: our typical use case should be a
small number of participants, so we may want the structure that performs best at
small sizes.

The list has been here since the beginning. I thought it was because of the
removals in SetMixedStatus(..., false). But maybe vectors are better anyway.

[aleloi, 2016-08-31 15:42:31]

Patchset #2 (id:50001) has been deleted

[kwiberg-webrtc, 2016-09-01 11:43:46]

lgtm

https://codereview.webrtc.org/2298163002/diff/70001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2298163002/diff/70001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:437:
ramp_list.emplace_back((*audio_source), audio_frame, false,
Unnecessary parentheses.

[aleloi, 2016-09-01 13:20:43]

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (right):

https://codereview.webrtc.org/2298163002/diff/40001/webrtc/modules/audio_mixe...
webrtc/modules/audio_mixer/audio_mixer_impl.h:29: typedef std::list<AudioFrame*>
AudioFrameList;
On 2016/08/31 15:01:06, ivoc wrote:
> I think list is in many cases less efficient than vector (mostly due to
> caching), are we sure that list is the right choice here? If you're interested
> there's an interesting talk by Bjarne Stroustrup on the topic:
> https://www.youtube.com/watch?v=YQs6IC-vgmo

Got convinced by watching. Changed to vector.

[ivoc, 2016-09-01 14:10:48]

lgtm.

[aleloi, 2016-09-07 14:30:28]

Description was changed from

==========
Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.
==========

to

==========
Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.

NOTRY=True
==========

[aleloi, 2016-09-07 14:30:35]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-09-07 14:30:35]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2298163002/#ps190001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-09-07 14:30:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-07 14:42:17]

Description was changed from

==========
Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.

NOTRY=True
==========

to

==========
Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.

NOTRY=True
==========

[commit-bot: I haz the power, 2016-09-07 14:42:18]

Committed patchset #8 (id:190001)

[commit-bot: I haz the power, 2016-09-07 14:42:30]

Description was changed from

==========
Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.

NOTRY=True
==========

to

==========
Simplifications of the mixing algorithm.

Methods are named more consistently and have a more consistent
signatures. The call structure of mixing is slightly
simplified. Anonymous participants are also ramped up.

NOTRY=True

Committed: https://crrev.com/652ac89c09adba4be85a27b0be8c2fc2395d16f1
Cr-Commit-Position: refs/heads/master@{#14110}
==========

[commit-bot: I haz the power, 2016-09-07 14:42:31]

Patchset 8 (id:??) landed as
https://crrev.com/652ac89c09adba4be85a27b0be8c2fc2395d16f1
Cr-Commit-Position: refs/heads/master@{#14110}