Support the Aec dump for the APM in chrome

Support the Aec dump for the APM in chrome.

The code will run behind the kEnableAudioTrackProcessor flag and the behaviour will be compatible with the existing behaviours.

Since AEC dump can be started only once, so we can't re-create a new instance of MediaStreamAudioProcessor when the format changes. This CL will create the MediaStreamAudioProcessor in the constructor of WebRtcAudioCapturer and call OnCaptureFormatChanged() to the MediaStreamAudioProcessor when the format changes.

BUG=264611
TEST=manual enable aec dump in chrome://webrtc-internals and verify the aec dump audio files.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255769

[no longer working on chromium, 2014-03-05 17:46:58]

Henrik, please review.

Thanks,
SX

[no longer working on chromium, 2014-03-06 08:36:35]

ping, Henrik, please take a look?
If you think it is insufficient to have only one reviewer for such case, feel
free to add other reviewers like Tommi or Joi.

Thanks,
SX

[Henrik Grunell, 2014-03-06 10:12:20]

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

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.h:87: // Called on the main
render thread.
Comment that it takes ownership of |aec_dump_file|.

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

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:151: FILE* handle
= base::FdopenPlatformFile(aec_dump_file, "w");
Change "handle" -> "file_stream" or "stream".

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

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:759:
GetWebRtcAudioDevice()->EnableAecDump(file_handle);
Have WebRtcAudioDeviceImpl::EnableAecDump take a base::PlatformFile instead and
use IPC::PlatformFileForTransitToPlatformFile here.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:763: // TODO(xians):
Remove the following code after kEnableAudioTrackProcessing
It shouldn't be removed when the flag is on by default, but when the flag is
removed.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:584: aec_dump_file_ ==
aec_dump_file);
IS it expected to be called multiple times with the same argument?

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:590: aec_dump_file_ =
aec_dump_file;
|audio_processor_| takes ownership of |aec_dump_file| so I don't think we should
store it in this class. What's the reason of doing that?

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:112: // Uses by the unittests to
inject their own source to the capturer.
"Used".

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:212: // Used for start the Aec
dump on the |audio_processor_|.
"starting"

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:456: StartAecDump();
Hmm, we re-use |aec_dump_file_| but that likely won't work. The APM that has
previously started dump will close the file stream when stopping the dump and
with that the file descriptor is closed (on posix at least).

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.h:350: // started on the default
capturer.
Comment that it takes ownership of |file_handle|.

Comment that if there's no capturer yet, it holds on to the file until one is
created and then enabled the dump.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.h:397: void StartAecDump();
Stick with either Enable/Disable or Start/Stop naming throughout all classes.

[no longer working on chromium, 2014-03-06 18:57:21]

Henrik, PTAL.

Thanks,
SX

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

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.h:87: // Called on the main
render thread.
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> Comment that it takes ownership of |aec_dump_file|.

Done.

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

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:151: FILE* handle
= base::FdopenPlatformFile(aec_dump_file, "w");
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> Change "handle" -> "file_stream" or "stream".

Done.

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

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:759:
GetWebRtcAudioDevice()->EnableAecDump(file_handle);
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> Have WebRtcAudioDeviceImpl::EnableAecDump take a base::PlatformFile instead
and
> use IPC::PlatformFileForTransitToPlatformFile here.

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:763: // TODO(xians):
Remove the following code after kEnableAudioTrackProcessing
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> It shouldn't be removed when the flag is on by default, but when the flag is
> removed.

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:584: aec_dump_file_ ==
aec_dump_file);
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> IS it expected to be called multiple times with the same argument?

The code was removed.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:590: aec_dump_file_ =
aec_dump_file;
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> |audio_processor_| takes ownership of |aec_dump_file| so I don't think we
should
> store it in this class. What's the reason of doing that?

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:112: // Uses by the unittests to
inject their own source to the capturer.
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> "Used".

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:212: // Used for start the Aec
dump on the |audio_processor_|.
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> "starting"

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:456: StartAecDump();
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> Hmm, we re-use |aec_dump_file_| but that likely won't work. The APM that has
> previously started dump will close the file stream when stopping the dump and
> with that the file descriptor is closed (on posix at least).

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.h:350: // started on the default
capturer.
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> Comment that it takes ownership of |file_handle|.
> 
> Comment that if there's no capturer yet, it holds on to the file until one is
> created and then enabled the dump.

Done.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.h:397: void StartAecDump();
On 2014/03/06 10:12:20, Henrik Grunell wrote:
> Stick with either Enable/Disable or Start/Stop naming throughout all classes.

I suggest not. It is better to put all the code together in one method instead
of spreading the use cases around.
By using this method, we know exactly what will happen after calling
StartAecDump(). Hope it is fine to keep it.

[Henrik Grunell, 2014-03-06 19:55:12]

