Issue 290993005: Support configuring number of audio buffers in MediaStream Pepper API.

Issue 290993005: Support configuring number of audio buffers in MediaStream Pepper API. (Closed)

Created:
6 years, 7 months ago by thembrown

Modified:
6 years, 6 months ago

Reviewers:
dmichael (off chromium), teravest, Peng, dcheng

CC:
chromium-reviews, darin-cc_chromium.org, jam

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Support configuring number of audio buffers in MediaStream Pepper API. A optionally larger number of buffers makes recording audio in a pnacl module more reliable when latency is not an issue. BUG=330851 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274511

Patch Set 1 #

Total comments: 13

Patch Set 2 : Add tests, fixes for peng #

Total comments: 15

Patch Set 3 : Fix review issues #

Total comments: 24

Patch Set 4 : Fix review issues (dmichael) #

Patch Set 5 : Rebased #

Total comments: 12

Patch Set 6 : changed number_of_buffers to int32_t #

Patch Set 7 : Use CheckedNumeric in buffer size calculations #

Total comments: 1

Patch Set 8 : Remove unnecessary buffer_size_aligned check #

Patch Set 9 : Rebased #

Patch Set 10 : fix uninitialized variable #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+359 lines, -39 lines)			Patch
M	AUTHORS	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	content/renderer/pepper/pepper_media_stream_audio_track_host.h	View	1 2 3 4 5	3 chunks	+23 lines, -2 lines	0 comments	Download
M	content/renderer/pepper/pepper_media_stream_audio_track_host.cc	View	1 2 3 4 5 6 7 8 9	5 chunks	+73 lines, -16 lines	0 comments	Download
M	content/renderer/pepper/pepper_media_stream_track_host_base.cc	View	1 2 3 4 5 6 7	3 chunks	+17 lines, -9 lines	0 comments	Download
M	ppapi/api/ppb_media_stream_audio_track.idl	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	ppapi/c/ppb_media_stream_audio_track.h	View	1 2 3	2 chunks	+3 lines, -2 lines	0 comments	Download
M	ppapi/cpp/media_stream_audio_track.h	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	ppapi/ppapi_shared.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+3 lines, -1 line	0 comments	Download
M	ppapi/ppapi_tests.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/proxy/media_stream_audio_track_resource.h	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	ppapi/proxy/media_stream_audio_track_resource.cc	View	1 2 3	4 chunks	+53 lines, -3 lines	0 comments	Download
M	ppapi/proxy/ppapi_messages.h	View	1 2 3 4 5 6 7 8	3 chunks	+9 lines, -0 lines	0 comments	Download
A	ppapi/shared_impl/media_stream_audio_track_shared.h	View	1 2 3	1 chunk	+25 lines, -0 lines	0 comments	Download
A	ppapi/shared_impl/media_stream_audio_track_shared.cc	View	1 2	1 chunk	+17 lines, -0 lines	0 comments	Download
A	ppapi/shared_impl/media_stream_audio_track_shared_unittest.cc	View	1	1 chunk	+33 lines, -0 lines	0 comments	Download
M	ppapi/tests/test_media_stream_audio_track.h	View	1	2 chunks	+4 lines, -3 lines	0 comments	Download
M	ppapi/tests/test_media_stream_audio_track.cc	View	1 2 3	4 chunks	+87 lines, -1 line	0 comments	Download

Messages

Total messages: 32 (0 generated)

Expand Messages | Collapse Messages

teravest

-teravest +dmichael to reviewers. I don't follow MediaStream development much; Dave probably knows more of ...

6 years, 7 months ago (2014-05-23 15:56:47 UTC) #2

Peng

https://codereview.chromium.org/290993005/diff/1/content/renderer/pepper/pepper_media_stream_audio_track_host.cc File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/290993005/diff/1/content/renderer/pepper/pepper_media_stream_audio_track_host.cc#newcode92 content/renderer/pepper/pepper_media_stream_audio_track_host.cc:92: if (changed && bytes_per_second_ > 0) { {} is ...

