Merge AssociateStreamWithProducer message into CreateStream message for both audio output and input.

Merge AssociateStreamWithProducer message into CreateStream message for both audio output and input.

Also:

1. Removed RendererAudioOutputDevice class, since it's no longer necessary to manage asynchronously associating a render view with a stream.

2. Clean-ups: Applied several changes/bug fixes to bring the audio input classes/impls up-to-date w.r.t. the audio output classes/impls.  Filed bug 179597 for follow-up.

3. Added missing plumbing for associating audio input devices created by WebRtcAudioCapturer to render views.

4. Updated unit tests.

BUG=166779
TEST=media_unittests, content_unittests, TSAN, chrome/test/functional/webrtc_*.py, and manual testing described in http://codereview.chromium.org/11359196/

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

[DaleCurtis, 2013-03-01 00:27:52]

- I like the removal of |stream_id_| I was just thinking that was ridiculous.
- The interfaces should be changed to accept a scoped_ptr if you want to
transfer ownership.
- Don't nest the class, you shouldn't need to.
- Be very careful around ownership, AudioMessageFilter frequently dies before
AudioOutputDevice does.

https://codereview.chromium.org/12383016/diff/1/content/renderer/media/audio_...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/12383016/diff/1/content/renderer/media/audio_...
content/renderer/media/audio_message_filter.h:63: class AudioOutputIPCImpl;
What's the point of nesting this? Seems it should just be entirely inside the
message filter.

https://codereview.chromium.org/12383016/diff/1/media/audio/audio_output_devi...
File media/audio/audio_output_device.cc (left):

https://codereview.chromium.org/12383016/diff/1/media/audio/audio_output_devi...
media/audio/audio_output_device.cc:243: ipc_ = NULL;
filter_ is destroyed after this is called, so you need to be careful if you're
going to leave ipc_ around. As is, I think this crashes.

[DaleCurtis, 2013-03-01 03:05:12]

I also just realized MessageFilter is ref counted while AudioOutputIPC is not.
You should probably have each instance of your internal class hold a ref to
filter_.

[miu, 2013-03-02 00:24:15]

Hold off on further review until I've polished and completed this change. 
Responses to comments so far...

https://codereview.chromium.org/12383016/diff/1/content/renderer/media/audio_...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/12383016/diff/1/content/renderer/media/audio_...
content/renderer/media/audio_message_filter.h:63: class AudioOutputIPCImpl;
On 2013/03/01 00:27:52, DaleCurtis wrote:
> What's the point of nesting this? Seems it should just be entirely inside the
> message filter.

I needed it to have access to private members (the Send() method, and delegates_
IDMap).  I could also just "friend" the class.  I feel that making it a nested
class better communicates the structure of the program, but it wouldn't kill me
to change it if you strongly prefer some other way.

https://codereview.chromium.org/12383016/diff/1/media/audio/audio_output_devi...
File media/audio/audio_output_device.cc (left):

https://codereview.chromium.org/12383016/diff/1/media/audio/audio_output_devi...
media/audio/audio_output_device.cc:243: ipc_ = NULL;
On 2013/03/01 00:27:52, DaleCurtis wrote:
> filter_ is destroyed after this is called, so you need to be careful if you're
> going to leave ipc_ around. As is, I think this crashes.

That's why I added the comment in the header file "Only valid when state_ !=
IPC_CLOSED" and I made sure the code is following that.  All said, I will be
doing VERY CAREFUL testing here.  I'm aware how sensitive and exposed this code
is.  ;-)

[miu, 2013-03-05 05:44:25]

Dale,

All set for review.  This looks far worse than it really is (most files are just
trivial, small changes).  Change description explains "extra things" that were
done, in addition to just merging the messaging/IPC code.

Thanks,
Yuri

[miu, 2013-03-05 05:45:45]

palmer: Please review IPC messaging changes in
content/common/media/audio_messages.h.

xians: Please review webrtc-related changes: content/renderer/media/webrtc* and
*media_stream_dependency_factory*

Thanks,
Yuri

[no longer working on chromium, 2013-03-05 18:33:09]

It looks great, I have some nits and two questions. lgtm on the webrtc code.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:14: namespace {
nit, add an empty line.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:20: }
ditto

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:180: :
render_view_id_(render_view_id), stream_id_(kStreamIDNotSet) {}
nit, one line for each member initialization.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:161: case AudioInputIPCDelegate::kError:
What are we expected to do here when getting a error callback?
Any possibility that the stream is torn down?

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:203: state_ = IPC_CLOSED;
why do we need this IPC_CLOSED state? Can we simply put it IDLE here?

