PPB_AudioInput_Dev: support multiple audio input devices - Part 1.

PPB_AudioInput_Dev: support multiple audio input devices - Part 1.

- This CL implements PPB_AudioInput_Dev v0.2 and extends examples/audio_input.
- This CL doesn't actually open devices other than the default one. That will be in a separate CL.

BUG=None
TEST=examples/audio_input


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

[yzshen1, 2012-03-01 01:07:39]

Hi, Brett and Trung.

Please take a look. Thanks!

[brettw, 2012-03-02 19:13:41]

http://codereview.chromium.org/9557007/diff/1/ppapi/api/dev/ppb_audio_input_d...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/api/dev/ppb_audio_input_d...
ppapi/api/dev/ppb_audio_input_dev.idl:133: * StartCapture() starts the capture
of the audio input resource and begins
Can both start & stop note that they return true if the audio capture is already
started/stopped?

http://codereview.chromium.org/9557007/diff/1/ppapi/api/dev/ppb_audio_input_d...
ppapi/api/dev/ppb_audio_input_dev.idl:145: /**
Can you remove "pointer to a function" here?

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.cc
File ppapi/cpp/dev/audio_input_dev.cc (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.c...
ppapi/cpp/dev/audio_input_dev.cc:103: if
(!callback.pp_completion_callback().func)
I don't think you need this check since the C API will do it for you. Is the
same true for the enum_state_ check? The C++ wrappers should be as thin as
possible.

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.h
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.h...
ppapi/cpp/dev/audio_input_dev.h:24: AudioInput_Dev(const InstanceHandle&
instance,
Can you mark this deprecated?

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.h...
ppapi/cpp/dev/audio_input_dev.h:60: /// In order to maintain backward
compatibility, this method doesn't have
Can we have a overloaded version of Open() that has the new parameters?

[yzshen1, 2012-03-04 08:04:01]

Thanks, Brett!
Please take another look.

http://codereview.chromium.org/9557007/diff/1/ppapi/api/dev/ppb_audio_input_d...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/api/dev/ppb_audio_input_d...
ppapi/api/dev/ppb_audio_input_dev.idl:133: * StartCapture() starts the capture
of the audio input resource and begins
On 2012/03/02 19:13:41, brettw wrote:
> Can both start & stop note that they return true if the audio capture is
already
> started/stopped?

Done.

http://codereview.chromium.org/9557007/diff/1/ppapi/api/dev/ppb_audio_input_d...
ppapi/api/dev/ppb_audio_input_dev.idl:145: /**
On 2012/03/02 19:13:41, brettw wrote:
> Can you remove "pointer to a function" here?

Done.

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.cc
File ppapi/cpp/dev/audio_input_dev.cc (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.c...
ppapi/cpp/dev/audio_input_dev.cc:103: if
(!callback.pp_completion_callback().func)
Because we insert an extra layer of callback in the middle (i.e.,
OnEnumerateDevicesComplete), EnumerateDevices() will return us
PP_OK_COMPLETIONPENDING for sure. We have to ensure that the callback from the
caller is not NULL. Otherwise, we don't know how to proceed.

With regard to enum_state_, if we allow to call into EnumerateDevices() without
an enum_state_ NULL check, we have to keep track of multiple enum_state_
objects, in order to implement AbortEnumerateDevices() properly. I think it
doesn't worth the effort, moreover, the underlying implementation doesn't
support multiple EnumerateDevices() calls at the same time anyway. Therefore, I
think a NULL check here is necessary. :)


On 2012/03/02 19:13:41, brettw wrote:
> I don't think you need this check since the C API will do it for you. Is the
> same true for the enum_state_ check? The C++ wrappers should be as thin as
> possible.

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.h
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.h...
ppapi/cpp/dev/audio_input_dev.h:24: AudioInput_Dev(const InstanceHandle&
instance,
On 2012/03/02 19:13:41, brettw wrote:
> Can you mark this deprecated?

Done.

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.h...
ppapi/cpp/dev/audio_input_dev.h:60: /// In order to maintain backward
compatibility, this method doesn't have
On 2012/03/02 19:13:41, brettw wrote:
> Can we have a overloaded version of Open() that has the new parameters?

Done.

[brettw, 2012-03-05 21:32:34]

lgtm

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.cc
File ppapi/cpp/dev/audio_input_dev.cc (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.c...
ppapi/cpp/dev/audio_input_dev.cc:103: if
(!callback.pp_completion_callback().func)
Okay, I see.

[yzshen1, 2012-03-05 22:19:13]

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.cc
File ppapi/cpp/dev/audio_input_dev.cc (right):

http://codereview.chromium.org/9557007/diff/1/ppapi/cpp/dev/audio_input_dev.c...
ppapi/cpp/dev/audio_input_dev.cc:103: if
(!callback.pp_completion_callback().func)
Thanks! Once your work of array/async out param is checked in, I will change
this code and make it simpler.

On 2012/03/05 21:32:34, brettw wrote:
> Okay, I see.

[viettrungluu, 2012-03-06 19:17:28]

Here's half a review....

http://codereview.chromium.org/9557007/diff/8001/ppapi/api/dev/ppb_audio_inpu...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/api/dev/ppb_audio_inpu...
ppapi/api/dev/ppb_audio_input_dev.idl:31: [version=0.1]
Probably this should have a "deprecate" label.

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
File ppapi/cpp/dev/audio_input_dev.cc (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.cc:42: std::vector<DeviceRef_Dev>* devices;
Probably add a comment that it's owned by the "user"?

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.cc:223: for (uint32_t index = 0; index <
resources.size(); ++index) {
size_t, not uint32_t, I guess.

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_dev.h
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.h:104: // Not owned by this object.
It'd be a little more helpful if you specified whom it was actually owned by
(and whence it came). :)

http://codereview.chromium.org/9557007/diff/8001/ppapi/proxy/resource_creatio...
File ppapi/proxy/resource_creation_proxy.cc (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/proxy/resource_creatio...
ppapi/proxy/resource_creation_proxy.cc:356: }  // namespace ppapi
Reviewed up to here.

[viettrungluu, 2012-03-06 19:26:30]

LGTM on everything else, though my head hurts.

http://codereview.chromium.org/9557007/diff/8001/ppapi/examples/audio_input/a...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:108:
PostMessage(pp::Var("OpenFailed"));
Admittedly, it's a bit strange that you check that Create() succeeded here, but
not the return value of other calls.

http://codereview.chromium.org/9557007/diff/8001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:120: assert(false);
You should probably include some header for this. Or, better, use the macros in
ppapi/cpp/logging.h.

[yzshen1, 2012-03-07 00:30:35]

Thanks, Trung.

http://codereview.chromium.org/9557007/diff/8001/ppapi/api/dev/ppb_audio_inpu...
File ppapi/api/dev/ppb_audio_input_dev.idl (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/api/dev/ppb_audio_inpu...
ppapi/api/dev/ppb_audio_input_dev.idl:31: [version=0.1]
On 2012/03/06 19:17:28, viettrungluu wrote:
> Probably this should have a "deprecate" label.

Done.

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
File ppapi/cpp/dev/audio_input_dev.cc (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.cc:42: std::vector<DeviceRef_Dev>* devices;
On 2012/03/06 19:17:28, viettrungluu wrote:
> Probably add a comment that it's owned by the "user"?

Done.

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.cc:223: for (uint32_t index = 0; index <
resources.size(); ++index) {
Because we usually use uint32_t in PPAPI for index and array size,
ResourceArray_Dev also returns size as uint32_t.

Does that sound okay?
On 2012/03/06 19:17:28, viettrungluu wrote:
> size_t, not uint32_t, I guess.

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_dev.h
File ppapi/cpp/dev/audio_input_dev.h (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/cpp/dev/audio_input_de...
ppapi/cpp/dev/audio_input_dev.h:104: // Not owned by this object.
On 2012/03/06 19:17:28, viettrungluu wrote:
> It'd be a little more helpful if you specified whom it was actually owned by
> (and whence it came). :)

Good suggestion. I should have done that earlier. :)

http://codereview.chromium.org/9557007/diff/8001/ppapi/examples/audio_input/a...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/9557007/diff/8001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:108:
PostMessage(pp::Var("OpenFailed"));
More checks added.

On 2012/03/06 19:26:30, viettrungluu wrote:
> Admittedly, it's a bit strange that you check that Create() succeeded here,
but
> not the return value of other calls.

http://codereview.chromium.org/9557007/diff/8001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:120: assert(false);
On 2012/03/06 19:26:30, viettrungluu wrote:
> You should probably include some header for this. Or, better, use the macros
in
> ppapi/cpp/logging.h.

Done.

[commit-bot: I haz the power, 2012-03-07 06:16:01]

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

[commit-bot: I haz the power, 2012-03-07 07:48:50]

Change committed as 125362