6 years, 7 months ago (2014-05-26 14:48:48 UTC) #3

thembrown

Added tests + fixes https://codereview.chromium.org/290993005/diff/1/content/renderer/pepper/pepper_media_stream_audio_track_host.cc File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/290993005/diff/1/content/renderer/pepper/pepper_media_stream_audio_track_host.cc#newcode92 content/renderer/pepper/pepper_media_stream_audio_track_host.cc:92: if (changed && bytes_per_second_ > ...

6 years, 7 months ago (2014-05-27 10:36:01 UTC) #4

Peng

LGTM with several minor comments. https://codereview.chromium.org/290993005/diff/1/ppapi/proxy/media_stream_audio_track_resource.h File ppapi/proxy/media_stream_audio_track_resource.h (right): https://codereview.chromium.org/290993005/diff/1/ppapi/proxy/media_stream_audio_track_resource.h#newcode9 ppapi/proxy/media_stream_audio_track_resource.h:9: #include <string> On 2014/05/27 ...

6 years, 7 months ago (2014-05-27 14:35:49 UTC) #5

thembrown

https://codereview.chromium.org/290993005/diff/20001/ppapi/ppapi_shared.gypi File ppapi/ppapi_shared.gypi (right): https://codereview.chromium.org/290993005/diff/20001/ppapi/ppapi_shared.gypi#newcode48 ppapi/ppapi_shared.gypi:48: 'shared_impl/media_stream_audio_track_shared.cc', On 2014/05/27 14:35:49, Peng wrote: > alphabetical order ...

6 years, 7 months ago (2014-05-27 17:11:23 UTC) #6

dmichael (off chromium)

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc#newcode32 content/renderer/pepper/pepper_media_stream_audio_track_host.cc:32: const int32_t kMaxNumberOfBuffers = 1000; // 10 sec I'm ...

6 years, 7 months ago (2014-05-27 20:22:33 UTC) #7

thembrown

https://codereview.chromium.org/290993005/diff/20001/ppapi/shared_impl/media_stream_audio_track_shared.h File ppapi/shared_impl/media_stream_audio_track_shared.h (right): https://codereview.chromium.org/290993005/diff/20001/ppapi/shared_impl/media_stream_audio_track_shared.h#newcode8 ppapi/shared_impl/media_stream_audio_track_shared.h:8: #include "ppapi/c/ppb_audio_buffer.h" On 2014/05/27 17:11:23, thembrown wrote: > On ...

6 years, 6 months ago (2014-05-28 10:55:58 UTC) #8

https://codereview.chromium.org/290993005/diff/20001/ppapi/shared_impl/media_...
File ppapi/shared_impl/media_stream_audio_track_shared.h (right):

https://codereview.chromium.org/290993005/diff/20001/ppapi/shared_impl/media_...
ppapi/shared_impl/media_stream_audio_track_shared.h:8: #include
"ppapi/c/ppb_audio_buffer.h"
On 2014/05/27 17:11:23, thembrown wrote:
> On 2014/05/27 14:35:49, Peng wrote:
> > I think this header file is for int32_t? consider using ppapi/c/pp_stdint.h
> > instead.
> 
> Done.

Reverted. It's for PPAPI_SHARED_EXPORT.

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:32: const
int32_t kMaxNumberOfBuffers = 1000;  // 10 sec
On 2014/05/27 20:22:33, dmichael wrote:
> I'm slightly surprised that this isn't part of the new shared Attributes class
> instead.

Hm, adding it to Attributes::VerifyAttributes would change behaviour to failing
for greater values. The documentation on PP_MEDIASTREAMAUDIOTRACK_ATTRIB_BUFFERS
states that the value just serves as an advisory for the browser and it may
choose to allocate more or less buffers, depending on available resources. It
currently just clips the value - but I guess this is where some more advanced
treatment would happen in the future.

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:86: bool
changed;
On 2014/05/27 20:22:33, dmichael wrote:
> I think it would be good to have the same DCHECK_EQ as from 103 since this
> should always be on the main thread.

