Classify ARM Chromebooks as high latency audio.

Classify ARM Chromebooks as high latency audio.

BUG=288367, 289770
TEST=Ran on Daisy, checked Pepper Flash gets 2048 now instead of 1024.

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

[ilja, 2013-09-12 04:28:43]

Please take a look.

[DaleCurtis, 2013-09-12 18:12:53]

lgtm, but you'll need a pepper owner.

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

https://codereview.chromium.org/23672035/diff/1/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:87: #if defined(OS_CHROMEOS) &&
defined(ARCH_CPU_ARM_FAMILY)
might need #include build/build_config.h

[ilja, 2013-09-13 20:22:43]

dmichael: OWNERS review please.

https://chromiumcodereview.appspot.com/23672035/diff/1/ppapi/shared_impl/ppb_...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://chromiumcodereview.appspot.com/23672035/diff/1/ppapi/shared_impl/ppb_...
ppapi/shared_impl/ppb_audio_config_shared.cc:87: #if defined(OS_CHROMEOS) &&
defined(ARCH_CPU_ARM_FAMILY)
On 2013/09/12 18:12:53, DaleCurtis wrote:
> might need #include build/build_config.h

Done.

[dmichael (off chromium), 2013-09-13 20:44:46]

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

https://codereview.chromium.org/23672035/diff/2/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:90: // now we classify them as high
latency. See crbug.com/289770.
This doesn't seem like the right place to fix this. Pepper shouldn't need to
know much about specific hardware; that should usually be shared. What about
AudioHardwareConfig?:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_h...

[DaleCurtis, 2013-09-13 20:49:57]

Only PPAPI should use this larger value on ARM.  HTML5, WebRTC, WebAudio have
the advantage of realtime threads.

[dmichael (off chromium), 2013-09-13 21:15:55]

A few suggestions (you guys understand this code better than I, so take them or
leave them).

lgtm

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

https://codereview.chromium.org/23672035/diff/2/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:92: sample_frame_count,
kHighLatencySampleFrameCount);
should we copy the high latency section below and use:
std::max(kHighLatencySampleFrameCount, hardware_sample_frame_count);
?

Or add another way in to that "if" condition, so we don't need this separate
call? E.g.:
#if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARM_FAMILY)
const bool kHighLatencyDevice = true;
#else
const bool kHighLatencyDevice = true;
#endif

and edit 106 to say
if (kHighLatencyDevice ||
    //... etc ...

Also, it would be nice to comment on why we do this platform checking here
instead of at a lower level.

[ilja, 2013-09-14 01:44:39]

Dale, I had to reorder the conditions otherwise I followed dmichaels
suggestions. Can you take another look?

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

https://codereview.chromium.org/23672035/diff/2/ppapi/shared_impl/ppb_audio_c...
ppapi/shared_impl/ppb_audio_config_shared.cc:92: sample_frame_count,
kHighLatencySampleFrameCount);
I followed the second approach and left a comment.

[DaleCurtis, 2013-09-16 16:27:46]

https://codereview.chromium.org/23672035/diff/9002/ppapi/shared_impl/ppb_audi...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/23672035/diff/9002/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_config_shared.cc:92: const bool kHighLatencyDevice =
true;
indent is wrong.

https://codereview.chromium.org/23672035/diff/9002/ppapi/shared_impl/ppb_audi...
ppapi/shared_impl/ppb_audio_config_shared.cc:110: // If client is using same
sample rate as audio hardware, then recommend a
This will result in a larger than necessary buffer size for clients with hw_rate
< 44.1kHz.  There aren't a lot of those and things will still work fine in the
Flash case, but its possible (future, currently non-existent) PPAPI clients may
see higher latency.

[ilja, 2013-09-16 19:15:33]

https://chromiumcodereview.appspot.com/23672035/diff/9002/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://chromiumcodereview.appspot.com/23672035/diff/9002/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:92: const bool kHighLatencyDevice =
true;
On 2013/09/16 16:27:46, DaleCurtis wrote:
> indent is wrong.

Done.

https://chromiumcodereview.appspot.com/23672035/diff/9002/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:110: // If client is using same
sample rate as audio hardware, then recommend a
Thanks for clarifying!

[DaleCurtis, 2013-09-16 19:17:42]

lgtm

[commit-bot: I haz the power, 2013-09-16 19:22:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ihf@chromium.org/23672035/24001

[commit-bot: I haz the power, 2013-09-16 21:47:07]

Change committed as 223428