Support multiple files for AEC dump.

Support multiple files for AEC dump.

* This adds support for AEC dump files in multiple tabs and if multiple getUserMedia calls are made in one tab.
* Each AEC dump consumer registers with the browser to recieve a file handle when AEC dump is enabled.
* Add dedicated aec dump message filter an messages file.
* Works with and without track-processing.

NOTRY=true

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

[Henrik Grunell, 2014-06-16 14:50:03]

New patch: Support for track processing. All existing tests pass now. Manual
multi-tab test OK.

[Henrik Grunell, 2014-06-16 15:21:09]

Hi, please review. Suggested review responsibilities:

xians:
content/browser/media/*
content/renderer/media/*
content gypi files.

sky:
content/browser/renderer_host/render_process_host_impl.*

jamesr:
content/renderer/render_thread_impl.*

tsepez:
content/common/content_message_generator.h
ipc/ipc_message_start.h

[Henrik Grunell, 2014-06-16 15:22:22]

tsepez: Also
content/common/media/aec_dump_messages.h
content/common/media/media_stream_messages.h

[sky, 2014-06-16 15:23:45]

https://codereview.chromium.org/334743006/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/334743006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2211: break;
You're missing a {} on 2209 as this doesn't do what you want.

[Henrik Grunell, 2014-06-16 15:27:59]

https://codereview.chromium.org/334743006/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/334743006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2211: break;
On 2014/06/16 15:23:45, sky wrote:
> You're missing a {} on 2209 as this doesn't do what you want.

Oops. Fixed, thanks.

[Tom Sepez, 2014-06-16 17:33:18]

Messages LGTM, but a couple of other nits.

https://codereview.chromium.org/334743006/diff/80001/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/334743006/diff/80001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:420: break;
I'm confused by this loop which seems to only execute once?

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

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:520: int id =
g_capturer_id_counter++;
why is it OK to increment this outside the lock?

[no longer working on chromium, 2014-06-16 18:30:55]

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
File content/common/media/aec_dump_messages.h (right):

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
ditto

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:8: //#include "base/basictypes.h"
??

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:14: #undef IPC_MESSAGE_EXPORT
?? add comment if it is required.

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:16: #define IPC_MESSAGE_START
AecDumpMsgStart
ditto

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
File content/renderer/media/aec_dump_message_filter.cc (right):

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.cc:35: return g_filter;
which thread creates the AecDumpMessageFilter? and which thread calls this
Get()?
can this be a race if they are accessed be different threads? 

Can we add thread check?

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.cc:48: if (sender_)
I am not familiar with the IPC code, could you please get some one to take a
look at it?

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.cc:115:
observer_->OnAecDumpFile(id, file_handle);
why not call OnEnableAecDump?

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
File content/renderer/media/aec_dump_message_filter.h (right):

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit, you don't need the (c)

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.h:43: DCHECK(!observer !=
!observer_);
move the implementation to .cc

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.h:43: DCHECK(!observer !=
!observer_);
it seems a high skilled DCHECK, can you simply it and make it explicitly?

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

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:22: int g_capturer_id_counter
= 0;
why? can't it be a member instead?

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:234: if
(MediaStreamAudioProcessor::IsAudioTrackProcessingEnabled()) {
WebRtcAudioDeviceImpl is not the right place for all these AEC dump code, they
should be in MediaStreamAudioProcessor, that says, we should allocate one aec
dump file descriptor for each MediaStreamAudioProcessor. Right now you code
allocates one dump file desc for each capturer, this is not completely correct.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:236:
DCHECK(aec_dump_message_filter_);
you don't need this DCHECK(), the following line will crash if it is NULL

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:240:
RegisterAecDumpForId(iter->first);
indentation.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:267:
aec_dump_message_filter_->SetObserver(NULL);
if you have more than 1 tab using webrtc, this SetObserver method will try to
overwrite each other, is it OK?

[Henrik Grunell, 2014-06-17 18:40:11]

jamesr@ and sky@ can you please review?

[jamesr, 2014-06-17 19:09:39]

content/renderer/render_thread_impl lgtm, but it's not very clear what AEC dumps
are or why they are important.  Can you make the comment more descriptive before
landing please?

[sky, 2014-06-17 19:20:29]

LGTM

[Henrik Grunell, 2014-06-17 19:56:44]

On 2014/06/17 19:09:39, jamesr wrote:
> content/renderer/render_thread_impl lgtm, but it's not very clear what AEC
dumps
> are or why they are important.  Can you make the comment more descriptive
before
> landing please?

Sure, done.

[Henrik Grunell, 2014-06-17 20:21:42]

Shijing, PTAL.

https://codereview.chromium.org/334743006/diff/80001/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/334743006/diff/80001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:420: break;
On 2014/06/16 17:33:18, Tom Sepez wrote:
> I'm confused by this loop which seems to only execute once?

Right, the loop is not needed. (Copy paste error.) Fixed.

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
File content/common/media/aec_dump_messages.h (right):

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/06/16 18:30:55, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:8: //#include "base/basictypes.h"
On 2014/06/16 18:30:55, xians1 wrote:
> ??

Removed.

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:14: #undef IPC_MESSAGE_EXPORT
On 2014/06/16 18:30:55, xians1 wrote:
> ?? add comment if it is required.

Well, it's there in all other *messages.h files in this folder at least. Without
comments. I'm assuming it's needed for certain build cases.

https://codereview.chromium.org/334743006/diff/80001/content/common/media/aec...
content/common/media/aec_dump_messages.h:16: #define IPC_MESSAGE_START
AecDumpMsgStart
On 2014/06/16 18:30:55, xians1 wrote:
> ditto

ditto

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
File content/renderer/media/aec_dump_message_filter.cc (right):

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.cc:35: return g_filter;
On 2014/06/16 18:30:55, xians1 wrote:
> which thread creates the AecDumpMessageFilter? and which thread calls this
> Get()?

It's created by the RenderThreadImpl on the main thread. It's accessed once in
PeerConnectionDependencyFactory *or* once in WebRtcAudioDeviceImpl.

> can this be a race if they are accessed be different threads? 

I don't see what the race would be.

> 
> Can we add thread check?

Doesn't seem necessary.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.cc:48: if (sender_)
On 2014/06/16 18:30:55, xians1 wrote:
> I am not familiar with the IPC code, could you please get some one to take a
> look at it?

Sure. But the code is similar to existing code in other message filters, if you
want to compare.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.cc:115:
observer_->OnAecDumpFile(id, file_handle);
On 2014/06/16 18:30:55, xians1 wrote:
> why not call OnEnableAecDump?

The observer doesn't have such a function. Not sure what you mean.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
File content/renderer/media/aec_dump_message_filter.h (right):

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/06/16 18:30:55, xians1 wrote:
> nit, you don't need the (c)

Done.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.h:43: DCHECK(!observer !=
!observer_);
On 2014/06/16 18:30:55, xians1 wrote:
> move the implementation to .cc

Done.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/a...
content/renderer/media/aec_dump_message_filter.h:43: DCHECK(!observer !=
!observer_);
On 2014/06/16 18:30:55, xians1 wrote:
> it seems a high skilled DCHECK, can you simply it and make it explicitly?

Changed it to be more readable.

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

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:22: int g_capturer_id_counter
= 0;
On 2014/06/16 18:30:55, xians1 wrote:
> why? can't it be a member instead?

Right, it can. Done.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:234: if
(MediaStreamAudioProcessor::IsAudioTrackProcessingEnabled()) {
On 2014/06/16 18:30:55, xians1 wrote:
> WebRtcAudioDeviceImpl is not the right place for all these AEC dump code, they
> should be in MediaStreamAudioProcessor, that says, we should allocate one aec
> dump file descriptor for each MediaStreamAudioProcessor. Right now you code
> allocates one dump file desc for each capturer, this is not completely
correct.

After discussing offline I understand that this is required for future planned
changes of MSAP and the capturer. Done.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:236:
DCHECK(aec_dump_message_filter_);
On 2014/06/16 18:30:55, xians1 wrote:
> you don't need this DCHECK(), the following line will crash if it is NULL

Done.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:240:
RegisterAecDumpForId(iter->first);
On 2014/06/16 18:30:55, xians1 wrote:
> indentation.

Done.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:267:
aec_dump_message_filter_->SetObserver(NULL);
On 2014/06/16 18:30:55, xians1 wrote:
> if you have more than 1 tab using webrtc, this SetObserver method will try to
> overwrite each other, is it OK?

This is changed now. MSAP is a delegate instead.

https://codereview.chromium.org/334743006/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc_audio_device_impl.cc:520: int id =
g_capturer_id_counter++;
On 2014/06/16 17:33:18, Tom Sepez wrote:
> why is it OK to increment this outside the lock?

This has been removed, ids are handled in the message filter.

[no longer working on chromium, 2014-06-18 08:45:39]

one comment, please address it.

lgtm

https://codereview.chromium.org/334743006/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/334743006/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1379:
IPC_MESSAGE_HANDLER(AecDumpMsg_RegisterAecDumpConsumer,
either put them into ENABLE_WEBRTC or move all the code out of ENABLE_WEBRTC

[Henrik Grunell, 2014-06-18 10:40:41]

https://codereview.chromium.org/334743006/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/334743006/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1379:
IPC_MESSAGE_HANDLER(AecDumpMsg_RegisterAecDumpConsumer,
On 2014/06/18 08:45:38, xians1 wrote:
> either put them into ENABLE_WEBRTC or move all the code out of ENABLE_WEBRTC

Done.

[Henrik Grunell, 2014-06-18 10:40:48]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-06-18 10:42:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/334743006/160001

[commit-bot: I haz the power, 2014-06-18 13:15:49]

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

[commit-bot: I haz the power, 2014-06-18 13:15:50]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Henrik Grunell, 2014-06-18 13:34:01]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-06-18 13:35:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/334743006/180001

[Henrik Grunell, 2014-06-18 14:25:54]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-06-18 14:27:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/334743006/200001

[commit-bot: I haz the power, 2014-06-18 18:21:34]

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

[commit-bot: I haz the power, 2014-06-18 18:21:35]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Henrik Grunell, 2014-06-18 19:35:47]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-06-18 19:37:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grunell@chromium.org/334743006/200001

[commit-bot: I haz the power, 2014-06-18 19:39:10]

Change committed as 278148