Implement writing mic audio input data to file for debugging purposes.

Implement writing mic audio input data to file for debugging purposes.

This is enabled in chrome://webrtc-internals with the current "enable diagnostic audio recordings" checkbox. The user chooses a base filename, on which extensions are added as:
<base name>.<process id>.source_input.<stream id>.pcm

WebRTCInternals tells all RenderProcessHosts to enable recording, which in turn calls the AudioInputRenderHost. The AIRH tells all AudioInputControllers (if any) to enable recording and gives it a pointer to an AudioInputWriter.

When audio data reaches AIC it tells the AudioInputWriter to write the data to file.

When a new AudioInputController is created in AIRH, the recording status is checked, and if enabled it's enabled for the new AIC.

BUG=516584
NOTRY=true
TEST=Browser test. Manually by enabling in http://webrtc-internals and make WebRTC audio call.

Committed: https://crrev.com/7566ffc1f76281738648633c1e7e5fc919d70e13
Cr-Commit-Position: refs/heads/master@{#347605}

[Henrik Grunell, 2015-08-07 10:24:48]

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

[Henrik Grunell, 2015-08-07 10:25:01]

PTAL.

[tommi (sloooow) - chröme, 2015-08-07 12:09:51]

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:265: audio_debug_recordings_ = false;
add thread check here and elsewhere for these members

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.h:98: base::FilePath
audio_debug_recordings_file_path() {
nit: return const & and make the method const?

also, what do you think about moving these methods into the cc file and have a
thread check there so we can be sure these variables are always accessed on the
same thread?

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_debug_writer.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_debug_writer.cc:34: int
data_size_bytes = 2 * data->frames() * data->channels();
Can you add a note here saying that we convert the data to 16 bit audio here and
change 2 to sizeof(int16)?

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_debug_writer.cc:41:
file_.WriteAtCurrentPos(interleaved_data_.get(), interleaved_data_size_);
nit: should the second parameter be data_size_bytes instead of
interleaved_data_size_?
At the moment they are the same, but I'm wondering if we should use
data_size_bytes for representing the amount of actually used bytes in the buffer
and interleaved_data_size_ for the size of the buffer itself.

If we do that, we can change the if() condition above to:
if (!interleaved_data_ || interleaved_data_size_ < data_size_bytes) {

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:48: file_path,
not sure about the indent here... 4 spaces from the variable name? (is git cl
format perhaps messing this up?)

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:51: VLOG(1) <<
"Could not open debug recording file, error=" <<
nit: could use [P]LOG_IF(ERROR, !recording_file.IsValid()) << "blahh";

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:57: void
CloseFile(base::File* file) {
just wondering - could ownership be passed via move semantics and automatically
deleted at end of scope?

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:63: void
DeleteInputDebugWriterOnFileThread(AudioInputDebugWriter* writer) {
same here... wondering if we could use scoped_ptr<>

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:91:
AudioInputDebugWriter* input_debug_writer;
I think you could still use a scoped_ptr even though you pass ownership over

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:139: for
(AudioEntryMap::iterator i = audio_entries_.begin();
for (const auto& entry : audio_entries_)
  EnabledDebugRecordingForId(file_with_extensions, entry.first);

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:146: for
(AudioEntryMap::iterator it = audio_entries_.begin();
could use a range based loop here as well

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:582:
entry->input_debug_writer));
would be nice to do input_debug_writer.Pass() :)

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:675: return;
should we dcheck here or is this an expected case?

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:683:
base::Unretained(new base::File(file.Pass()))));
is there no way we could just do
file.Pass() here?

I would have expected base::Passed(file.Pass()) to work if CloseFile() simply
accepted a base::File parameter (by value that is).

[Henrik Grunell, 2015-08-17 15:05:17]

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:265: audio_debug_recordings_ = false;
On 2015/08/07 12:09:50, tommi wrote:
> add thread check here and elsewhere for these members

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.h:98: base::FilePath
audio_debug_recordings_file_path() {
On 2015/08/07 12:09:51, tommi wrote:
> nit: return const & and make the method const?

Agree, done.

> also, what do you think about moving these methods into the cc file and have a
> thread check there so we can be sure these variables are always accessed on
the
> same thread?

That's good, done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_debug_writer.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_debug_writer.cc:34: int
data_size_bytes = 2 * data->frames() * data->channels();
On 2015/08/07 12:09:51, tommi wrote:
> Can you add a note here saying that we convert the data to 16 bit audio here
and
> change 2 to sizeof(int16)?

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_debug_writer.cc:41:
file_.WriteAtCurrentPos(interleaved_data_.get(), interleaved_data_size_);
On 2015/08/07 12:09:51, tommi wrote:
> nit: should the second parameter be data_size_bytes instead of
> interleaved_data_size_?
> At the moment they are the same, but I'm wondering if we should use
> data_size_bytes for representing the amount of actually used bytes in the
buffer
> and interleaved_data_size_ for the size of the buffer itself.
> 
> If we do that, we can change the if() condition above to:
> if (!interleaved_data_ || interleaved_data_size_ < data_size_bytes) {

Sure, agree it's slightly better. Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:48: file_path,
On 2015/08/07 12:09:51, tommi wrote:
> not sure about the indent here... 4 spaces from the variable name? (is git cl
> format perhaps messing this up?)

Indeed. Creative indenting. :) Fixed.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:51: VLOG(1) <<
"Could not open debug recording file, error=" <<
On 2015/08/07 12:09:51, tommi wrote:
> nit: could use [P]LOG_IF(ERROR, !recording_file.IsValid()) << "blahh";

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:57: void
CloseFile(base::File* file) {
On 2015/08/07 12:09:51, tommi wrote:
> just wondering - could ownership be passed via move semantics and
automatically
> deleted at end of scope?

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:63: void
DeleteInputDebugWriterOnFileThread(AudioInputDebugWriter* writer) {
On 2015/08/07 12:09:51, tommi wrote:
> same here... wondering if we could use scoped_ptr<>

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:91:
AudioInputDebugWriter* input_debug_writer;
On 2015/08/07 12:09:51, tommi wrote:
> I think you could still use a scoped_ptr even though you pass ownership over

I think that would signal that it's OK if it's deleted automatically in the
AudioEntry dtor which it isn't since it must be deleted on the file tread. A raw
pointer is clearer imo.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:139: for
(AudioEntryMap::iterator i = audio_entries_.begin();
On 2015/08/07 12:09:51, tommi wrote:
> for (const auto& entry : audio_entries_)
>   EnabledDebugRecordingForId(file_with_extensions, entry.first);

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:146: for
(AudioEntryMap::iterator it = audio_entries_.begin();
On 2015/08/07 12:09:51, tommi wrote:
> could use a range based loop here as well

Done.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:582:
entry->input_debug_writer));
On 2015/08/07 12:09:51, tommi wrote:
> would be nice to do input_debug_writer.Pass() :)

Why not? Made it move-only. :)

EDIT: Argh, can't do that since |file_| should only be accessed on the FILE
thread. Using scoped_ptr anyway.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:675: return;
On 2015/08/07 12:09:51, tommi wrote:
> should we dcheck here or is this an expected case?

It could happen if the user selects a folder without write access for example.

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:683:
base::Unretained(new base::File(file.Pass()))));
On 2015/08/07 12:09:51, tommi wrote:
> is there no way we could just do
> file.Pass() here?
> 
> I would have expected base::Passed(file.Pass()) to work if CloseFile() simply
> accepted a base::File parameter (by value that is).

Done.

[Henrik Grunell, 2015-08-19 09:39:23]

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

[Henrik Grunell, 2015-08-19 09:45:55]

nasko@: Please review (as owner of)
* content/browser/renderer_host/render_process_host_impl.h/cc
* content/public/browser/render_process_host.h

Tommi: another round please.

[tommi (sloooow) - chröme, 2015-08-19 11:32:21]

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:91:
AudioInputDebugWriter* input_debug_writer;
On 2015/08/17 15:05:17, Henrik Grunell wrote:
> On 2015/08/07 12:09:51, tommi wrote:
> > I think you could still use a scoped_ptr even though you pass ownership over
> 
> I think that would signal that it's OK if it's deleted automatically in the
> AudioEntry dtor which it isn't since it must be deleted on the file tread. A
raw
> pointer is clearer imo.

By that logic, it sounds like not deleting it should be OK.  However, I don't
think that's OK either.

On the contrary I'd say that if we delete the object and crash or cause a race
to be triggered (caught by tsan), then it would be preferable.

Since this variable is being converted to a scoped_ptr, then Pass()ed and
eventually set to nullptr, I think it should just be a scoped_ptr.  Not worrying
about setting to nullptr is one less thing to worry about and we can still do a
DCHECK in the dtor.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/media/w...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/media/w...
content/browser/media/webrtc_internals.cc:55: FILE_PATH_LITERAL("audio_debug"));
is the extension no longer useful?

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_debug_writer.cc (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.cc:37:
interleaved_data_.reset(new char[data_size_bytes]);
nit: if you change interleaved_data_ to be of type int16, you don't need the
sizeof(int16) thing in the calculation of data_size_bytes.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.cc:40:
data->ToInterleaved(data->frames(), sizeof(int16), interleaved_data_.get());
...and here you could use sizeof(interleaved_data_[0])
actually, maybe interleaved_data_ should be scoped_array

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_debug_writer.h (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.h:30: 
nit: remove

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.h:44:
scoped_ptr<char[]> interleaved_data_;
maybe a comment about the format of the buffer? (interleaved 16 bit audio? 
maybe it should be int16?)

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:580:
entry->input_debug_writer = nullptr;
I'd still prefer scoped_ptr :)  with raw pointers, overwriting and forgetting to
set to nullptr is easier.

[Henrik Grunell, 2015-08-19 19:57:16]

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.cc:91:
AudioInputDebugWriter* input_debug_writer;
On 2015/08/19 11:32:21, tommi wrote:
> On 2015/08/17 15:05:17, Henrik Grunell wrote:
> > On 2015/08/07 12:09:51, tommi wrote:
> > > I think you could still use a scoped_ptr even though you pass ownership
over
> > 
> > I think that would signal that it's OK if it's deleted automatically in the
> > AudioEntry dtor which it isn't since it must be deleted on the file tread. A
> raw
> > pointer is clearer imo.
> 
> By that logic, it sounds like not deleting it should be OK.  However, I don't
> think that's OK either.

Hm, well, not sure I agree. :) But maybe "scoped_ptr primarily means I own it
and shall delete it" logic is commonly used in Chromium. Anyway, I'll change it.

> On the contrary I'd say that if we delete the object and crash or cause a race
> to be triggered (caught by tsan), then it would be preferable.

Sure, that will catch it. And that's a pro.

> Since this variable is being converted to a scoped_ptr, then Pass()ed and
> eventually set to nullptr, I think it should just be a scoped_ptr.  Not
worrying
> about setting to nullptr is one less thing to worry about and we can still do
a
> DCHECK in the dtor.

Done.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/media/w...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/media/w...
content/browser/media/webrtc_internals.cc:55: FILE_PATH_LITERAL("audio_debug"));
On 2015/08/19 11:32:21, tommi wrote:
> is the extension no longer useful?