Done.

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:102: void
PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {
On 2014/05/27 20:22:33, dmichael wrote:
> These two names (InitBuffers and InitBuffersOnMainThread) are pretty confusing
> now. Especially since both are called on the main thread.
> 
> What about maybe:
> SetFormatOnMainThread
> and
> InitBuffers
> ?

Agreed. Changed to SetFormatOnMainThread.

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:107:
bytes_per_second_ * kMaxDuration / base::Time::kMicrosecondsPerSecond;
On 2014/05/27 20:22:33, dmichael wrote:
> The way I'm reading this, you're now dividing by 10^6 (kMicrosecondsPerSecond)
> whereas the old code divided by 1000. kMaxDuration is supposedly in
> milliseconds. Did you mean kMillisecondsPerSecond? Or am I missing something?

Changed. I guess it was too late for me yesterday.

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:110: kRead);
On 2014/05/27 20:22:33, dmichael wrote:
> nit: indentation is off. It looks like they all will fit if you line up with
> where number_of_buffers_ is.

Done.

https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:229:
CHECK(MediaStreamAudioTrackShared::VerifyAttributes(attributes));
On 2014/05/27 20:22:33, dmichael wrote:
> I don't think we should CHECK here. We're crashing the renderer because of
> plugin error. IIRC this kind of thing can make fuzz testing harder. I think we
> should just return PP_ERROR_FAILED.
> 
> (I notice that we do something similar in part of Video, so we should probably
> turn that in to PP_ERROR_FAILED separtely also).

Agreed. How about PP_ERROR_BADARGUMENT like in MediaStreamAudioTrackResource?

https://codereview.chromium.org/290993005/diff/40001/ppapi/proxy/media_stream...
File ppapi/proxy/media_stream_audio_track_resource.cc (right):

https://codereview.chromium.org/290993005/diff/40001/ppapi/proxy/media_stream...
ppapi/proxy/media_stream_audio_track_resource.cc:54: // Do not support
configure, if audio buffers are hold by plugin.
On 2014/05/27 20:22:33, dmichael wrote:
> nits:
> The comma is unnecessary.
> "hold by plugin"->"held by the plugin"

Done.

https://codereview.chromium.org/290993005/diff/40001/ppapi/proxy/media_stream...
ppapi/proxy/media_stream_audio_track_resource.cc:62: case
PP_MEDIASTREAMAUDIOTRACK_ATTRIB_BUFFERS:
On 2014/05/27 20:22:33, dmichael wrote:
> nit: Everything inside the switch should be indented another 2 spaces.

Done.

https://codereview.chromium.org/290993005/diff/40001/ppapi/tests/test_media_s...
File ppapi/tests/test_media_stream_audio_track.cc (right):

https://codereview.chromium.org/290993005/diff/40001/ppapi/tests/test_media_s...
ppapi/tests/test_media_stream_audio_track.cc:21: const int32_t
kMaxNumberOfBuffers = 1000;
On 2014/05/27 20:22:33, dmichael wrote:
> It would be good to reference the file where the real max is defined, since
> they're supposed to match.

Done.

https://codereview.chromium.org/290993005/diff/40001/ppapi/tests/test_media_s...
ppapi/tests/test_media_stream_audio_track.cc:181: 
On 2014/05/27 20:22:33, dmichael wrote:
> nit: unnecessary carriage return

Done.

https://codereview.chromium.org/290993005/diff/40001/ppapi/tests/test_media_s...
ppapi/tests/test_media_stream_audio_track.cc:203: // Configure should fail while
plugin holds buffers.
On 2014/05/27 20:22:33, dmichael wrote:
> It would probably be good to document this in the API. That is, change the
IDL,
> regenerate so that the C header gets the info, and also add to the C++ header.

Done.

dmichael (off chromium)

lgtm, thanks for doing this! https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/290993005/diff/40001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc#newcode32 content/renderer/pepper/pepper_media_stream_audio_track_host.cc:32: const int32_t kMaxNumberOfBuffers = ...

6 years, 6 months ago (2014-05-28 15:27:43 UTC) #9

