Strip away the keyboard mic channel if no audio processing.

Strip away the keyboard mic channel if no audio processing.

This will currently never be exercised, but is required when we support multiple MediaStreamAudioProcessors with different constraints.

TEST=Since we don't support this yet, I have tested with a custom build hard coded to not create |audio_processor_| in MSAP (to simulate custom constraint settings that would cause no audio processing to happen).
BUG=345296
Committed: https://crrev.com/e372e4f4bbcb94a6ff427b337845ad452133e527
Cr-Commit-Position: refs/heads/master@{#297386}

[Henrik Grunell, 2014-09-23 16:08:37]

grunell@chromium.org changed reviewers:
+ xians@chromium.org

[Henrik Grunell, 2014-09-23 16:08:49]

grunell@chromium.org changed reviewers:
+ tommi@chromium.org

[Henrik Grunell, 2014-09-23 16:09:21]

I think Shijing knows this class best, but Tommi feel free to have a look if you
have time.

[no longer working on chromium, 2014-09-24 12:41:27]

I think a unittest is desired for such changes.

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:232: if
(input_format_.channel_layout() ==
you should move these code to MediaStreamAudioFifo

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:235:
scoped_ptr<media::AudioBus> audio_source_stereo =
use a member in MediaStreamAudioFifo, and set it up when MediaStreamAudioFifo is
created.

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:486:
input_format.channel_layout();
can the output_channel_layout be media::CHANNEL_LAYOUT_STEREO_AND_KEYBOARD_MIC
at all?

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:526: new
MediaStreamAudioFifo(capture_fifo_channels,
how about modify MediaStreamAudioFifo constructor and inject a boolean there to
indicate if audio processing is enabled or not, then all these channel layout
things will be handled internally in MediaStreamAudioFifo.

[Henrik Grunell, 2014-09-24 20:01:29]

Will move it to the fifo tomorrow if you're OK with the additional info
injected.

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:232: if
(input_format_.channel_layout() ==
On 2014/09/24 12:41:27, xians1 wrote:
> you should move these code to MediaStreamAudioFifo

Yep, but I need the additional info about layout and audio processing enabled.

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:486:
input_format.channel_layout();
On 2014/09/24 12:41:27, xians1 wrote:
> can the output_channel_layout be media::CHANNEL_LAYOUT_STEREO_AND_KEYBOARD_MIC
> at all?

No.

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:526: new
MediaStreamAudioFifo(capture_fifo_channels,
On 2014/09/24 12:41:27, xians1 wrote:
> how about modify MediaStreamAudioFifo constructor and inject a boolean there
to
> indicate if audio processing is enabled or not, then all these channel layout
> things will be handled internally in MediaStreamAudioFifo.

Yes, but the fifo would need to know about the layout, since we can have 3
channels in other layouts as well.

[Henrik Grunell, 2014-09-26 10:04:27]

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:232: if
(input_format_.channel_layout() ==
On 2014/09/24 20:01:29, Henrik Grunell wrote:
> On 2014/09/24 12:41:27, xians1 wrote:
> > you should move these code to MediaStreamAudioFifo
> 
> Yep, but I need the additional info about layout and audio processing enabled.

Changed to what I proposed offline: take source and destination channels in the
fifo ctor, use that info to possibly strip away channels.

https://codereview.chromium.org/594233002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_audio_processor.cc:235:
scoped_ptr<media::AudioBus> audio_source_stereo =
On 2014/09/24 12:41:27, xians1 wrote:
> use a member in MediaStreamAudioFifo, and set it up when MediaStreamAudioFifo
is
> created.

Done.

[no longer working on chromium, 2014-09-26 11:20:41]

lgtm with one minor comment.

https://codereview.chromium.org/594233002/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/594233002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:154:
audio_source_intermediate_ =
You can set up this audio_source_intermediate_ in the constructor, and in this
Push() call, do 
if (audio_source_intermediate_) {
  for (int i = 0; i < destination_->bus()->channels(); ++i) {
    audio_source_intermediate_->SetChannelData(
        i, const_cast<float*>(source->channel(i)));
  }
  audio_source_intermediate_->set_frames(source->frames());
  source_to_push = audio_source_intermediate_.get();
}

[Henrik Grunell, 2014-09-30 07:46:54]

https://codereview.chromium.org/594233002/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/594233002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:154:
audio_source_intermediate_ =
On 2014/09/26 11:20:41, xians1 wrote:
> You can set up this audio_source_intermediate_ in the constructor, and in this
> Push() call, do 
> if (audio_source_intermediate_) {
>   for (int i = 0; i < destination_->bus()->channels(); ++i) {
>     audio_source_intermediate_->SetChannelData(
>         i, const_cast<float*>(source->channel(i)));
>   }
>   audio_source_intermediate_->set_frames(source->frames());
>   source_to_push = audio_source_intermediate_.get();
> }

Done.

[Henrik Grunell, 2014-09-30 07:55:23]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-09-30 07:56:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/594233002/40001

[commit-bot: I haz the power, 2014-09-30 08:53:25]

Committed patchset #3 (id:40001) as f0bf2187d4916bc5a8d75efdccc683f02ac156fc

[commit-bot: I haz the power, 2014-09-30 08:55:50]

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