There's a lot of changes in the patch set 2. I can't easily see how some are
related to the AEC dump. Please explain. A summary in the CL description would
be good.

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://codereview.chromium.org/187913002/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.h:397: void StartAecDump();
On 2014/03/06 18:57:21, xians1 wrote:
> On 2014/03/06 10:12:20, Henrik Grunell wrote:
> > Stick with either Enable/Disable or Start/Stop naming throughout all
classes.
> 
> I suggest not. It is better to put all the code together in one method instead
> of spreading the use cases around.
> By using this method, we know exactly what will happen after calling
> StartAecDump(). Hope it is fine to keep it.

I just meant the naming of the functions. Either StartAecDump everywhere or
EnableAecDump everywhere. Same with Stop or Disable.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.h (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.h:55: void
OnCaptureFormatChanged(const media::AudioParameters& source_params);
Why is this needed in this CL?

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:152: float sample_rate);
What has removing those parameters to do with the AEC dump?

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:180: // Notify the playout
sink of the change.
Is this needed for the AEC dump? Why?

[Henrik Grunell, 2014-03-06 19:56:54]

The particular comments in my last comment was some examples; please explain the
changes on a high level.

[no longer working on chromium, 2014-03-06 20:00:36]

Done with adding some description to the CL.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.h (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.h:55: void
OnCaptureFormatChanged(const media::AudioParameters& source_params);
On 2014/03/06 19:55:12, Henrik Grunell wrote:
> Why is this needed in this CL?

It is needed in order not to create a new processor when the format has changed.
Previously, we create a new instance of MediaStreamAudioProcessor when the
capture format has changed, but this will be a problem for AEC dump since we can
only start AEC dump on on processor.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.h:152: float sample_rate);
On 2014/03/06 19:55:12, Henrik Grunell wrote:
> What has removing those parameters to do with the AEC dump?

They are used to create the MediaStreamAudioProcessor.
they are not needed any more since MediaStreamAudioProcessor will be created on
the constructor.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:180: // Notify the playout
sink of the change.
On 2014/03/06 19:55:12, Henrik Grunell wrote:
> Is this needed for the AEC dump? Why?

It is not from this CL, just some rebased code from the latest sync.

[Henrik Grunell, 2014-03-07 09:14:57]

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

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:166:
InitializeCaptureConverter(source_params);
Is InitializeCaptureConverter() OK to call multiple times?

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:470: StopAecDump();
This shouldn't be necessary, it will be stopped when |audio_processing_| goes
away. It doesn't hurt though.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.h (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.h:54: // Called on the main
render thread.  The caller is responsible fo
Ooo, now I'm curious! :) Responsible for what? World economy? Sorting candy into
the right boxes? (Comment is chopped.)

Maybe add a comment that the capture thread will change?

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:456: {
Nit: don't need {}

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:513: StartAecDump();
Have StartAecDump() take the file as an argument, then you don't need to store
it in the member.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:521: aec_dump_file_ =
base::kInvalidPlatformFileValue;
Close the file before setting to invalid.

[no longer working on chromium, 2014-03-07 10:14:54]

Henrik, PTAL.

SX

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

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:166:
InitializeCaptureConverter(source_params);
On 2014/03/07 09:14:57, Henrik Grunell wrote:
> Is InitializeCaptureConverter() OK to call multiple times?

Yes.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.cc:470: StopAecDump();
On 2014/03/07 09:14:57, Henrik Grunell wrote:
> This shouldn't be necessary, it will be stopped when |audio_processing_| goes
> away. It doesn't hurt though.

As discussed offline, lets keep this.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor.h (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor.h:54: // Called on the main
render thread.  The caller is responsible fo
On 2014/03/07 09:14:57, Henrik Grunell wrote:
> Ooo, now I'm curious! :) Responsible for what? World economy? Sorting candy
into
> the right boxes? (Comment is chopped.)
> 
> Maybe add a comment that the capture thread will change?

Done.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:456: {
On 2014/03/07 09:14:57, Henrik Grunell wrote:
> Nit: don't need {}

Done.

https://codereview.chromium.org/187913002/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:513: StartAecDump();
On 2014/03/07 09:14:57, Henrik Grunell wrote:
> Have StartAecDump() take the file as an argument, then you don't need to store
> it in the member.

Discussed offline, keep the way how it is.

[Henrik Grunell, 2014-03-07 10:51:10]

LGTM. Make sure you test with the command line flag. Did you run the automatic
tests?

[no longer working on chromium, 2014-03-07 18:08:21]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:08:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/187913002/100001

[commit-bot: I haz the power, 2014-03-07 18:11:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:11:57]

Try jobs failed on following builders: mac_chromium_rel

[no longer working on chromium, 2014-03-07 19:00:17]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-07 19:01:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/187913002/100001

[commit-bot: I haz the power, 2014-03-07 20:25:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/187913002/100001

[commit-bot: I haz the power, 2014-03-08 10:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/187913002/100001

[commit-bot: I haz the power, 2014-03-08 13:09:43]

Change committed as 255769