Yet other extensions are added to this. Also, this name is the base for both the
AEC dump and the "mic input" dump (which has nothing to do with AEC).

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_debug_writer.cc (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.cc:37:
interleaved_data_.reset(new char[data_size_bytes]);
On 2015/08/19 11:32:21, tommi wrote:
> nit: if you change interleaved_data_ to be of type int16, you don't need the
> sizeof(int16) thing in the calculation of data_size_bytes.

Done.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.cc:40:
data->ToInterleaved(data->frames(), sizeof(int16), interleaved_data_.get());
On 2015/08/19 11:32:21, tommi wrote:
> ...and here you could use sizeof(interleaved_data_[0])

Done.

> actually, maybe interleaved_data_ should be scoped_array

scoped_array is deprecated and no more.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_debug_writer.h (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.h:30: 
On 2015/08/19 11:32:21, tommi wrote:
> nit: remove

Done.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_debug_writer.h:44:
scoped_ptr<char[]> interleaved_data_;
On 2015/08/19 11:32:21, tommi wrote:
> maybe a comment about the format of the buffer? (interleaved 16 bit audio? 
> maybe it should be int16?)

Done.

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:580:
entry->input_debug_writer = nullptr;
On 2015/08/19 11:32:21, tommi wrote:
> I'd still prefer scoped_ptr :)  with raw pointers, overwriting and forgetting
to
> set to nullptr is easier.

Done.

[tommi (sloooow) - chröme, 2015-08-19 20:06:34]

thanks for dealing with all the nits.  I think you can remove the two reset()'s
below now, but lgtm!  good stuff.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:577:
entry->input_debug_writer.reset();
should be no need for this since you've already called Pass()

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:715:
entry->input_debug_writer.reset();
ditto

[nasko, 2015-08-19 22:04:15]

This CL has issues with accessing objects across threads and possibly object
lifetime issues too. It will be great if all the thread hops have some comments
associated with them explaining what is going on or a high level overview of the
intended flow.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
render_process_host_(render_process_host),
Why pass the full RenderProcessHost object? It is only used to get an ID and it
already has an immutable ID, regardless of which underlying OS process is
associated with it.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:657:
IntToStringType(base::GetProcId(render_process_host_->GetHandle())));
This is not allowed. From what I can see in this file, this method is called on
the IO thread, where render_process_host_ cannot be accessed.

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

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1787:
audio_input_renderer_host_,
How do we guarantee that audio_input_renderer_host_ is not destroyed before the
IO thread gets to execute this task?

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1806:
audio_input_renderer_host_));
Same as above, it isn't clear how we guarantee lifetime and safety here.

