Improve recommended sample counts for PPAPI.

Improve recommended sample counts for PPAPI.

Testing shows we can allow for much lower buffer sizes on WASAPI,
OSX, and PulseAudio clients.  In most cases, this change brings those
clients down to the nearest multiple of 512.  WaveOut and ALSA clients
will use the nearest multiple of 2048.

Based on testing done in https://codereview.chromium.org/14570002/

BUG=162207
TEST=Using these buffer sizes for HTML5 audio works without glitching
when an AudioConverter is in the graph.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202775

[DaleCurtis, 2013-05-01 22:59:55]

Still needs a bunch of testing, but this is the general idea. WDYT?

[brettw, 2013-05-02 21:13:15]

I don't have enough backround to know if this is correct or not. I don't see
anything that looks wrong, however. Is there an audio person who can review
this?

[DaleCurtis, 2013-05-02 21:35:41]

+crogers, +henrika for audio correctness review.  Is there any worry about
impacting existing users of this function (who now might get a smaller buffer
size?)

[brettw, 2013-05-02 22:40:35]

I'm not that concerned, as this is supposed to be platform-specific. It's
possible we'll break somebody, but for this I'd rather worry about that
reactively.

[DaleCurtis, 2013-05-03 17:19:55]

sgtm.  Do you know of any PPAPI apps which use this API today? I thought Bastion
or NaCl-Box might, but no luck.  A web search on the API isn't turning up
anything useful either.

[brettw, 2013-05-03 19:39:58]

No

[Chris Rogers, 2013-05-03 19:55:05]

Thanks Dale, as a followup to this CL I'd recommend that we elevate the audio
thread priority for *trusted* PPAPI, similar to what we do in
media/audio/audio_device_thread.cc

  PlatformThread::CreateWithPriority(0, this, &thread_,
                                     base::kThreadPriority_RealtimeAudio);

When we start getting down to buffer-sizes of 512 it will improve the
resilience.

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:88: NOTREACHED() << "Unsupported
sample rate, please update "
I could be wrong, but isn't NOTREACHED() supposed to only be used in cases where
the codepath can (or should) never be reached unless there is a bug in the code.
 It's essentially equivalent to DCHECK (or ASSERT in Webkit/Blink)?

For situations which *can* actually occur in run-time, maybe a DLOG?

[DaleCurtis, 2013-05-03 20:02:08]

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:88: NOTREACHED() << "Unsupported
sample rate, please update "
On 2013/05/03 19:55:06, Chris Rogers wrote:
> I could be wrong, but isn't NOTREACHED() supposed to only be used in cases
where
> the codepath can (or should) never be reached unless there is a bug in the
code.
>  It's essentially equivalent to DCHECK (or ASSERT in Webkit/Blink)?
> 
> For situations which *can* actually occur in run-time, maybe a DLOG?

There's PP_DCHECK() which wraps assert().  If that's okay to use here, I'll
convert this appropriately.

[henrika (OOO until Aug 14), 2013-05-06 07:49:22]

"+crogers, +henrika for audio correctness review.  Is there any worry about
impacting existing users of this function (who now might get a smaller buffer
size?)"

I'm not sure if I understand the question Dale; can you please elaborate.

[DaleCurtis, 2013-05-06 16:52:00]

Essentially just: Given your knowledge of your respective platforms (windows,
osx), does this logic looks reasonable for choosing client buffer sizes which
need resampling?

The data I've collected supports this change, but I'd like to draw upon your
expertise in case I've missed something.

Also, more eyes on code is never bad :)

[henrika (OOO until Aug 14), 2013-05-13 08:11:56]

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:117: const uint32_t
kLowLatencySampleFrameCount = 512;
Would it be beneficial to do as for WebRTC and always open up at the preferred
buffer size (e.g. 441, 447 or 448 for 44.1kHz) and then do any buffer-size
compensation on the render side. In WebRTC, that ensures that we never use the
AudioConverter FIFO (should lead to best callback timing over SyncSocket+shmem I
assume).

[DaleCurtis, 2013-05-13 21:57:36]

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:117: const uint32_t
kLowLatencySampleFrameCount = 512;
On 2013/05/13 08:11:56, henrika wrote:
> Would it be beneficial to do as for WebRTC and always open up at the preferred
> buffer size (e.g. 441, 447 or 448 for 44.1kHz) and then do any buffer-size
> compensation on the render side. In WebRTC, that ensures that we never use the
> AudioConverter FIFO (should lead to best callback timing over SyncSocket+shmem
I
> assume).

We could provide AudioConverter to PPAPI, but that's a much larger change and
requires support for renderer side device changes.

[henrika (OOO until Aug 14), 2013-05-14 07:29:35]

LGTM.

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:119: // Special case for 48kHz ->
44.1kHz and buffer sizes too large.  Which
This sentence is a bit vague IMO. Could it be clarified? Especially the "buffer
sizes too large" part feels unclear.

[DaleCurtis, 2013-05-21 23:16:43]

Comments addressed.  PTAL.  Automated test ideas appreciated.

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:88: NOTREACHED() << "Unsupported
sample rate, please update "
On 2013/05/03 20:02:08, DaleCurtis wrote:
> On 2013/05/03 19:55:06, Chris Rogers wrote:
> > I could be wrong, but isn't NOTREACHED() supposed to only be used in cases
> where
> > the codepath can (or should) never be reached unless there is a bug in the
> code.
> >  It's essentially equivalent to DCHECK (or ASSERT in Webkit/Blink)?
> > 
> > For situations which *can* actually occur in run-time, maybe a DLOG?
> 
> There's PP_DCHECK() which wraps assert().  If that's okay to use here, I'll
> convert this appropriately.

Actually cpp/ which defines PP_DCHECK is excluded from shared_impl/ DEPS, so
this the best I can do.  Technically this check is already done during Init() so
I've just added a comment there and removed this section.

https://codereview.chromium.org/14573006/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:119: // Special case for 48kHz ->
44.1kHz and buffer sizes too large.  Which
On 2013/05/14 07:29:35, henrika wrote:
> This sentence is a bit vague IMO. Could it be clarified? Especially the
"buffer
> sizes too large" part feels unclear.

Cleaned up a bit.  Linux part is unnecessary.

[DaleCurtis, 2013-05-23 23:58:15]

Ping?

[brettw, 2013-05-28 23:26:34]

owners lgtm rubberstamp

[Chris Rogers, 2013-05-29 01:15:27]

lgtm having discussed with Dale offline

[commit-bot: I haz the power, 2013-05-29 01:16:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/14573006/21001

[commit-bot: I haz the power, 2013-05-29 04:17:10]

Change committed as 202775