PepperPlatformAudioInputImpl: support audio input device selection.

PepperPlatformAudioInputImpl: support audio input device selection.

BUG=None
TEST=ppapi/examples/audio_input


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128110

[yzshen1, 2012-03-15 08:28:48]

Hi, Henrik, Wei and Trung.

Would you please take a look? Thanks!

Henrik and Wei: Please review the way how audio input devices are used.
Trung: Please review Pepper-related changes.

[henrika (OOO until Aug 14), 2012-03-15 12:10:32]

Yuzhu,

Shijing knows the details about media-stream better than I do. I've moved myself
to CC and added Shijing as reviewer instead.

[yzshen, 2012-03-15 17:36:54]

Hi, Henrik.

Please build chrome and the audio input example
(ppapi_example_audio_input). And then run the following command to launch
chrome (on Windows):
chrome
--register-pepper-plugins="path\to\your\chrome\src\build\Debug\ppapi_example_audio_input.dll;application/x-ppapi-example-audio-input"
--ppapi-out-of-process
"path\to\your\chrome\src\ppapi\examples\audio_input\audio_input.html"

(Please change target path and name accordingly on Linux.)

On the page, there are links that you can choose to open devices. Once a
device is selected, the audio input data will be displayed as a curve. And
there are Stop / Start links on the page to stop and re-start capturing.

Please note that re-start doesn't work for now. Because
AudioInputHostMsg_CloseStream is issued when the user chooses to stop
capturing. There is no IPC message to ask the browser side to pause
capturing (right?).
This is not an issue introduced by this CL. I will work on a separate CL to
fix it.

Thanks!

On Thu, Mar 15, 2012 at 5:15 AM, <henrika@chromium.org> wrote:

> I am new to Pepper. Could you please add some details on what's required
> to try
> the HTML tests and what's the expected result? Would be fun to try it out.
>
>
http://codereview.chromium.**org/9705056/<http://codereview.chromium.org/9705...
>



-- 
Best regards,
Yuzhu Shen.

[no longer working on chromium, 2012-03-15 17:47:37]

Hi Yuzhu,

I did not have time to take a look at it today, but will do it tomorrow, hope it
is fine.

Just a thought, the pepper_platform_audio_input_impl.cc looks pretty similar to
audio_input_device.cc, I believe it will be simplied a lot if we switch to use
audio_input_device.cc instead, another benefit is that we don't need to maintain
two implementation.
You may have already known, html5, webaudio, webrtc are all using the
audio_device class, so it will be just great if we all use one audio path.

I will try out the HTML test too, thanks.

[yzshen, 2012-03-15 18:07:00]

On Thu, Mar 15, 2012 at 10:47 AM, <xians@chromium.org> wrote:

> Hi Yuzhu,
>
> I did not have time to take a look at it today, but will do it tomorrow,
> hope it
> is fine.

I wish I could land it before M19 branching (one week from now). So
tomorrow is fine. Thanks a lot!


> Just a thought, the pepper_platform_audio_input_**impl.cc looks pretty
> similar to
> audio_input_device.cc, I believe it will be simplied a lot if we switch to
> use
> audio_input_device.cc instead, another benefit is that we don't need to
> maintain
> two implementation.
> You may have already known, html5, webaudio, webrtc are all using the
> audio_device class, so it will be just great if we all use one audio path.
>
I will take a closer look. Thanks for the suggestion!

If we want to go down this direction, at least we need to do some
refactoring: audio_input_device should be able to pass the sync socket and
shared memory handle to its user, instead of creating a thread and calling
callback on it. This is necessary for pepper, because we have to pass the
handles to the plugin processes.




>
> I will try out the HTML test too, thanks.
>
>
>
http://codereview.chromium.**org/9705056/<http://codereview.chromium.org/9705...
>



-- 
Best regards,
Yuzhu Shen.

[wjia(left Chromium), 2012-03-15 23:41:34]

lgtm on media_stream_dispatcher.cc.

I will leave audio_input_device stuff to xians@ since he is more familiar with
that area.