[Henrik Grunell, 2015-08-20 07:26:19]

Code review fixes. Moved getting pid for filename to RPHI.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
render_process_host_(render_process_host),
On 2015/08/19 22:04:15, nasko wrote:
> Why pass the full RenderProcessHost object? It is only used to get an ID and
it
> already has an immutable ID, regardless of which underlying OS process is
> associated with it.

Actually, I think it's better to get the pid in RPHI instead of here so I did
that removed this ptr.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:577:
entry->input_debug_writer.reset();
On 2015/08/19 20:06:34, tommi wrote:
> should be no need for this since you've already called Pass()

Done.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:657:
IntToStringType(base::GetProcId(render_process_host_->GetHandle())));
On 2015/08/19 22:04:15, nasko wrote:
> This is not allowed. From what I can see in this file, this method is called
on
> the IO thread, where render_process_host_ cannot be accessed.

As above comment, this is removed now.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:715:
entry->input_debug_writer.reset();
On 2015/08/19 20:06:34, tommi wrote:
> ditto

Done.

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

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1787:
audio_input_renderer_host_,
On 2015/08/19 22:04:15, nasko wrote:
> How do we guarantee that audio_input_renderer_host_ is not destroyed before
the
> IO thread gets to execute this task?

AudioInputRendererHost is reference counted. Added comment about that.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1806:
audio_input_renderer_host_));
On 2015/08/19 22:04:15, nasko wrote:
> Same as above, it isn't clear how we guarantee lifetime and safety here.