[palmer, 2013-03-05 21:09:32]

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_renderer_host.cc:239: uint32
mem_size = sizeof(media::AudioInputBufferParameters) + buffer_size;
This addition could overflow; buffer_size comes from an untrustworthy source.
Check that buffer_size is sane before doing this addition.

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_renderer_host.h:125: // this
call is successful this object would keep an internal entry of the
Would keep, or does keep?

How does the caller know if the call was successful?

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:275: int
output_memory_size = AudioBus::CalculateMemorySize(audio_params);
This should be an unsigned type (size_t or uint32).

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:278: int
input_memory_size =
This should be an unsigned type (size_t or uint32).

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:286: uint32
io_buffer_size = output_memory_size + input_memory_size;
How do we know this addition won't overflow? |output_memory_size| and
|input_memory_size| should be constrained to sane values, preferably values that
when added cannot overflow a uint32. (Otherwise you have to manually check for
overflow).

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.h:111: // If this call
is successful this object would keep an internal entry of the
Same comment as before: Probably should says "does keep", and how do callers
know if the call was successful?

https://codereview.chromium.org/12383016/diff/6001/content/common/media/audio...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/common/media/audio...
content/common/media/audio_messages.h:112: std::string /* device_id */,
Does this really need to be a string? What form do device IDs take? It's always
better to send a more constrained type, and not to make the other side do any
parsing.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:23: // Creates a AudioOutputDevice
using the currently registered factory.
NIT: Creates "an"

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:31: const std::string&
device_id,
It'd be better to constrain this type, rather than using string.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:126: // shit applies here.
This would be the first naughty word I've seen in Chromium code. :) Maybe leave
it out?

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.h (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.h:120: // Note: The ordering of members in this
enum is critical to correct behavior!
Maybe therefore assign them explicit values (IPC_CLOSED = 0, et c.).

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_ipc.h
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_ip...
media/audio/audio_input_ipc.h:73: const std::string& device_id,
Same, no string.

[DaleCurtis, 2013-03-05 23:29:54]

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_renderer_host.cc:217:
DCHECK_LT(0, render_view_id);
Yoda is persona non grata in Chrome style.

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_renderer_host.cc:239: uint32
mem_size = sizeof(media::AudioInputBufferParameters) + buffer_size;
On 2013/03/05 21:09:32, Chris P. wrote:
> This addition could overflow; buffer_size comes from an untrustworthy source.
> Check that buffer_size is sane before doing this addition.

This is sanitized in media_param_traits by checking AudioParameters::IsValid().

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:261: DCHECK_LT(0,
render_view_id);
Drive your Smart Car elsewhere. http://what-if.xkcd.com/3/

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:286: uint32
io_buffer_size = output_memory_size + input_memory_size;
On 2013/03/05 21:09:32, Chris P. wrote:
> How do we know this addition won't overflow? |output_memory_size| and
> |input_memory_size| should be constrained to sane values, preferably values
that
> when added cannot overflow a uint32. (Otherwise you have to manually check for
> overflow).

Same AudioParameters verification ensures CalculateMemorySize is sane.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:19:
AudioInputMessageFilter* g_filter = NULL;
This should be a scoped_refptr() since IPC::ChannelProxy::MessageFilter is ref
counted.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:53:
CHECK(delegates_.IsEmpty());
This can't be done here since the message filter is not destructed on the IO
thread.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:59:
AudioInputMessageFilter* AudioInputMessageFilter::Get() {
Can we remove the singleton aspects in favor of scoped_refptr() and requesting
the MessageFilter from the RenderThread?

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:19: AudioMessageFilter* g_filter
= NULL;
Same comments as the input message filter.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:119: DCHECK_LT(0, length);
/fist-shake.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:126: // shit applies here.
On 2013/03/05 21:09:32, Chris P. wrote:
> This would be the first naughty word I've seen in Chromium code. :) Maybe
leave
> it out?

Damn straight.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_ipc.h
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_ip...
media/audio/audio_input_ipc.h:73: const std::string& device_id,
On 2013/03/05 21:09:32, Chris P. wrote:
> Same, no string.

I think this is a change for a separate CL if it ends up being done.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_output_d...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_output_d...
media/audio/audio_output_device.cc:183: case AudioOutputIPCDelegate::kError:
Technically this isn't really implemented either :) I believe all of these calls
fizzle out somewhere along the line :p

[miu, 2013-03-06 19:25:41]

xians: Addressed your comments.  Will upload code changes after addressing other
reviewers' comments.  Thanks for the review!  :)

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:14: namespace {
On 2013/03/05 18:33:10, xians1 wrote:
> nit, add an empty line.

Done.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:20: }
On 2013/03/05 18:33:10, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:180: :
render_view_id_(render_view_id), stream_id_(kStreamIDNotSet) {}
On 2013/03/05 18:33:10, xians1 wrote:
> nit, one line for each member initialization.

Half the reviewers want fewer LOC, and half want more.  ;-)  Happy to do it
either way...

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:161: case AudioInputIPCDelegate::kError:
On 2013/03/05 18:33:10, xians1 wrote:
> What are we expected to do here when getting a error callback?
> Any possibility that the stream is torn down?

Hmm...Yeah, these "OnStateChanged" IPCs (both input and output side) need some
hefty clean-up as well.  Sigh.

In answer to your question, the only implementation of
callback_->OnCaptureError() is in WebRtcAudioCapturer, and it just has a
NOTIMPLEMENTED() statement.  So, absolutely nothing happens here!  :)

