Moved MixerAudioSource and removed audio_mixer_defines.h.

Moved MixerAudioSource and removed audio_mixer_defines.h.

MixerAudioSource is moved to AudioMixerImpl::Source. Structures and methods of the MixerAudioSource interface have been renamed. The RemixFrame method has added checks and is moved to audio_frame_manipulator.h

BUG=webrtc:6346
Committed: https://crrev.com/e89141500a7375f7eba7814dd88b690bf4135223
Cr-Commit-Position: refs/heads/master@{#14600}

[aleloi, 2016-10-06 12:30:50]

Patchset #1 (id:1) has been deleted

[aleloi, 2016-10-06 13:15:47]

Patchset #1 (id:20001) has been deleted

[aleloi, 2016-10-06 13:15:54]

Patchset #1 (id:40001) has been deleted

[aleloi, 2016-10-07 08:53:39]

Patchset #1 (id:60001) has been deleted

[aleloi, 2016-10-07 12:33:49]

Patchset #1 (id:80001) has been deleted

[aleloi, 2016-10-07 12:37:07]

Patchset #1 (id:100001) has been deleted

[aleloi, 2016-10-07 12:59:20]

aleloi@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[aleloi, 2016-10-07 12:59:21]

This CL moves the content of audio_mixer_defines.h into audio_mixer.h. It also
changes the ramping functionality, moves RemixFrame into audio_frame_operations
and moves ShouldMixBefore outside of SourceFrame.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (left):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:73:
AudioFrameOperations::StereoToMono(frame);
Moved to audio_frame_operations

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:81:
NewMixerRampIn(source_frame.audio_frame_);
Replaced with single 'Ramp' in audio_frame_operations.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:49: RTC_DCHECK(audio_frame);
Added checks.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.cc:72: }
Moved ShouldMixBefore outside.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer_impl.h (right):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer_impl.h:41: typedef
std::vector<SourceStatus> MixerAudioSourceList;
Question: could we use a type alias instead of typedef here? Or should we remove
the alias completely? Quote from style guide:

"Don't put an alias in your public API just to save typing in the
implementation; do so only if you intend it to be used by your clients."

[hlundin-webrtc, 2016-10-07 13:18:35]

henrik.lundin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-10-07 13:18:36]