Ditto.

[nasko, 2015-08-20 19:08:56]

Would you please include a overview in the CL description about how this is
supposed to work? It will be useful both for review and for historical context
in the future.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
render_process_host_(render_process_host),
On 2015/08/20 07:26:18, Henrik Grunell wrote:
> On 2015/08/19 22:04:15, nasko wrote:
> > Why pass the full RenderProcessHost object? It is only used to get an ID and
> it
> > already has an immutable ID, regardless of which underlying OS process is
> > associated with it.
> 
> Actually, I think it's better to get the pid in RPHI instead of here so I did
> that removed this ptr.

What happens if the process crashes though? The RPH will stay the same, but the
underlying process will be different next time it is used, so the PID will
change.

https://codereview.chromium.org/1272223003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:24: //#include
"content/public/browser/render_process_host.h"
Remove the line instead of commenting it out.

[Henrik Grunell, 2015-08-20 20:03:50]

Updated CL description.

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
render_process_host_(render_process_host),
On 2015/08/20 19:08:56, nasko wrote:
> On 2015/08/20 07:26:18, Henrik Grunell wrote:
> > On 2015/08/19 22:04:15, nasko wrote:
> > > Why pass the full RenderProcessHost object? It is only used to get an ID
and
> > it
> > > already has an immutable ID, regardless of which underlying OS process is
> > > associated with it.
> > 
> > Actually, I think it's better to get the pid in RPHI instead of here so I
did
> > that removed this ptr.
> 
> What happens if the process crashes though? The RPH will stay the same, but
the
> underlying process will be different next time it is used, so the PID will
> change.