On the browser side, in AudioInputRendererHost, it looks like there is
inconsistent handling of errors: In some cases, this message is sent and nothing
else happens.  In other cases, this message is sent and then the stream is
immediately forced closed.

Filed bug to address this stuff: http://crbug.com/180640

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:203: state_ = IPC_CLOSED;
On 2013/03/05 18:33:10, xians1 wrote:
> why do we need this IPC_CLOSED state? Can we simply put it IDLE here?

See header file comments for the enum.  IDLE indicates ipc_ is valid and a
stream can be created/started.  IPC_CLOSED indicates ipc_ is invalid and the
whole AudioInputDevice instance is basically dead now.

[miu, 2013-03-06 22:36:52]

Addressed all reviewers' comments.  PTAL.

Thanks,
Yuri

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_renderer_host.cc:217:
DCHECK_LT(0, render_view_id);
On 2013/03/05 23:29:54, DaleCurtis wrote:
> Yoda is persona non grata in Chrome style.

Gtest semantics, I blame.  Fixed all of these, I have.

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_input_renderer_host.h:125: // this
call is successful this object would keep an internal entry of the
On 2013/03/05 21:09:32, Chris P. wrote:
> Would keep, or does keep?
> 
> How does the caller know if the call was successful?

Clarified the comment.

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:275: int
output_memory_size = AudioBus::CalculateMemorySize(audio_params);
On 2013/03/05 21:09:32, Chris P. wrote:
> This should be an unsigned type (size_t or uint32).

I agree with you.  But, the style guide generally bans use of the unsigned
types.  This could be an exception case (use of size_t here?).  If you think
this should be changed, please add a note to http://crbug.com/120319.

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.h:111: // If this call
is successful this object would keep an internal entry of the
On 2013/03/05 21:09:32, Chris P. wrote:
> Same comment as before: Probably should says "does keep", and how do callers
> know if the call was successful?

Done.

https://codereview.chromium.org/12383016/diff/6001/content/common/media/audio...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/common/media/audio...
content/common/media/audio_messages.h:112: std::string /* device_id */,
On 2013/03/05 21:09:32, Chris P. wrote:
> Does this really need to be a string? What form do device IDs take? It's
always
> better to send a more constrained type, and not to make the other side do any
> parsing.

Absolutely 100% agree with you.  See the TODO comment a few lines above.  We
actually want to remove it altogether.  ;-)

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:23: // Creates a AudioOutputDevice
using the currently registered factory.
On 2013/03/05 21:09:32, Chris P. wrote:
> NIT: Creates "an"

Done.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:19:
AudioInputMessageFilter* g_filter = NULL;
On 2013/03/05 23:29:54, DaleCurtis wrote:
> This should be a scoped_refptr() since IPC::ChannelProxy::MessageFilter is ref
> counted.

Done.  Removed the global pointer scheme in Audio*MessageFilter.  All accesses
are through RenderThreadImpl, and ref-counted pointers maintained where needed.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:53:
CHECK(delegates_.IsEmpty());
On 2013/03/05 23:29:54, DaleCurtis wrote:
> This can't be done here since the message filter is not destructed on the IO
> thread.

Added comment in code.  Explanation: When the destructor is called, there should
be no references outstanding (ref-count is zero).  That means there shouldn't be
code on any other thread attempting to reference delegates_.