Drive-by comments. I just had a look at the first files, then I realized that
you should probably split the CL. It seems there are independent changes, and
the size is not tiny.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:30: int samples =
static_cast<int>(audio_frame->samples_per_channel_ *
Sanity check the current and target gains?

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:30: int samples =
static_cast<int>(audio_frame->samples_per_channel_ *
int -> size_t

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:34: audio_frame->data_[i]
*= current + (target - current) * i * fraction;
Wait a minute. This looks like you are applying different gains for the same
sample but in different channels. For instance, in the case of ramping down from
1, in stereo, I would expect the gains to be like this:
Left:  [1.0 0.9 0.8 ... ]
Right: [1.0 0.9 0.8 ... ]

I used a .1 decrement here for simplicity, the point is that the all values for
the same time instant should get the same gain across all channels.

I realize that you are keeping the behavior of the old code, but I think that is
wrong.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:39:
RTC_DCHECK(number_of_channels == 1 || number_of_channels == 2);
You get better DCHECK output if you do this in two steps:
RTC_DCHECK_GE(number_of_channels, 1);
RTC_DCHECK_LE(number_of_channels, 2);

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.h (right):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.h:23: // linearly increase
the samples it frame from 0 to full volume.
"it frame"?

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.h:27: void
RemixFrame(AudioFrame* frame, size_t number_of_channels);
Rename parameter to target_number_of_channels.

[aleloi, 2016-10-07 14:07:56]

I divided this CL into two. This one contains the move of MixerAudioSource to
AudioMixer::Source. I also moved RemixFrame (it's a very small change). The next
changes the ramping behavior.

[aleloi, 2016-10-07 14:23:03]

Patchset #2 (id:140001) has been deleted

[aleloi, 2016-10-07 14:23:23]

Thanks for the comments!

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:30: int samples =
static_cast<int>(audio_frame->samples_per_channel_ *
On 2016/10/07 13:18:36, hlundin-webrtc wrote:
> Sanity check the current and target gains?

Done in next CL https://codereview.webrtc.org/2398083005

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:34: audio_frame->data_[i]
*= current + (target - current) * i * fraction;
On 2016/10/07 13:18:36, hlundin-webrtc wrote:
> Wait a minute. This looks like you are applying different gains for the same
> sample but in different channels. For instance, in the case of ramping down
from
> 1, in stereo, I would expect the gains to be like this:
> Left:  [1.0 0.9 0.8 ... ]
> Right: [1.0 0.9 0.8 ... ]
> 
> I used a .1 decrement here for simplicity, the point is that the all values
for
> the same time instant should get the same gain across all channels.
> 
> I realize that you are keeping the behavior of the old code, but I think that
is
> wrong.

I changed the loop to the following in the next CL:

audio_frame->data_[i] *=
 current +
 (target - current) * (i / audio_frame->samples_per_channel_) * fraction;

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:39:
RTC_DCHECK(number_of_channels == 1 || number_of_channels == 2);
On 2016/10/07 13:18:36, hlundin-webrtc wrote:
> You get better DCHECK output if you do this in two steps:
> RTC_DCHECK_GE(number_of_channels, 1);
> RTC_DCHECK_LE(number_of_channels, 2);

Done.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.h (right):

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.h:23: // linearly increase
the samples it frame from 0 to full volume.
On 2016/10/07 13:18:36, hlundin-webrtc wrote:
> "it frame"?

Done.

https://codereview.webrtc.org/2396803004/diff/120001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.h:27: void
RemixFrame(AudioFrame* frame, size_t number_of_channels);
On 2016/10/07 13:18:36, hlundin-webrtc wrote:
> Rename parameter to target_number_of_channels.

Done.

[the sun, 2016-10-11 09:47:07]

lgtm

[hlundin-webrtc, 2016-10-11 10:57:46]

First a comment on the commit message: it is a bit thin. You did write a better
description in the first message to reviewers ("This CL moves the content of
..."). Please, consider fleshing out the commit message with some of that text.

[hlundin-webrtc, 2016-10-11 11:11:49]

Some comments and questions.

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:65: void
RemixFrame(AudioFrame* frame, size_t target_number_of_channels) {
Put input parameters before input/output parameters.
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:66:
RTC_DCHECK_GE(target_number_of_channels, static_cast<size_t>(1));
Can't you write 1u and 2u instead of static_cast...?

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer.h (right):

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:43: struct AudioFrameWithMuted {
It is an AudioFrame with AudioFrameInfo. Suggest you rename it to
AudioFrameWithInfo.

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:48: // The implementation of
GetAudioFrameWithMuted should update
So, the source gives away a pointer to an internal object, which the mixer is
allowed to modify, is that correct? What I'm getting at is the fact that this
method returns a non-const pointer to an AudioFrame. Maybe this is necessary,
but in that case I think this comment should say that the mixer may modify the
contents of the AudioFrame anytime during the assumed valid period.

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:54: virtual AudioFrameWithMuted
GetAudioFrameWithMuted(int32_t id,
Similarly, GetAudioFrameWithInfo.

[hlundin-webrtc, 2016-10-11 11:12:42]

On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> Some comments and questions.
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> webrtc/modules/audio_mixer/audio_frame_manipulator.cc:65: void
> RemixFrame(AudioFrame* frame, size_t target_number_of_channels) {
> Put input parameters before input/output parameters.
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> webrtc/modules/audio_mixer/audio_frame_manipulator.cc:66:
> RTC_DCHECK_GE(target_number_of_channels, static_cast<size_t>(1));
> Can't you write 1u and 2u instead of static_cast...?
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> File webrtc/modules/audio_mixer/audio_mixer.h (right):
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> webrtc/modules/audio_mixer/audio_mixer.h:43: struct AudioFrameWithMuted {
> It is an AudioFrame with AudioFrameInfo. Suggest you rename it to
> AudioFrameWithInfo.
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> webrtc/modules/audio_mixer/audio_mixer.h:48: // The implementation of
> GetAudioFrameWithMuted should update
> So, the source gives away a pointer to an internal object, which the mixer is
> allowed to modify, is that correct? What I'm getting at is the fact that this
> method returns a non-const pointer to an AudioFrame. Maybe this is necessary,
> but in that case I think this comment should say that the mixer may modify the
> contents of the AudioFrame anytime during the assumed valid period.
> 
>
https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
> webrtc/modules/audio_mixer/audio_mixer.h:54: virtual AudioFrameWithMuted
> GetAudioFrameWithMuted(int32_t id,
> Similarly, GetAudioFrameWithInfo.

Now I even realize that the commit message is wrong. :) You removed the ramping
stuff from this CL.

[aleloi, 2016-10-11 11:47:52]

Description was changed from

==========
Changed ramping functionality and moved MixerAudioSource.

MixerAudioSource is moved to AudioMixerImpl::Source.

BUG=webrtc:6346
==========

to

==========
Moved MixerAudioSource and removed audio_mixer_defines.h.

MixerAudioSource is moved to AudioMixerImpl::Source. Structures and methods of
the MixerAudioSource interface have been renamed. The RemixFrame method has
added checks and is moved to audio_frame_manipulator.h

BUG=webrtc:6346
==========

[aleloi, 2016-10-11 11:48:57]

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:65: void
RemixFrame(AudioFrame* frame, size_t target_number_of_channels) {
On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> Put input parameters before input/output parameters.
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

Thanks for noticing! I did the same mistake in the next commit for Ramp.

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:66:
RTC_DCHECK_GE(target_number_of_channels, static_cast<size_t>(1));
On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> Can't you write 1u and 2u instead of static_cast...?

I've already changed this in the next dependent CL (solenberg@ pointed out the
same issue in another place).

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_mixer.h (right):

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:43: struct AudioFrameWithMuted {
On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> It is an AudioFrame with AudioFrameInfo. Suggest you rename it to
> AudioFrameWithInfo.

Good idea, done!

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:48: // The implementation of
GetAudioFrameWithMuted should update
On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> So, the source gives away a pointer to an internal object, which the mixer is
> allowed to modify, is that correct? What I'm getting at is the fact that this
> method returns a non-const pointer to an AudioFrame. Maybe this is necessary,
> but in that case I think this comment should say that the mixer may modify the
> contents of the AudioFrame anytime during the assumed valid period.

I've tried to improve the comment in the last patch set.

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_mixer.h:54: virtual AudioFrameWithMuted
GetAudioFrameWithMuted(int32_t id,
On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> Similarly, GetAudioFrameWithInfo.

Done.

[hlundin-webrtc, 2016-10-11 11:54:11]

lgtm

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right):

https://codereview.webrtc.org/2396803004/diff/160001/webrtc/modules/audio_mix...
webrtc/modules/audio_mixer/audio_frame_manipulator.cc:66:
RTC_DCHECK_GE(target_number_of_channels, static_cast<size_t>(1));
On 2016/10/11 11:48:57, aleloi wrote:
> On 2016/10/11 11:11:49, hlundin-webrtc wrote:
> > Can't you write 1u and 2u instead of static_cast...?
> 
> I've already changed this in the next dependent CL (solenberg@ pointed out the
> same issue in another place).

Acknowledged.

[aleloi, 2016-10-11 12:38:59]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-10-11 12:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2396803004/#ps180001 (title:
"Renamed AudioFrameWithMuted to AudioFrameWithInfo and changed RemixFrame
argument order.")

[commit-bot: I haz the power, 2016-10-11 12:39:08]

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-10-11 13:18:33]

Description was changed from

==========
Moved MixerAudioSource and removed audio_mixer_defines.h.

MixerAudioSource is moved to AudioMixerImpl::Source. Structures and methods of
the MixerAudioSource interface have been renamed. The RemixFrame method has
added checks and is moved to audio_frame_manipulator.h

BUG=webrtc:6346
==========

to

==========
Moved MixerAudioSource and removed audio_mixer_defines.h.

MixerAudioSource is moved to AudioMixerImpl::Source. Structures and methods of
the MixerAudioSource interface have been renamed. The RemixFrame method has
added checks and is moved to audio_frame_manipulator.h

BUG=webrtc:6346
==========

[commit-bot: I haz the power, 2016-10-11 13:18:34]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2016-10-11 13:18:41]

Description was changed from

==========
Moved MixerAudioSource and removed audio_mixer_defines.h.

MixerAudioSource is moved to AudioMixerImpl::Source. Structures and methods of
the MixerAudioSource interface have been renamed. The RemixFrame method has
added checks and is moved to audio_frame_manipulator.h

BUG=webrtc:6346
==========

to

==========
Moved MixerAudioSource and removed audio_mixer_defines.h.

MixerAudioSource is moved to AudioMixerImpl::Source. Structures and methods of
the MixerAudioSource interface have been renamed. The RemixFrame method has
added checks and is moved to audio_frame_manipulator.h

BUG=webrtc:6346

Committed: https://crrev.com/e89141500a7375f7eba7814dd88b690bf4135223
Cr-Commit-Position: refs/heads/master@{#14600}
==========

[commit-bot: I haz the power, 2016-10-11 13:18:43]

Patchset 3 (id:??) landed as
https://crrev.com/e89141500a7375f7eba7814dd88b690bf4135223
Cr-Commit-Position: refs/heads/master@{#14600}