Each time the recording is enabled it constructs the filename including getting
the pid. So it will get the current correct pid. (See RPH)

https://codereview.chromium.org/1272223003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:24: //#include
"content/public/browser/render_process_host.h"
On 2015/08/20 19:08:56, nasko wrote:
> Remove the line instead of commenting it out.

Oops, missed that. Done.

[nasko, 2015-08-20 20:56:14]

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
render_process_host_(render_process_host),
On 2015/08/20 20:03:49, Henrik Grunell wrote:
> On 2015/08/20 19:08:56, nasko wrote:
> > On 2015/08/20 07:26:18, Henrik Grunell wrote:
> > > On 2015/08/19 22:04:15, nasko wrote:
> > > > Why pass the full RenderProcessHost object? It is only used to get an ID
> and
> > > it
> > > > already has an immutable ID, regardless of which underlying OS process
is
> > > > associated with it.
> > > 
> > > Actually, I think it's better to get the pid in RPHI instead of here so I
> did
> > > that removed this ptr.
> > 
> > What happens if the process crashes though? The RPH will stay the same, but
> the
> > underlying process will be different next time it is used, so the PID will
> > change.
> 
> Each time the recording is enabled it constructs the filename including
getting
> the pid. So it will get the current correct pid. (See RPH)

Do we stop recording if the renderer process dies? I don't see it in the diff
here and it isn't obvious that we do.

https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1800: // thread.
AudioInputRendererHost is reference counted, so it's lifetime is
This new comment is better suited before the PostTask call, not this one, since
the PostTaskAndReply doesn't pass the audio_input_renderer_host_ as a parameter.

https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2543: return
file.AddExtension(kAecDumpFileNameAddition);
This seems an overkill to have a method with only one statement. In addition,
the input parameter and output result are the same type.

[Henrik Grunell, 2015-08-21 13:51:03]

nasko@ ptal

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
render_process_host_(render_process_host),
On 2015/08/20 20:56:13, nasko wrote:
> On 2015/08/20 20:03:49, Henrik Grunell wrote:
> > On 2015/08/20 19:08:56, nasko wrote:
> > > On 2015/08/20 07:26:18, Henrik Grunell wrote:
> > > > On 2015/08/19 22:04:15, nasko wrote:
> > > > > Why pass the full RenderProcessHost object? It is only used to get an
ID
> > and
> > > > it
> > > > > already has an immutable ID, regardless of which underlying OS process
> is
> > > > > associated with it.
> > > > 
> > > > Actually, I think it's better to get the pid in RPHI instead of here so
I
> > did
> > > > that removed this ptr.
> > > 
> > > What happens if the process crashes though? The RPH will stay the same,
but
> > the
> > > underlying process will be different next time it is used, so the PID will
> > > change.
> > 
> > Each time the recording is enabled it constructs the filename including
> getting
> > the pid. So it will get the current correct pid. (See RPH)
> 
> Do we stop recording if the renderer process dies? I don't see it in the diff
> here and it isn't obvious that we do.

Yes, then the channel closes and in AudioInputRendererHost::OnChannelClosing all
entries are deleted, i.e. all streams are closed and deleted which stops the
recording.

https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1800: // thread.
AudioInputRendererHost is reference counted, so it's lifetime is
On 2015/08/20 20:56:13, nasko wrote:
> This new comment is better suited before the PostTask call, not this one,
since
> the PostTaskAndReply doesn't pass the audio_input_renderer_host_ as a
parameter.

Done.

https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2543: return
file.AddExtension(kAecDumpFileNameAddition);
On 2015/08/20 20:56:13, nasko wrote:
> This seems an overkill to have a method with only one statement. In addition,
> the input parameter and output result are the same type.

Sure, removed the function.

[Henrik Grunell, 2015-08-22 04:18:23]