dcheng

https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc#newcode110 content/renderer/pepper/pepper_media_stream_audio_track_host.cc:110: static_cast<int32_t>(buffer_size), Will we need to be concerned about overflow ...

6 years, 6 months ago (2014-05-28 15:55:43 UTC) #10

thembrown

6 years, 6 months ago (2014-05-28 17:13:10 UTC) #11

dcheng

6 years, 6 months ago (2014-05-28 17:21:06 UTC) #12

thembrown

6 years, 6 months ago (2014-05-28 19:09:53 UTC) #13

dcheng

On 2014/05/28 19:09:53, thembrown wrote: > https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc > File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): > > https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc#newcode110 > ...

6 years, 6 months ago (2014-05-29 16:12:25 UTC) #14

dcheng

On 2014/05/29 16:12:25, dcheng wrote: > On 2014/05/28 19:09:53, thembrown wrote: > > > https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper/pepper_media_stream_audio_track_host.cc ...

6 years, 6 months ago (2014-05-29 16:13:32 UTC) #15

On 2014/05/29 16:12:25, dcheng wrote:
> On 2014/05/28 19:09:53, thembrown wrote:
> >
>
https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper...
> > File content/renderer/pepper/pepper_media_stream_audio_track_host.cc
(right):
> > 
> >
>
https://codereview.chromium.org/290993005/diff/100001/content/renderer/pepper...
> > content/renderer/pepper/pepper_media_stream_audio_track_host.cc:110:
> > static_cast<int32_t>(buffer_size),
> > On 2014/05/28 17:21:06, dcheng wrote:
> > > On 2014/05/28 17:13:11, thembrown wrote:
> > > > On 2014/05/28 15:55:44, dcheng wrote:
> > > > > Will we need to be concerned about overflow in these calculations in
the
> > > > future?
> > > > 
> > > > Hm - with audio @192kHz, 16Bit, 10ch bytes_per_second_ would be 38.4M,
so
> I
> > > > guess larger values of kMaxDuration could overflow a 32bit size_t in
this
> > > > calculation.
> > > > However the calculation not have changed. I think currently with
> > > kMaxDuration=10
> > > > it should not be safe.
> > > 
> > > We use bytes per second though, not bits.
> > > 
> > > So it's 192KHz * 2 bytes per sample * 32 channels (according to the max
> > channel
> > > limits) = 12,880,000 bytes per second.
> > > 
> > > Combined with a max duration of 10 msec, that means each buffer is at most
> > > 122,800 bytes long. With a max of 1000 buffers, we're still short of
> > overflowing
> > > int32_t. My concern is for when this code is updated in the future to
> increase
> > > bytes per sample/sample rate/number of channels/max buffer duration/etc
> > though.
> > 
> > I was concerned about the intermediate result of (bytes_per_second *
> > kMaxDuration), still safe though. 
> > What do you suggest to avoid possible overflows introduced by future
changes?
> > Add a comment to kMaxDuration? Use int64_t to do the calculation and CHECK
the
> > size of the result? I'm not sure what's appropriate.
> 
> Well, the safest thing to do here would be to use CheckedNumeric (here and in
PepperMediaStreamTrackHostBase and probably PepperMediaStreamVideoTrackHost as
well).

thembrown

[...] > > Well, the safest thing to do here would be to use CheckedNumeric ...

6 years, 6 months ago (2014-06-02 10:44:34 UTC) #16

dcheng

https://codereview.chromium.org/290993005/diff/140001/content/renderer/pepper/pepper_media_stream_track_host_base.cc File content/renderer/pepper/pepper_media_stream_track_host_base.cc (right): https://codereview.chromium.org/290993005/diff/140001/content/renderer/pepper/pepper_media_stream_track_host_base.cc#newcode46 content/renderer/pepper/pepper_media_stream_track_host_base.cc:46: if (!buffer_size_aligned.IsValid() || !size.IsValid()) Nit: you only need to ...

6 years, 6 months ago (2014-06-02 21:57:23 UTC) #18