BTW--This particular LOC was changed to make this match ~AudioMessageFilter().

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_input_message_filter.cc:59:
AudioInputMessageFilter* AudioInputMessageFilter::Get() {
On 2013/03/05 23:29:54, DaleCurtis wrote:
> Can we remove the singleton aspects in favor of scoped_refptr() and requesting
> the MessageFilter from the RenderThread?

Agreed and done.

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:19: AudioMessageFilter* g_filter
= NULL;
On 2013/03/05 23:29:54, DaleCurtis wrote:
> Same comments as the input message filter.

Done.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:119: DCHECK_LT(0, length);
On 2013/03/05 23:29:54, DaleCurtis wrote:
> /fist-shake.

With a cabasa, maybe?

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:126: // shit applies here.
On 2013/03/05 21:09:32, Chris P. wrote:
> This would be the first naughty word I've seen in Chromium code. :) Maybe
leave
> it out?

Whoops.  I meant to, um, *elaborate* on this.  ;-)

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
File media/audio/audio_input_device.h (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_de...
media/audio/audio_input_device.h:120: // Note: The ordering of members in this
enum is critical to correct behavior!
On 2013/03/05 21:09:32, Chris P. wrote:
> Maybe therefore assign them explicit values (IPC_CLOSED = 0, et c.).

C++ spec requires enum values assigned as 0, 1, 2, ..., when not specified. 
However, I still agree with you about needing something safer than just a
comment to protect the validity of the code.  Therefore, I've added
COMPILE_ASSERTs in the constructor for both AudioOutputDevice and
AudioInputDevice.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_ipc.h
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_input_ip...
media/audio/audio_input_ipc.h:73: const std::string& device_id,
On 2013/03/05 23:29:54, DaleCurtis wrote:
> On 2013/03/05 21:09:32, Chris P. wrote:
> > Same, no string.
> 
> I think this is a change for a separate CL if it ends up being done.

Definitely falls within the scope of http://crbug.com/179597.

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_output_d...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/12383016/diff/6001/media/audio/audio_output_d...
media/audio/audio_output_device.cc:183: case AudioOutputIPCDelegate::kError:
On 2013/03/05 23:29:54, DaleCurtis wrote:
> Technically this isn't really implemented either :) I believe all of these
calls
> fizzle out somewhere along the line :p

Yep.  http://crbug.com/180640

[palmer, 2013-03-06 23:11:59]

IPC security LGTM. Looking forward to more fixes. :)

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/12383016/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_renderer_host.cc:275: int
output_memory_size = AudioBus::CalculateMemorySize(audio_params);
> I agree with you.  But, the style guide generally bans use of the unsigned
> types.  This could be an exception case (use of size_t here?).  If you think
> this should be changed, please add a note to http://crbug.com/120319.

Yeah, this is an area where the style guide is simply wrong, unfortunately.
Well, partly wrong. "int" means "integer" and the use of unsigned integers for
integers whose value you never expect to be < 0 is arguable.

But in any case, "size_t" means "this is a size or offset", which is clearly
correct; and in any case, integer overflow behavior is undefined for signed
integers but it is defined for unsigned integer types (all, not just size_t). So
for security and correctness reasons, where integer overflow can happen, an
unsigned type (usually size_t) is correct and the style guide's dictum does not
apply in those cases.

I'll comment on that bug, thanks for the pointer.

https://codereview.chromium.org/12383016/diff/6001/content/common/media/audio...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/12383016/diff/6001/content/common/media/audio...
content/common/media/audio_messages.h:112: std::string /* device_id */,
> Absolutely 100% agree with you.  See the TODO comment a few lines above.  We
> actually want to remove it altogether.  ;-)

Ok. I'll be your eager LGTMer for IPC security on that review. :)

[DaleCurtis, 2013-03-07 21:02:58]

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_device_factory.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_device_factory.cc:30:
RenderThreadImpl::current()->audio_message_filter();
I don't think this works? AudioRendererMixerManager will call this method from
the MediaPipeline thread and current() is only usable on the render thread.

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.cc:50:
CHECK(delegates_.IsEmpty());
This may be safe, but will fail the DCHECK(CalledOnValidThread()) in IDMap which
is why I removed it originally. You'd never know though since apparently we
don't have any tests which destruct RenderThreadImpls after a delegate has been
added. :)

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.cc:217: DCHECK_NE(stream_id_,
kStreamIDNotSet);
Instead of DCHECK should these methods just fire a NOTREACHED() and return if
kStreamIDNotSet is encountered?