On 2015/08/21 13:51:03, Henrik Grunell wrote:
> nasko@ ptal
> 
>
https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):
> 
>
https://codereview.chromium.org/1272223003/diff/60001/content/browser/rendere...
> content/browser/renderer_host/media/audio_input_renderer_host.cc:118:
> render_process_host_(render_process_host),
> On 2015/08/20 20:56:13, nasko wrote:
> > On 2015/08/20 20:03:49, Henrik Grunell wrote:
> > > On 2015/08/20 19:08:56, nasko wrote:
> > > > On 2015/08/20 07:26:18, Henrik Grunell wrote:
> > > > > On 2015/08/19 22:04:15, nasko wrote:
> > > > > > Why pass the full RenderProcessHost object? It is only used to get
an
> ID
> > > and
> > > > > it
> > > > > > already has an immutable ID, regardless of which underlying OS
process
> > is
> > > > > > associated with it.
> > > > > 
> > > > > Actually, I think it's better to get the pid in RPHI instead of here
so
> I
> > > did
> > > > > that removed this ptr.
> > > > 
> > > > What happens if the process crashes though? The RPH will stay the same,
> but
> > > the
> > > > underlying process will be different next time it is used, so the PID
will
> > > > change.
> > > 
> > > Each time the recording is enabled it constructs the filename including
> > getting
> > > the pid. So it will get the current correct pid. (See RPH)
> > 
> > Do we stop recording if the renderer process dies? I don't see it in the
diff
> > here and it isn't obvious that we do.
> 
> Yes, then the channel closes and in AudioInputRendererHost::OnChannelClosing
all
> entries are deleted, i.e. all streams are closed and deleted which stops the
> recording.
> 
>
https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
> content/browser/renderer_host/render_process_host_impl.cc:1800: // thread.
> AudioInputRendererHost is reference counted, so it's lifetime is
> On 2015/08/20 20:56:13, nasko wrote:
> > This new comment is better suited before the PostTask call, not this one,
> since
> > the PostTaskAndReply doesn't pass the audio_input_renderer_host_ as a
> parameter.
> 
> Done.
> 
>
https://codereview.chromium.org/1272223003/diff/100001/content/browser/render...
> content/browser/renderer_host/render_process_host_impl.cc:2543: return
> file.AddExtension(kAecDumpFileNameAddition);
> On 2015/08/20 20:56:13, nasko wrote:
> > This seems an overkill to have a method with only one statement. In
addition,
> > the input parameter and output result are the same type.
> 
> Sure, removed the function.

nasko@ - friendly ping for another review.

[nasko, 2015-08-24 18:19:13]

