Microphone support for Pepper Flash.

Microphone support for Pepper Flash.

[Committing for pbrophy@adobe.com. Original review:
http://codereview.chromium.org/8138008/ .]

This change supports audio capture from the microphone and supplies the data
through a Pepper interface. Its enumeration is limited to the default audio
device that uses mono 44.1kHz.

TBR=tony@chromium.org

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

[viettrungluu, 2011-11-16 19:31:47]

Peter, please have a look at the (small) changes I've made.

jam, dmichael: I'll need OWNERS stamps.... I'm still rather unhappy about the
general design (it's a mess and has a ton of duplicated code), but I'd like to
get this in, since it's blocking other work. (Peter Brophy from Adobe is working
on writing tests for the audio capture. The original review of Peter's CL is
here: http://codereview.chromium.org/8138008 .)

[jam, 2011-11-16 20:49:52]

content lgtm

http://codereview.chromium.org/8574029/diff/8001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8574029/diff/8001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:398: MessageLoop*
main_message_loop_;
nit: better to use MessageLoopProxy

[viettrungluu, 2011-11-16 23:51:17]

Thanks, John.

Dave -- friendly ping?

http://codereview.chromium.org/8574029/diff/8001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8574029/diff/8001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:398: MessageLoop*
main_message_loop_;
On 2011/11/16 20:49:52, John Abd-El-Malek wrote:
> nit: better to use MessageLoopProxy

Done (assuming that all I needed to do was to change MessageLoop ->
base::MessageLoopProxy).

[dmichael (off chromium), 2011-11-17 04:05:21]

Mostly nits so far...  I'll finish reviewing tomorrow.

http://codereview.chromium.org/8574029/diff/18001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8574029/diff/18001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:367: virtual void ShutDown();
OVERRIDE?

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
ppapi/api/dev/ppb_audio_input_dev.idl:28: [version=0.1,
macro="PPB_AUDIO_INPUT_DEV_INTERFACE"]
nit: I don't think you need this line ^^^

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
ppapi/api/dev/ppb_audio_input_dev.idl:50: [in] PP_Resource audio_input);
nit: It seems like this could all be 1 line (same below)

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
File ppapi/api/trusted/ppb_audio_input_trusted_dev.idl (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:16: * resource returned is an
Audio input esource; most of the PPB_Audio
nit: esource -> resource

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:19: [version=0.1,
macro="PPB_AUDIO_INPUT_TRUSTED_DEV_INTERFACE"]
nit: I don't think you need this line.

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:23: [in] PP_Instance
instance);
Why do we need a separate 'Create' function? Can't we use the same
PPB_AudioInput_Dev.Create function, and allow passing a PPB_AudioInput_Dev
resource to the functions below?

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:35: [in] PP_CompletionCallback
create_callback);
nit: it seems like the params could align right after the 'Open(', with the
first param on the same line as Open. (Same for functions below)

http://codereview.chromium.org/8574029/diff/18001/ppapi/cpp/dev/audio_input_d...
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/cpp/dev/audio_input_d...
ppapi/cpp/dev/audio_input_dev.h:26: /// Getter function for returning the
internal <code>PPB_AudioConfig</code>
nit: PPB_AudioConfig struct->AudioConfig object
(same below)

http://codereview.chromium.org/8574029/diff/18001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:501: // PPB_AudioInput.
nit: PPB_AudioInput_Dev (consistent with the earlier comment)