[no longer working on chromium, 2012-03-16 14:33:23]

It looks quite good, we are almost there.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:21:
shutdown_called_(false) {
why do we need a new shutdown_called flag? can we use stream_id_ instead?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:52: bool
PepperPlatformAudioInputImpl::StartCapture() {
It looks like this function has no way to return false, change it to void?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:61: bool
PepperPlatformAudioInputImpl::StopCapture() {
void?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:81: bool
PepperPlatformAudioInputImpl::Initialize(
void?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:144:
filter_->Send(new AudioInputHostMsg_RecordStream(stream_id_));
What happens if we are in the middle of starting the device, while the stream
has not been created, maybe we should not send AudioInputHostMsg_RecordStream in
this case.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:153:
filter_->Send(new AudioInputHostMsg_CloseStream(stream_id_));
same situation here, we need to do similar things as in audio_input_device.cc to
handle two possibilities.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:175: Release();  //
Release for the delegate, balances out the reference taken in
What does this Release() do? We execute CloseDevice after Release(), is it
intentional?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:191: 
Which thread does this OnStreamCreated() happen? IO thread?
If it does, we can check the stream_id_, and free the handles if it is 0, then
we don't need to post a task to the main thread.

What about making it more readable by:
if (base::MessageLoopProxy::current() != main_message_loop_proxy_) {
  main_message_loop_proxy_->PostTask(
      FROM_HERE,
      base::Bind(&PepperPlatformAudioInputImpl::OnStreamCreated, this, handle,
socket_handle, length));
  return;
}

// it looks like client_ is not thread safe anyway, consider a lock here?
if (client_) {
  ....

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:259:
NotifyStreamCreationFailed();
shall we still send the notification when plugin_delegate_ == NULL?

[no longer working on chromium, 2012-03-16 14:35:11]

Monday and Tuesday are the dates for our skiing trip, so I will try to take a
second look during the weekend if my comments have been addressed.

[viettrungluu, 2012-03-16 22:29:08]

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:18: class
PepperPluginDelegateImpl;
Alphabetical order?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:86: // THIS MUST ONLY
BE ACCESSED ON THE MAIN THREAD.
Is the thread usage of this class documented somewhere?

Also, it might be helpful to implement appropriate getters/setters that DCHECK()
the thread.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:89: // The label of
the opened device. THIS MUST ONLY BE ACCESSED ON THE MAIN
What exactly is a "label"? (The human-readable name? In what encoding?)

http://codereview.chromium.org/9705056/diff/1/webkit/plugins/ppapi/plugin_del...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9705056/diff/1/webkit/plugins/ppapi/plugin_del...
webkit/plugins/ppapi/plugin_delegate.h:199: // The (interface for the) client
used by |PlatformAudio| and
Probably this comment needs to be updated. Maybe it should be
PlatformAudioClientBase (and PlatformAudioOutputClient made a trivial subclass
of it, or something).

[yzshen1, 2012-03-16 22:36:47]

On 2012/03/16 14:35:11, xians1 wrote:
> Monday and Tuesday are the dates for our skiing trip, so I will try to take a
> second look during the weekend if my comments have been addressed.

Thanks, Shijing!
I will upload a new patch set today.

[yzshen1, 2012-03-17 09:32:56]

Hi, Shijing.

Please take another look. Thanks!

(Haven't addressed issues that Trung raised. Will do that soon.)

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:21:
shutdown_called_(false) {
On 2012/03/16 14:33:23, xians1 wrote:
> why do we need a new shutdown_called flag? can we use stream_id_ instead?

ShutDown() may be called before stream_id_ is set to non-zero. This is possible
because there is an async step before initializing, i.e., OpenDevice ->
OnDeviceOpened.

So we need this extra variable.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:52: bool
PepperPlatformAudioInputImpl::StartCapture() {
On 2012/03/16 14:33:23, xians1 wrote:
> It looks like this function has no way to return false, change it to void?

Done.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:61: bool
PepperPlatformAudioInputImpl::StopCapture() {
On 2012/03/16 14:33:23, xians1 wrote:
> void?

Done.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:81: bool
PepperPlatformAudioInputImpl::Initialize(
I changed two DCHECKs to input parameter validation, so I kept the bool return
value.

On 2012/03/16 14:33:23, xians1 wrote:
> void?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:144:
filter_->Send(new AudioInputHostMsg_RecordStream(stream_id_));
Thanks for pointing this out!

This class doesn't maintain state to enforce the order of those public
operations: StartCapture/StopCapture/Shutdown.

ppb_audio_input_shared.cc maintains a simple state machine which makes sure
PepperPlatformAudioInputImpl is used in a reasonable way. So I added the check
there.

(Besides, it won't do any harm if a RecordStream is sent before the device is
started.)

On 2012/03/16 14:33:23, xians1 wrote:
> What happens if we are in the middle of starting the device, while the stream
> has not been created, maybe we should not send AudioInputHostMsg_RecordStream
in
> this case.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:153:
filter_->Send(new AudioInputHostMsg_CloseStream(stream_id_));
Please see my comment above.
On 2012/03/16 14:33:23, xians1 wrote:
> same situation here, we need to do similar things as in audio_input_device.cc
to
> handle two possibilities.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:175: Release();  //
Release for the delegate, balances out the reference taken in
This is a ref-counted object, Release() decreases the ref count by 1.

CloseDevice() has to be executed on the main thread, so a task is posted. It
will be safe to execute it after Release(), because Bind will maintain ref
count, too.

On 2012/03/16 14:33:23, xians1 wrote:
> What does this Release() do? We execute CloseDevice after Release(), is it
> intentional?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:191: 
On 2012/03/16 14:33:23, xians1 wrote:
> Which thread does this OnStreamCreated() happen? IO thread?
Yes. And then we will post a task (using the same method) to the main thread.

> If it does, we can check the stream_id_, and free the handles if it is 0, then
> we don't need to post a task to the main thread.
Yes. That will work. But we still need to check client_ and do clean up on the
main thread if it is NULL. Because ShutDown may be called after the task was
posted but before it gets processed on the main thread.

> 
> What about making it more readable by:
> if (base::MessageLoopProxy::current() != main_message_loop_proxy_) {
>   main_message_loop_proxy_->PostTask(
>       FROM_HERE,
>       base::Bind(&PepperPlatformAudioInputImpl::OnStreamCreated, this, handle,
> socket_handle, length));
>   return;
> }
Okay. I changed the condition and switched the two branches.
 
> // it looks like client_ is not thread safe anyway, consider a lock here?
> if (client_) {
>   ....
client_ is never accessed on threads other than the main thread, so we don't
need a lock.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:259:
NotifyStreamCreationFailed();
On 2012/03/16 14:33:23, xians1 wrote:
> shall we still send the notification when plugin_delegate_ == NULL?

As long as client_ is not NULL, I think it makes sense to send the notification.
(And that is what the current code does.)

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:18: class
PepperPluginDelegateImpl;
I thought 'class' should be prior to 'struct'.

On 2012/03/16 22:29:09, viettrungluu wrote:
> Alphabetical order?

[viettrungluu, 2012-03-17 23:52:22]

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:18: class
PepperPluginDelegateImpl;
Most places I've seen alphabetize based on the name, ignoring "class" or
"struct". <shrug>

On 2012/03/17 09:32:56, yzshen1 wrote:
> I thought 'class' should be prior to 'struct'.
> 
> On 2012/03/16 22:29:09, viettrungluu wrote:
> > Alphabetical order?
>

[no longer working on chromium, 2012-03-18 00:38:59]

My second round. To save time, lgtm if my comments are addressed again.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:21:
shutdown_called_(false) {
On 2012/03/17 09:32:56, yzshen1 wrote:
> On 2012/03/16 14:33:23, xians1 wrote:
> > why do we need a new shutdown_called flag? can we use stream_id_ instead?
> 
> ShutDown() may be called before stream_id_ is set to non-zero. This is
possible
> because there is an async step before initializing, i.e., OpenDevice ->
> OnDeviceOpened.
> 
> So we need this extra variable.

Do I miss something here? It looks to me they all are on IO thread. If it is
true, they are synchronous. Would you please double check?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:144:
filter_->Send(new AudioInputHostMsg_RecordStream(stream_id_));
On 2012/03/17 09:32:56, yzshen1 wrote:
> Thanks for pointing this out!
> 
> This class doesn't maintain state to enforce the order of those public
> operations: StartCapture/StopCapture/Shutdown.
> 
> ppb_audio_input_shared.cc maintains a simple state machine which makes sure
> PepperPlatformAudioInputImpl is used in a reasonable way. So I added the check
> there.
> 
> (Besides, it won't do any harm if a RecordStream is sent before the device is
> started.)
> 
> On 2012/03/16 14:33:23, xians1 wrote:
> > What happens if we are in the middle of starting the device, while the
stream
> > has not been created, maybe we should not send
AudioInputHostMsg_RecordStream
> in
> > this case.
> 

Thanks for clarification.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:175: Release();  //
Release for the delegate, balances out the reference taken in
On 2012/03/17 09:32:56, yzshen1 wrote:
> This is a ref-counted object, Release() decreases the ref count by 1.
> 
> CloseDevice() has to be executed on the main thread, so a task is posted. It
> will be safe to execute it after Release(), because Bind will maintain ref
> count, too.
> 
> On 2012/03/16 14:33:23, xians1 wrote:
> > What does this Release() do? We execute CloseDevice after Release(), is it
> > intentional?
> 

True, I know it is safe due to PepperPlatformAudioInputImpl is ref-counted. I
asked the question because we come from Shutdown() in main thread, then here we
post a task back to main thread. Can we just call CloseDevice in Shutdown()? Or
they have to be done in this way?

http://codereview.chromium.org/9705056/diff/11001/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/9705056/diff/11001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:242: if (succeeded)
{
it is more clear to be
if (succeeded && plugin_delegate_)

http://codereview.chromium.org/9705056/diff/11001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:244: 
add a DCHECK(!label_.empty()) ?

http://codereview.chromium.org/9705056/diff/11001/ppapi/examples/audio_input/...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/9705056/diff/11001/ppapi/examples/audio_input/...
ppapi/examples/audio_input/audio_input.cc:236: if (!audio_input_.StartCapture())
hasn't this function been changed to void?

[yzshen1, 2012-03-19 21:47:54]

Thanks, Trung and Shijing.

Please take another look.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:21:
shutdown_called_(false) {
On 2012/03/18 00:38:59, xians1 wrote:
> On 2012/03/17 09:32:56, yzshen1 wrote:
> > On 2012/03/16 14:33:23, xians1 wrote:
> > > why do we need a new shutdown_called flag? can we use stream_id_ instead?
> > 
> > ShutDown() may be called before stream_id_ is set to non-zero. This is
> possible
> > because there is an async step before initializing, i.e., OpenDevice ->
> > OnDeviceOpened.
> > 
> > So we need this extra variable.
> 
> Do I miss something here? It looks to me they all are on IO thread.
OpenDevice() and OnDeviceOpened() happens on the main thread (as you can see,
there are DCHECKs to guarantee that.)

> If it is true, they are synchronous. Would you please double check?
I don't understand what do you mean by "they are synchronous". Would you please
explain?

The following example illustrates what I was trying to say:
- [on main thread] Initialize() is called, which calls
plugin_delegate_->OpenDevice() (assuming device_id is not empty).
- [on main thread] ShutDown() is called, which posts a task to run
ShutDownOnIOThread().
- [on main thread] OnDeviceOpened() is called, which posts a task to run
InitializeOnIOThread().
- [on IO thread] ShutDownOnIOThread() is called, which sets shutdown_called_ to
true.
- [on IO thread] InitializeOnIOThread() is called. Please note that we cannot
use stream_id_ here to determine whether shutdown has occurred, because
stream_id_ is generated in _this_ method. That is why we need shutdown_called_.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:175: Release();  //
Release for the delegate, balances out the reference taken in
On 2012/03/18 00:38:59, xians1 wrote:
> On 2012/03/17 09:32:56, yzshen1 wrote:
> > This is a ref-counted object, Release() decreases the ref count by 1.
> > 
> > CloseDevice() has to be executed on the main thread, so a task is posted. It
> > will be safe to execute it after Release(), because Bind will maintain ref
> > count, too.
> > 
> > On 2012/03/16 14:33:23, xians1 wrote:
> > > What does this Release() do? We execute CloseDevice after Release(), is it
> > > intentional?
> > 
> 
> True, I know it is safe due to PepperPlatformAudioInputImpl is ref-counted. I
> asked the question because we come from Shutdown() in main thread, then here
we
> post a task back to main thread. Can we just call CloseDevice in Shutdown()?
Or
> they have to be done in this way?

I try to maintain the following order:
open device -> start device -> create stream -> record stream -> close stream ->
close device
Because "open device" comes the first, I would like to make sure "close device"
comes the last.

I haven't investigated whether there are any issues if "close device" is called
prior to "close stream". I will take a closer look at the browser side code.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:18: class
PepperPluginDelegateImpl;
On 2012/03/17 23:52:22, viettrungluu wrote:
> Most places I've seen alphabetize based on the name, ignoring "class" or
> "struct". <shrug>
> 
> On 2012/03/17 09:32:56, yzshen1 wrote:
> > I thought 'class' should be prior to 'struct'.
> > 
> > On 2012/03/16 22:29:09, viettrungluu wrote:
> > > Alphabetical order?
> > 
> 

Done.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:86: // THIS MUST ONLY
BE ACCESSED ON THE MAIN THREAD.
On 2012/03/16 22:29:09, viettrungluu wrote:
> Is the thread usage of this class documented somewhere?
Done. I added a threading comment for this class at the top.

> 
> Also, it might be helpful to implement appropriate getters/setters that
DCHECK()
> the thread.
Adding getters/setters bloats the class definition a little bit. Since all these
data members are private and they are clearly commented, I think it is okay to
not use getters/setters.
(Besides, all methods are DCHECKed to make sure they are used on the right
thread.)

What do you think?

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:89: // The label of
the opened device. THIS MUST ONLY BE ACCESSED ON THE MAIN
This is a unique ID generated by the media code to identify a request.
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/browser/renderer_ho...

On 2012/03/16 22:29:09, viettrungluu wrote:
> What exactly is a "label"? (The human-readable name? In what encoding?)

http://codereview.chromium.org/9705056/diff/1/webkit/plugins/ppapi/plugin_del...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9705056/diff/1/webkit/plugins/ppapi/plugin_del...
webkit/plugins/ppapi/plugin_delegate.h:199: // The (interface for the) client
used by |PlatformAudio| and
On 2012/03/16 22:29:09, viettrungluu wrote:
> Probably this comment needs to be updated. Maybe it should be
> PlatformAudioClientBase (and PlatformAudioOutputClient made a trivial subclass
> of it, or something).

Done.

http://codereview.chromium.org/9705056/diff/11001/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/9705056/diff/11001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:242: if (succeeded)
{
Yeah. Thanks for pointing it out.
On 2012/03/18 00:38:59, xians1 wrote:
> it is more clear to be
> if (succeeded && plugin_delegate_)

http://codereview.chromium.org/9705056/diff/11001/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:244: 
On 2012/03/18 00:38:59, xians1 wrote:
> add a DCHECK(!label_.empty()) ?

Done.

http://codereview.chromium.org/9705056/diff/11001/ppapi/examples/audio_input/...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/9705056/diff/11001/ppapi/examples/audio_input/...
ppapi/examples/audio_input/audio_input.cc:236: if (!audio_input_.StartCapture())
On 2012/03/18 00:38:59, xians1 wrote:
> hasn't this function been changed to void?

We changed PlatformAudioInput::StartCapture() to return void, but not
PPB_AudioInput_Dev.StartCapture().

As I mentioned in a previous comment, ppb_audio_input_shared maintains a simple
state machine, which makes sure PlatformAudioInput is used in a reasonable way.
So PPB_AudioInput_Dev.StartCapture() will return a boolean value, for example,
PP_FALSE when the device has been closed, etc.

[viettrungluu, 2012-03-20 18:20:02]

LGTM with (very small) nits.

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

http://codereview.chromium.org/9705056/diff/1/content/renderer/pepper/pepper_...
content/renderer/pepper/pepper_platform_audio_input_impl.h:86: // THIS MUST ONLY
BE ACCESSED ON THE MAIN THREAD.
On 2012/03/19 21:47:54, yzshen1 wrote:
> On 2012/03/16 22:29:09, viettrungluu wrote:
> > Is the thread usage of this class documented somewhere?
> Done. I added a threading comment for this class at the top.
> 
> > 
> > Also, it might be helpful to implement appropriate getters/setters that
> DCHECK()
> > the thread.
> Adding getters/setters bloats the class definition a little bit. Since all
these
> data members are private and they are clearly commented, I think it is okay to
> not use getters/setters.
> (Besides, all methods are DCHECKed to make sure they are used on the right
> thread.)
> 
> What do you think?

Sounds fine, now that you've clarified the thread semantics.

> 
>

http://codereview.chromium.org/9705056/diff/15001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9705056/diff/15001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:245: // Starts the capture.
Nit: I think this comment is now officially useless, and should be removed.

http://codereview.chromium.org/9705056/diff/15001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:248: // Stops the capture.
"

[yzshen1, 2012-03-21 17:55:45]

Thanks!

https://chromiumcodereview.appspot.com/9705056/diff/15001/webkit/plugins/ppap...
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/9705056/diff/15001/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_delegate.h:245: // Starts the capture.
On 2012/03/20 18:20:02, viettrungluu wrote:
> Nit: I think this comment is now officially useless, and should be removed.

Done.

https://chromiumcodereview.appspot.com/9705056/diff/15001/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_delegate.h:248: // Stops the capture.
On 2012/03/20 18:20:02, viettrungluu wrote:
> "

Done.

[yzshen1, 2012-03-21 18:02:15]

[+brettw]

Hi, Brett.
Would you please do an OWNER review for content/renderer?
Thanks!

On 2012/03/21 17:55:45, yzshen1 wrote:
> Thanks!
> 
>
https://chromiumcodereview.appspot.com/9705056/diff/15001/webkit/plugins/ppap...
> File webkit/plugins/ppapi/plugin_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/9705056/diff/15001/webkit/plugins/ppap...
> webkit/plugins/ppapi/plugin_delegate.h:245: // Starts the capture.
> On 2012/03/20 18:20:02, viettrungluu wrote:
> > Nit: I think this comment is now officially useless, and should be removed.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9705056/diff/15001/webkit/plugins/ppap...
> webkit/plugins/ppapi/plugin_delegate.h:248: // Stops the capture.
> On 2012/03/20 18:20:02, viettrungluu wrote:
> > "
> 
> Done.

[brettw, 2012-03-21 18:12:19]

lgtm

[commit-bot: I haz the power, 2012-03-21 20:00:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/9705056/29002

[commit-bot: I haz the power, 2012-03-21 21:24:54]

Can't apply patch for file
content/renderer/pepper/pepper_platform_audio_input_impl.cc.
While running patch -p1 --forward --force;
patching file content/renderer/pepper/pepper_platform_audio_input_impl.cc
Hunk #1 FAILED at 10.
Hunk #3 FAILED at 70.
Hunk #4 succeeded at 133 (offset -3 lines).
Hunk #5 succeeded at 160 (offset -3 lines).
2 out of 5 hunks FAILED -- saving rejects to file
content/renderer/pepper/pepper_platform_audio_input_impl.cc.rej

[commit-bot: I haz the power, 2012-03-21 22:13:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/9705056/24004

[commit-bot: I haz the power, 2012-03-21 23:44:00]

Change committed as 128110