Sorry for the delay, sheriffing has been surprisingly busy :(.

LGTM

[Henrik Grunell, 2015-08-26 11:10:40]

* Updated the browser test. (And added test for the new file.)
* Fixed incorrect filename when tab opened and recording already enabled.
(Missing renderer pid.) I now give the pid in the AIRH ctor, as well as update
it when needed. (When a channel is connected.)

PTAL.

[tommi (sloooow) - chröme, 2015-08-31 17:19:07]

sorry for the delay. I didn't realize you were asking me to take another look.
(thought you were waiting for nasko)

https://codereview.chromium.org/1272223003/diff/180001/content/browser/media/...
File content/browser/media/webrtc_audio_debug_recordings_browsertest.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/media/...
content/browser/media/webrtc_audio_debug_recordings_browsertest.cc:18: #define
IntToStringType base::IntToString16
nit / pondering: Can this be done with 'using' now?
E.g

using IntToStringType = base::IntToString16;

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.cc:210:
renderer_pid_ = renderer_pid;
Do we need to DCHECK() some state before assigning this value?

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.h:241: //
instabce.
instance

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.h:242: int32
renderer_pid_;
can this be const?

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1625:
base::Bind(&AudioInputRendererHost::set_renderer_pid,
when this happens, does the AudioInputRendererHost need to abort or reset
something if it is already doing something started by a previous process?

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1817: // guarantueed
during the lifetime of the closure.
guaranteed

https://codereview.chromium.org/1272223003/diff/180001/content/renderer/media...
File content/renderer/media/aec_dump_message_filter.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/renderer/media...
content/renderer/media/aec_dump_message_filter.cc:25: delegate_id_counter_(1),
why this change?

[Henrik Grunell, 2015-09-04 12:37:38]

Code review.

nasko@: I had to do some changes since your lgtm. Please review
* content/browser/renderer_host/render_process_host_impl.h/cc
* content/public/browser/render_process_host.h

https://codereview.chromium.org/1272223003/diff/180001/content/browser/media/...
File content/browser/media/webrtc_audio_debug_recordings_browsertest.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/media/...
content/browser/media/webrtc_audio_debug_recordings_browsertest.cc:18: #define
IntToStringType base::IntToString16
On 2015/08/31 17:19:06, tommi wrote:
> nit / pondering: Can this be done with 'using' now?
> E.g
> 
> using IntToStringType = base::IntToString16;

No, can alias types and templates but not functions.

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.cc:210:
renderer_pid_ = renderer_pid;
On 2015/08/31 17:19:07, tommi wrote:
> Do we need to DCHECK() some state before assigning this value?

No. Anything particular in mind?

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.h:241: //
instabce.
On 2015/08/31 17:19:07, tommi wrote:
> instance

Done.

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.h:242: int32
renderer_pid_;
On 2015/08/31 17:19:07, tommi wrote:
> can this be const?

No, see
void set_renderer_pid(int32 renderer_pid);
Must be able to change if a render process is replaced by another using the same
RPH. E.g. if crashed.

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1625:
base::Bind(&AudioInputRendererHost::set_renderer_pid,
On 2015/08/31 17:19:07, tommi wrote:
> when this happens, does the AudioInputRendererHost need to abort or reset
> something if it is already doing something started by a previous process?

No, if this happens the previous channel has been closed and AIRH, which is a
filter, then shuts down what it needs, including recording.

https://codereview.chromium.org/1272223003/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1817: // guarantueed
during the lifetime of the closure.
On 2015/08/31 17:19:07, tommi wrote:
> guaranteed

Done.

https://codereview.chromium.org/1272223003/diff/180001/content/renderer/media...
File content/renderer/media/aec_dump_message_filter.cc (right):

https://codereview.chromium.org/1272223003/diff/180001/content/renderer/media...
content/renderer/media/aec_dump_message_filter.cc:25: delegate_id_counter_(1),
On 2015/08/31 17:19:07, tommi wrote:
> why this change?

I wanted it to have the same start ID as input streams. The stream and AEC dump
delegate IDs don't map to each other, but in most cases people will have one tab
that renders an AEC dump file and an input audio file. Seems odd that one file
would have ".0" and one ".1" in its name.

[Henrik Grunell, 2015-09-04 12:41:25]

On 2015/09/04 12:37:38, Henrik Grunell wrote:
> Code review.
> 
> nasko@: I had to do some changes since your lgtm. Please review
> * content/browser/renderer_host/render_process_host_impl.h/cc
> * content/public/browser/render_process_host.h

And Tommi: the code review fixes was for your comment, ptal.

Super-clearly: Tommi and nasko@, ptal.

[tommi (sloooow) - chröme, 2015-09-04 13:36:03]

lgtm

[nasko, 2015-09-04 18:53:00]

Still LGTM

[tommi (sloooow) - chröme, 2015-09-05 09:38:17]

The CQ bit was checked by tommi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-05 09:38:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272223003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272223003/190001

[commit-bot: I haz the power, 2015-09-05 10:16:04]

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

[commit-bot: I haz the power, 2015-09-05 10:16:05]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Henrik Grunell, 2015-09-06 11:21:13]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-09-06 11:21:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1272223003/#ps210001
(title: "Fixed compile error.")

[commit-bot: I haz the power, 2015-09-06 11:21:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272223003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272223003/210001

[commit-bot: I haz the power, 2015-09-06 11:49:01]

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

[commit-bot: I haz the power, 2015-09-06 11:49:02]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Henrik Grunell, 2015-09-06 15:31:23]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-09-06 15:31:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1272223003/#ps230001
(title: "Fixed build with WebRTC disabled. Rebase.")

[commit-bot: I haz the power, 2015-09-06 15:31:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272223003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272223003/230001

[commit-bot: I haz the power, 2015-09-06 16:30:42]

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

[commit-bot: I haz the power, 2015-09-06 16:30:43]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Henrik Grunell, 2015-09-07 07:35:28]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-09-07 07:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1272223003/#ps250001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-09-07 07:35:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272223003/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272223003/250001

[commit-bot: I haz the power, 2015-09-07 07:39:52]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-09-07 07:40:33]

Patchset 14 (id:??) landed as
https://crrev.com/7566ffc1f76281738648633c1e7e5fc919d70e13
Cr-Commit-Position: refs/heads/master@{#347605}