http://codereview.chromium.org/8574029/diff/18001/ppapi/proxy/ppb_audio_input...
File ppapi/proxy/ppb_audio_input_proxy.cc (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/proxy/ppb_audio_input...
ppapi/proxy/ppb_audio_input_proxy.cc:57: PP_Resource config_;
You could use scoped_refptr<AudioConfigImpl> and save yourself the manual
ref-counting.

http://codereview.chromium.org/8574029/diff/18001/ppapi/proxy/ppb_audio_input...
ppapi/proxy/ppb_audio_input_proxy.cc:107: PP_CompletionCallback create_callback)
{
NOTREACHED()?

FWIW, it seems a little weird to me to have these here at all (though poking
around it looks like you just followed what Audio already did). It seems like
really these are 'In-process-only', not 'trusted'. I.e., these aren't really
available to trusted out-of-process plugins and presumably never will be. It
would kind of make sense to call them 'Private' instead and make a separate API
(but again, I see we're inconsistent there for other interfaces :-( )

Fine to have these here for this CL...  I'll have to think about how we could
factor these kinds of things better.

http://codereview.chromium.org/8574029/diff/18001/ppapi/proxy/ppb_audio_input...
ppapi/proxy/ppb_audio_input_proxy.cc:282: &shared_memory_length);
nit: indentation

http://codereview.chromium.org/8574029/diff/18001/ppapi/shared_impl/audio_inp...
File ppapi/shared_impl/audio_input_impl.h (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/shared_impl/audio_inp...
ppapi/shared_impl/audio_input_impl.h:45: void SetStopCaptureState();
Should these be protected? Maybe some of the others as well?

http://codereview.chromium.org/8574029/diff/18001/ppapi/shared_impl/audio_inp...
ppapi/shared_impl/audio_input_impl.h:59: virtual void Run();
Are all the other methods in the class run on the audio input thread too? (I'm
guessing not...)

I usually prefer to use a separate class for a separate thread, if practical. It
just makes it simpler to know what you can access on what thread.

[ilja, 2011-11-17 04:55:29]

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
ppapi/api/dev/ppb_audio_input_dev.idl:28: [version=0.1,
macro="PPB_AUDIO_INPUT_DEV_INTERFACE"]
On 2011/11/17 04:05:21, dmichael wrote:
> nit: I don't think you need this line ^^^

Peter and I put this in because the naming convention seems to be that
AudioInput in the interface definition below should be concatenated. But when
all lower cased (like in this file's name) or all upper cased (like in the
macro) then with an underscore.

[dmichael (off chromium), 2011-11-17 18:24:04]

Mostly nits. Looks pretty good overall, I guess that's because it already had a
thorough review (my bad, just saw that!)

Can I get a promise of a test for his next CL? :-) Just basic stuff creation and
making sure the input thread gets invoked is probably testable without too much
effort.

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
ppapi/api/dev/ppb_audio_input_dev.idl:28: [version=0.1,
macro="PPB_AUDIO_INPUT_DEV_INTERFACE"]
On 2011/11/17 04:55:29, ilja wrote:
> On 2011/11/17 04:05:21, dmichael wrote:
> > nit: I don't think you need this line ^^^
> 
> Peter and I put this in because the naming convention seems to be that
> AudioInput in the interface definition below should be concatenated. But when
> all lower cased (like in this file's name) or all upper cased (like in the
> macro) then with an underscore.
I don't think there is a consistent convention :-).  See, e.g., PPB_URLLoader_*
for counter examples.

That said, I do prefer the one you're doing here. I'll mention this to Noel to
see what he thinks; the IDL compiler should be able enforce this convention (if
it's the one we want) without any special hints, and maybe we should
special-case the *other* way instead.

Leave it as-is; thanks for explaining.

http://codereview.chromium.org/8574029/diff/18001/ppapi/thunk/ppb_audio_input...
File ppapi/thunk/ppb_audio_input_api.h (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/thunk/ppb_audio_input...
ppapi/thunk/ppb_audio_input_api.h:6: #define PPAPI_THUNK_AUDIO_INPUT_API_H_
I think that should be PPAPI_THUNK_PPB_AUDIO_INPUT_API_H_

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

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:319: virtual PlatformAudioInput*
CreateAudioInput(uint32_t sample_rate,
nit: I would usually put sample_rate on the next line at the same indentation as
the rest.

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
File webkit/plugins/ppapi/ppb_audio_input_impl.cc (right):

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
webkit/plugins/ppapi/ppb_audio_input_impl.cc:52: audio_input(new
PPB_AudioInput_Impl(instance));
nit: I'm used to seeing 4 space indent here ^^^

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
File webkit/plugins/ppapi/ppb_audio_input_impl.h (right):

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
webkit/plugins/ppapi/ppb_audio_input_impl.h:47: // Initialization function for
non-trusted init.
This seems to disagree with the comments on the constructor above. Should it say
'trusted' instead?

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
webkit/plugins/ppapi/ppb_audio_input_impl.h:49: PPB_AudioInput_Callback
callback, void* user_data);
nit: this is an unusual way to break the line & indent. It should probably be
either:
(a) On one line, if it can fit (guess it can't)
(b) each param on its own line, indented to line up:
bool Init(PP_Resource config_id,
          PPB_AudioInput_Callback callback,
          void* user_data);
(c) break the line at the first param that *won't* fit, and indent to the same
level:
bool Init(PP_Resource config_id, PPB_AudioInput_Callback callback,
          void* user_data);

[dmichael (off chromium), 2011-11-17 19:06:40]

LGTM

I think we should do some style cleanup and add a test in a separate CL, but
it's okay to land in the mean time.

[viettrungluu, 2011-11-17 22:52:08]

+tony for OWNERS review of webkit_glue.gypi.

[viettrungluu, 2011-11-17 22:52:08]

+tony for OWNERS review of webkit_glue.gypi.

[tony, 2011-11-18 00:26:15]

webkit/glue lgtm.

[James Cook, 2011-11-18 05:22:39]

This appears to have broken Win builder (dbg)(shared).

[James Cook, 2011-11-18 05:34:19]

On 2011/11/18 05:22:39, James Cook (Chromium) wrote:
> This appears to have broken Win builder (dbg)(shared).

Looks like a missing PPAPI_THUNK_EXPORT, rsleevi is attempting to patch with
http://codereview.chromium.org/8558012/

[seriesrover, 2011-11-18 18:59:42]

> add a test in a separate CL

Agreed. I'm working on this atm.

[viettrungluu, 2011-11-21 20:55:21]

Nits mentioned here fixed in this CL: http://codereview.chromium.org/8489002

(or deferred, in the case of .idl style).

On 2011/11/17 18:24:04, dmichael wrote:
> Mostly nits. Looks pretty good overall, I guess that's because it already had
a
> thorough review (my bad, just saw that!)
> 
> Can I get a promise of a test for his next CL? :-) Just basic stuff creation
and
> making sure the input thread gets invoked is probably testable without too
much
> effort.
> 
>
http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
> File ppapi/api/dev/ppb_audio_input_dev.idl (right):
> 
>
http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
> ppapi/api/dev/ppb_audio_input_dev.idl:28: [version=0.1,
> macro="PPB_AUDIO_INPUT_DEV_INTERFACE"]
> On 2011/11/17 04:55:29, ilja wrote:
> > On 2011/11/17 04:05:21, dmichael wrote:
> > > nit: I don't think you need this line ^^^
> > 
> > Peter and I put this in because the naming convention seems to be that
> > AudioInput in the interface definition below should be concatenated. But
when
> > all lower cased (like in this file's name) or all upper cased (like in the
> > macro) then with an underscore.
> I don't think there is a consistent convention :-).  See, e.g.,
PPB_URLLoader_*
> for counter examples.
> 
> That said, I do prefer the one you're doing here. I'll mention this to Noel to
> see what he thinks; the IDL compiler should be able enforce this convention
(if
> it's the one we want) without any special hints, and maybe we should
> special-case the *other* way instead.
> 
> Leave it as-is; thanks for explaining.
> 
>
http://codereview.chromium.org/8574029/diff/18001/ppapi/thunk/ppb_audio_input...
> File ppapi/thunk/ppb_audio_input_api.h (right):
> 
>
http://codereview.chromium.org/8574029/diff/18001/ppapi/thunk/ppb_audio_input...
> ppapi/thunk/ppb_audio_input_api.h:6: #define PPAPI_THUNK_AUDIO_INPUT_API_H_
> I think that should be PPAPI_THUNK_PPB_AUDIO_INPUT_API_H_
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/plugin...
> File webkit/plugins/ppapi/plugin_delegate.h (right):
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/plugin...
> webkit/plugins/ppapi/plugin_delegate.h:319: virtual PlatformAudioInput*
> CreateAudioInput(uint32_t sample_rate,
> nit: I would usually put sample_rate on the next line at the same indentation
as
> the rest.
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
> File webkit/plugins/ppapi/ppb_audio_input_impl.cc (right):
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
> webkit/plugins/ppapi/ppb_audio_input_impl.cc:52: audio_input(new
> PPB_AudioInput_Impl(instance));
> nit: I'm used to seeing 4 space indent here ^^^
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
> File webkit/plugins/ppapi/ppb_audio_input_impl.h (right):
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
> webkit/plugins/ppapi/ppb_audio_input_impl.h:47: // Initialization function for
> non-trusted init.
> This seems to disagree with the comments on the constructor above. Should it
say
> 'trusted' instead?
> 
>
http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
> webkit/plugins/ppapi/ppb_audio_input_impl.h:49: PPB_AudioInput_Callback
> callback, void* user_data);
> nit: this is an unusual way to break the line & indent. It should probably be
> either:
> (a) On one line, if it can fit (guess it can't)
> (b) each param on its own line, indented to line up:
> bool Init(PP_Resource config_id,
>           PPB_AudioInput_Callback callback,
>           void* user_data);
> (c) break the line at the first param that *won't* fit, and indent to the same
> level:
> bool Init(PP_Resource config_id, PPB_AudioInput_Callback callback,
>           void* user_data);

[viettrungluu, 2011-11-21 21:26:13]

Oops, stupid "Reply" doesn't send draft comments; sending for completeness.

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/dev/ppb_audio_inp...
ppapi/api/dev/ppb_audio_input_dev.idl:50: [in] PP_Resource audio_input);
On 2011/11/17 04:05:21, dmichael wrote:
> nit: It seems like this could all be 1 line (same below)

This is currently the style consistently in the .idl files. If we change it, we
should go ahead and change all instances, but I'll hold off on it for now.

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
File ppapi/api/trusted/ppb_audio_input_trusted_dev.idl (right):

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:16: * resource returned is an
Audio input esource; most of the PPB_Audio
On 2011/11/17 04:05:21, dmichael wrote:
> nit: esource -> resource

Done.

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:23: [in] PP_Instance
instance);
On 2011/11/17 04:05:21, dmichael wrote:
> Why do we need a separate 'Create' function? Can't we use the same
> PPB_AudioInput_Dev.Create function, and allow passing a PPB_AudioInput_Dev
> resource to the functions below?

I'd have to check whether anything special is done before |Open()| is called,
but in any case this is consistent with the PPB_AudioTrusted interface (FWIW).

http://codereview.chromium.org/8574029/diff/18001/ppapi/api/trusted/ppb_audio...
ppapi/api/trusted/ppb_audio_input_trusted_dev.idl:35: [in] PP_CompletionCallback
create_callback);
On 2011/11/17 04:05:21, dmichael wrote:
> nit: it seems like the params could align right after the 'Open(', with the
> first param on the same line as Open. (Same for functions below)

Also currently the style in all the .idl files.

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
File webkit/plugins/ppapi/ppb_audio_input_impl.cc (right):

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
webkit/plugins/ppapi/ppb_audio_input_impl.cc:52: audio_input(new
PPB_AudioInput_Impl(instance));
On 2011/11/17 18:24:04, dmichael wrote:
> nit: I'm used to seeing 4 space indent here ^^^

Done.

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
File webkit/plugins/ppapi/ppb_audio_input_impl.h (right):

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
webkit/plugins/ppapi/ppb_audio_input_impl.h:47: // Initialization function for
non-trusted init.
On 2011/11/17 18:24:04, dmichael wrote:
> This seems to disagree with the comments on the constructor above. Should it
say
> 'trusted' instead?

Done.

http://codereview.chromium.org/8574029/diff/18001/webkit/plugins/ppapi/ppb_au...
webkit/plugins/ppapi/ppb_audio_input_impl.h:49: PPB_AudioInput_Callback
callback, void* user_data);
On 2011/11/17 18:24:04, dmichael wrote:
> nit: this is an unusual way to break the line & indent. It should probably be
> either:
> (a) On one line, if it can fit (guess it can't)
> (b) each param on its own line, indented to line up:
> bool Init(PP_Resource config_id,
>           PPB_AudioInput_Callback callback,
>           void* user_data);
> (c) break the line at the first param that *won't* fit, and indent to the same
> level:
> bool Init(PP_Resource config_id, PPB_AudioInput_Callback callback,
>           void* user_data);

Done.

Also, note that (c) is at least moderately frowned upon, AFAIK (with (b) being
preferred).