That said it looks like AudioInputRendererHost properly aborts if an unknown
stream_id is encountered.

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_message_filter.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
All the same comments as the input message filter.

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_renderer_mixer_manager.cc:81: params,
output_params, AudioDeviceFactory::NewOutputDevice(
FYI: this is the part called on the MediaPipeline thread, not the RenderThread.

[miu, 2013-04-16 00:07:35]

Dale: Getting back to this, finally!  ;-)  We were very close.  I had to rebase
1+ month's worth of changes.  I put this all through rigorous testing again, so
we should be good to go.

PTAL.

Thanks,
Yuri

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_device_factory.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_device_factory.cc:30:
RenderThreadImpl::current()->audio_message_filter();
On 2013/03/07 21:02:58, DaleCurtis wrote:
> I don't think this works? AudioRendererMixerManager will call this method from
> the MediaPipeline thread and current() is only usable on the render thread.

I dedcided to put the AudioMessageFilter::Get() static method back in.  This
change is big enough that I don't want to also start refactoring to solve this
rather complex "create from any thread" problem.  ;-)

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.cc:50:
CHECK(delegates_.IsEmpty());
On 2013/03/07 21:02:58, DaleCurtis wrote:
> This may be safe, but will fail the DCHECK(CalledOnValidThread()) in IDMap
which
> is why I removed it originally. You'd never know though since apparently we
> don't have any tests which destruct RenderThreadImpls after a delegate has
been
> added. :)

Oh, duh.  I was looking at the IDMap code and saw the DetachFromThread() call in
~IDMap(), but we're calling IDMap::IsEmtpy() here.  Okay, I think we can safely
do without the CHECK here.

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.cc:217: DCHECK_NE(stream_id_,
kStreamIDNotSet);
On 2013/03/07 21:02:58, DaleCurtis wrote:
> Instead of DCHECK should these methods just fire a NOTREACHED() and return if
> kStreamIDNotSet is encountered?

Same thing, right?  (see base/logging.h:752)

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/12383016/diff/24001/content/renderer/media/au...
content/renderer/media/audio_message_filter.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/03/07 21:02:58, DaleCurtis wrote:
> All the same comments as the input message filter.

Done.

[DaleCurtis, 2013-04-16 20:35:04]

lgtm. Thanks Yuri!

[miu, 2013-04-16 21:13:17]

avi: Need OWNERs approval for content/content_renderer.gypi

yzshen: Need OWNERs approval for content/renderer/pepper/*

Thanks,
Yuri

[yzshen1, 2013-04-17 05:27:23]

lgtm

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:144: CHECK(ipc_);
[optional] shall we return false here instead of CHECK()?

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.h:95:
scoped_ptr<media::AudioInputIPC> ipc_;
- nit: please include scoped_ptr.h.
- does the 'except' part of the comment still make sense? (I don't know it well
enough, but I don't see it is used to send messages or get message loop.)

[miu, 2013-04-17 20:01:38]

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:144: CHECK(ipc_);
On 2013/04/17 05:27:23, yzshen1 wrote:
> [optional] shall we return false here instead of CHECK()?

Actually, the CreateAudioIPC() method should never return NULL.  I've removed
the CHECK, since it's really unnecessary (other callers don't feel the need to
check for NULL).

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

https://codereview.chromium.org/12383016/diff/55001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.h:95:
scoped_ptr<media::AudioInputIPC> ipc_;
On 2013/04/17 05:27:23, yzshen1 wrote:
> - nit: please include scoped_ptr.h.
> - does the 'except' part of the comment still make sense? (I don't know it
well
> enough, but I don't see it is used to send messages or get message loop.)

Done.

[miu, 2013-04-17 20:04:19]

avi: I put you on TBR so I can commit (just needed your OWNERS approval for a
trivial GYPI file change).  A lot of people are actively working on the files
I'm changing, so I want to avoid unnecessary merge pain by waiting any longer. 
;-)

Thanks,
Yuri

[Avi (use Gerrit), 2013-04-17 20:14:37]

lgtm

Out of my expertise, stamping it as it looks reasonable.

[miu, 2013-04-17 23:09:50]

Committed patchset #9 manually as r194711 (presubmit successful).

[henrika (OOO until Aug 14), 2013-04-18 08:12:40]

Thanks Yuri, and sorry for not adding comments. For some strange reason I was
not notified until the patch was committed.

LGTM ;-)