Pepper: Add an example/test plugin for audio input (a.k.a. microphone).

Pepper: Add an example/test plugin for audio input (a.k.a. microphone).


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

[viettrungluu, 2011-11-23 19:03:27]

Adding Peter Brophy to CC list.

[dmichael (off chromium), 2011-11-23 20:15:52]

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:58: timer_interval_ = 44100 /
sample_count_ + 5;
This doesn't make sense to me. timer_interval_ is used as ms, but the units here
are more like 1/s. Imagine if you had a higher sample frequency or lower sample
count. The timer_interval_ would then be bigger, but you should want it to be
smaller, I'd think.

Maybe should be something like:
timer_interval_ = (sample_count_ * 1000) / kSampleFrequency + 5;
(where 5 is apparently a fudge factor?)

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:124: uint32_t channel_count) {
nit: Is there a reason to use parameters for these last 3 instead of accessing
them directly? Just making it less coupled with the rest? As written, it could
probably be made static.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:149: int y = samples[i] *
max_amplitude / 32768 + mid_height;
nit: std::numeric_limits<int16_t>::max()+1 might look less magical than 32768.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:156: // TODO(viettrungluu): Danger! We
really should lock, but which thread
I have some threading primitive wrappers sitting somewhere locally, if you want.
I ended up not needing them yet, but I'm saving them for when I do need to write
a test or example with locking.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:159: // writing to a valid region of
memory.
But I agree... in this case, the worst that will happen (and it's very likely it
will happen) is drawing a bad frame, which will probably never look so bad as to
be noticed.

Of course, it is an example, so we should ultimately fix it lest it be
copied/pasted this way.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:163: MyInstance* thiz =
reinterpret_cast<MyInstance*>(ctx);
static_cast

[viettrungluu, 2011-11-23 21:06:25]

Thanks, new patch set up.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:58: timer_interval_ = 44100 /
sample_count_ + 5;
On 2011/11/23 20:15:52, dmichael wrote:
> This doesn't make sense to me. timer_interval_ is used as ms, but the units
here
> are more like 1/s. Imagine if you had a higher sample frequency or lower
sample
> count. The timer_interval_ would then be bigger, but you should want it to be
> smaller, I'd think.
> 
> Maybe should be something like:
> timer_interval_ = (sample_count_ * 1000) / kSampleFrequency + 5;
> (where 5 is apparently a fudge factor?)

Right, good catch. Fixed.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:124: uint32_t channel_count) {
On 2011/11/23 20:15:52, dmichael wrote:
> nit: Is there a reason to use parameters for these last 3 instead of accessing
> them directly? Just making it less coupled with the rest? As written, it could
> probably be made static.

No good reason; I got rid of the last three parameters.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:149: int y = samples[i] *
max_amplitude / 32768 + mid_height;
On 2011/11/23 20:15:52, dmichael wrote:
> nit: std::numeric_limits<int16_t>::max()+1 might look less magical than 32768.

Done.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:159: // writing to a valid region of
memory.
On 2011/11/23 20:15:52, dmichael wrote:
> But I agree... in this case, the worst that will happen (and it's very likely
it
> will happen) is drawing a bad frame, which will probably never look so bad as
to
> be noticed.
> 
> Of course, it is an example, so we should ultimately fix it lest it be
> copied/pasted this way.

Yeah, we should fix it, but let's hold off on it for the moment, if that sounds
okay to you.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:163: MyInstance* thiz =
reinterpret_cast<MyInstance*>(ctx);
On 2011/11/23 20:15:52, dmichael wrote:
> static_cast

Done.

[dmichael (off chromium), 2011-11-23 21:12:37]

lgtm

[yzshen1, 2011-11-23 21:21:17]

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:183: 
One redundant empty line could be removed.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:185: pp::AudioInput_Dev audio_input_;
nit: Allocated / freed may not be accurate. It sounds like |audio_input_| is an
object on the heap.
Maybe this comment is for |samples_|?

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

http://codereview.chromium.org/8662020/diff/2001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:94: pending_paint_ = false;
nit: it might be better to make Paint() the only one who modifies
|pending_paint_|.

http://codereview.chromium.org/8662020/diff/2001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:161: thiz->sample_count_ *
channel_count_ * sizeof(int16_t));
channel_count_ should be thiz->channel_count_, right?

[viettrungluu, 2011-11-23 21:31:30]

Thanks, Yuzhu. PTAL.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
File ppapi/examples/audio_input/audio_input.cc (right):

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:183: 
On 2011/11/23 21:21:17, yzshen1 wrote:
> One redundant empty line could be removed.

Done.

http://codereview.chromium.org/8662020/diff/1/ppapi/examples/audio_input/audi...
ppapi/examples/audio_input/audio_input.cc:185: pp::AudioInput_Dev audio_input_;
On 2011/11/23 21:21:17, yzshen1 wrote:
> nit: Allocated / freed may not be accurate. It sounds like |audio_input_| is
an
> object on the heap.
> Maybe this comment is for |samples_|?

I removed this misleading comment (which I forgot to remove when
cutting-and-pasting from examples/audio.cc).

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

http://codereview.chromium.org/8662020/diff/2001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:94: pending_paint_ = false;
On 2011/11/23 21:21:17, yzshen1 wrote:
> nit: it might be better to make Paint() the only one who modifies
> |pending_paint_|.

Done.

http://codereview.chromium.org/8662020/diff/2001/ppapi/examples/audio_input/a...
ppapi/examples/audio_input/audio_input.cc:161: thiz->sample_count_ *
channel_count_ * sizeof(int16_t));
On 2011/11/23 21:21:17, yzshen1 wrote:
> channel_count_ should be thiz->channel_count_, right?

Yes. I don't know how it managed to compile for me on Linux!

[yzshen1, 2011-11-23 21:33:21]

lgtm

[commit-bot: I haz the power, 2011-11-23 21:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/viettrungluu@chromium.org/8662020/3004

[commit-bot: I haz the power, 2011-11-23 23:40:03]

Change committed as 111444