Work on improving PpbAudioConfig:RecommendSampleFrameCount

Work on improving PpbAudioConfig:RecommendSampleFrameCount
Add version 1.1 which will query the audio back end for the best available
sample frame count. Also add RecommendSampleRate.  Switch pepper plugin
delegate to use AUDIO_PCM_LOW_LATENCY if client request is compatible.

TEST=included
BUG=http://code.google.com/p/chromium/issues/detail?id=107572
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=122653

[dmichael (off chromium), 2012-01-24 16:54:37]

You'll probably also want to modify the SDK example
(src/native_client_sdk/src/examples)

http://codereview.chromium.org/9129007/diff/4002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9129007/diff/4002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:235: uint32_t sample_rate,
uint32_t sample_count,
I was confused before when you asked me if we had any way "in WebKit" to handle
different versions separately. In src/webkit/plugins/ppapi, we can certainly
handle that, though it is mostly manual (having distinct methods suffixed with
the version number, which are called appropriately from the thunk layer).

I think it would be safer to do that, even if it requires plumbing to get the
information to this point of the code. It might even be worth wrapping up the
sample rate and sample count with the version information in to an internal
"AudioConfig" or "AudioParams" object so you don't have to touch so many
function signatures next time you have to make this kind of change.

http://codereview.chromium.org/9129007/diff/4002/ppapi/api/ppb_audio_config.idl
File ppapi/api/ppb_audio_config.idl (right):

http://codereview.chromium.org/9129007/diff/4002/ppapi/api/ppb_audio_config.i...
ppapi/api/ppb_audio_config.idl:94: * the same function signature but with
different behavior.
FWIW, it looks like the interface did change to require PP_Instance.

[nfullagar, 2012-01-24 19:14:58]

http://codereview.chromium.org/9129007/diff/4002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9129007/diff/4002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:235: uint32_t sample_rate,
uint32_t sample_count,
I did try a couple of dry runs to plumb more information regarding which
interface (new or old) called down to this function, but it ended up touching so
many files that in both cases I backed out.  I agree that long term we'll want
some pattern or mechanism to do this for ppapi (in general, not just audio,) but
that's beyond the scope of this CL.  I've talked to Chris and Erik about the
approach taken in this CL; it seems reasonable (and some of this approach was
suggested by Chris, iirc.)  The main concern is if an application built to the
1.0 interface happened to use the combination of recommended sample rate and
sample frame count, it might get the new back end on some platforms instead of
the old back end.  But from the application's perspective, the behavior should
be the same.

On 2012/01/24 16:54:38, dmichael wrote:
> I was confused before when you asked me if we had any way "in WebKit" to
handle
> different versions separately. In src/webkit/plugins/ppapi, we can certainly
> handle that, though it is mostly manual (having distinct methods suffixed with
> the version number, which are called appropriately from the thunk layer).
> 
> I think it would be safer to do that, even if it requires plumbing to get the
> information to this point of the code. It might even be worth wrapping up the
> sample rate and sample count with the version information in to an internal
> "AudioConfig" or "AudioParams" object so you don't have to touch so many
> function signatures next time you have to make this kind of change.

http://codereview.chromium.org/9129007/diff/4002/ppapi/api/ppb_audio_config.idl
File ppapi/api/ppb_audio_config.idl (right):

http://codereview.chromium.org/9129007/diff/4002/ppapi/api/ppb_audio_config.i...
ppapi/api/ppb_audio_config.idl:94: * the same function signature but with
different behavior.
thx, comment updated.

On 2012/01/24 16:54:38, dmichael wrote:
> FWIW, it looks like the interface did change to require PP_Instance.

[brettw, 2012-02-10 19:01:33]

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_config_rpc_server.cc
(right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_config_rpc_server.cc:17:
static const PPB_AudioConfig_1_0* audioConfig =
style: audioConfig -> audio_config

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc:204: NaClSrpcError
PpbAudioConfigRpcClient::PPB_AudioConfig_RecommendSampleFrameCount_1_0(
I'd wrap at the space.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/proxy/ppb_ins...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/proxy/ppb_ins...
ppapi/proxy/ppb_instance_proxy.cc:191: uint32_t PPB_Instance_Proxy::
Style: Don't wrap after the :: if you don't need to, wrap the arg to 4 spaces.
Same below.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/proxy/ppb_ins...
ppapi/proxy/ppb_instance_proxy.cc:194: dispatcher()->Send(new
PpapiHostMsg_PPBInstance_AudioHardwareOutputSampleRate(
Can the messages and handlers be made to match the function (...Recommend...).
The way you have this looks like the sample rate is being broadcast rather than
retrieved.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_audio_config_shared.h (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.h:28: static uint32_t
Wrap args to 4 spaces, put the function name on the previous line.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/thunk/ppb_aud...
File ppapi/thunk/ppb_audio_config_thunk.cc (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/thunk/ppb_aud...
ppapi/thunk/ppb_audio_config_thunk.cc:40: EnterInstance enter(instance);
The thunks shouldn't have actual code in them. I want to autogenerate these in
the future. So this should go to a function in Instance_API with the same name
with this logic.

https://chromiumcodereview.appspot.com/9129007/diff/46010/webkit/plugins/ppap...
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_delegate.h:308: virtual uint32_t
AudioHardwareOutputSampleRate() = 0;
Can this be called "GetAudio..." (same for the other function).

[nfullagar, 2012-02-14 00:42:37]

new upload, ptal thx.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_config_rpc_server.cc
(right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_config_rpc_server.cc:17:
static const PPB_AudioConfig_1_0* audioConfig =
On 2012/02/10 19:01:33, brettw wrote:
> style: audioConfig -> audio_config

Done.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
File ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/native_client...
ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc:204: NaClSrpcError
PpbAudioConfigRpcClient::PPB_AudioConfig_RecommendSampleFrameCount_1_0(
On 2012/02/10 19:01:33, brettw wrote:
> I'd wrap at the space.
this is autogenerated code...

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/proxy/ppb_ins...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/proxy/ppb_ins...
ppapi/proxy/ppb_instance_proxy.cc:191: uint32_t PPB_Instance_Proxy::
On 2012/02/10 19:01:33, brettw wrote:
> Style: Don't wrap after the :: if you don't need to, wrap the arg to 4 spaces.
> Same below. 

Done.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/proxy/ppb_ins...
ppapi/proxy/ppb_instance_proxy.cc:194: dispatcher()->Send(new
PpapiHostMsg_PPBInstance_AudioHardwareOutputSampleRate(
On 2012/02/10 19:01:33, brettw wrote:
> Can the messages and handlers be made to match the function (...Recommend...).
> The way you have this looks like the sample rate is being broadcast rather
than
> retrieved.
Changed to GetAudio... (as you suggested in a later comment)

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_audio_config_shared.h (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.h:28: static uint32_t
On 2012/02/10 19:01:33, brettw wrote:
> Wrap args to 4 spaces, put the function name on the previous line.

Done.

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/thunk/ppb_aud...
File ppapi/thunk/ppb_audio_config_thunk.cc (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/ppapi/thunk/ppb_aud...
ppapi/thunk/ppb_audio_config_thunk.cc:40: EnterInstance enter(instance);
On 2012/02/10 19:01:33, brettw wrote:
> The thunks shouldn't have actual code in them. I want to autogenerate these in
> the future. So this should go to a function in Instance_API with the same name
> with this logic.

code moved, shared_impl func takes the same args as thunk.

https://chromiumcodereview.appspot.com/9129007/diff/46010/webkit/plugins/ppap...
File webkit/plugins/ppapi/plugin_delegate.h (right):

https://chromiumcodereview.appspot.com/9129007/diff/46010/webkit/plugins/ppap...
webkit/plugins/ppapi/plugin_delegate.h:308: virtual uint32_t
AudioHardwareOutputSampleRate() = 0;
On 2012/02/10 19:01:33, brettw wrote:
> Can this be called "GetAudio..." (same for the other function).

Done.

[dmichael (off chromium), 2012-02-14 17:19:24]

LGTM, but you might want to wait for brettw to look again.

http://codereview.chromium.org/9129007/diff/59001/ppapi/shared_impl/ppb_audio...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

http://codereview.chromium.org/9129007/diff/59001/ppapi/shared_impl/ppb_audio...
ppapi/shared_impl/ppb_audio_config_shared.cc:75:
enter.functions()->GetAudioHardwareOutputSampleRate(instance));
nit: 4-space indent

http://codereview.chromium.org/9129007/diff/59001/ppapi/shared_impl/ppb_audio...
ppapi/shared_impl/ppb_audio_config_shared.cc:77:
enter.functions()->GetAudioHardwareOutputBufferSize(instance);
nit: 4-space indent

[dmichael (off chromium), 2012-02-14 17:20:11]

On 2012/02/14 17:19:24, dmichael wrote:
> LGTM, but you might want to wait for brettw to look again.
> 
>
http://codereview.chromium.org/9129007/diff/59001/ppapi/shared_impl/ppb_audio...
> File ppapi/shared_impl/ppb_audio_config_shared.cc (right):
> 
>
http://codereview.chromium.org/9129007/diff/59001/ppapi/shared_impl/ppb_audio...
> ppapi/shared_impl/ppb_audio_config_shared.cc:75:
> enter.functions()->GetAudioHardwareOutputSampleRate(instance));
> nit: 4-space indent
> 
>
http://codereview.chromium.org/9129007/diff/59001/ppapi/shared_impl/ppb_audio...
> ppapi/shared_impl/ppb_audio_config_shared.cc:77:
> enter.functions()->GetAudioHardwareOutputBufferSize(instance);
> nit: 4-space indent
Oh yeah, I forgot, you should update the SDK sine_synth example in
native_client_sdk. If you don't, this CL will break the SDK bots.

[brettw, 2012-02-14 18:50:18]

LGTM

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

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:73: // Get the hardware config.
Can you put a blank line before this?

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:78: // Clamp the input.
This comment is probably not necessary

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:81: // If client is using same
sample rate as audio hardware, then recommend a
I'd also do a blank line above this comment...

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:92: // Otherwise, recommend a
conservative 30ms buffer based on sample rate.
...and here as well.

[nfullagar, 2012-02-14 23:21:22]

thx, on its way to the trybots

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

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:73: // Get the hardware config.
On 2012/02/14 18:50:18, brettw wrote:
> Can you put a blank line before this?

Done.

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:75:
enter.functions()->GetAudioHardwareOutputSampleRate(instance));
On 2012/02/14 17:19:24, dmichael wrote:
> nit: 4-space indent

Done.

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:77:
enter.functions()->GetAudioHardwareOutputBufferSize(instance);
On 2012/02/14 17:19:24, dmichael wrote:
> nit: 4-space indent

Done.

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:78: // Clamp the input.
On 2012/02/14 18:50:18, brettw wrote:
> This comment is probably not necessary

Done.

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:81: // If client is using same
sample rate as audio hardware, then recommend a
On 2012/02/14 18:50:18, brettw wrote:
> I'd also do a blank line above this comment...

Done.

https://chromiumcodereview.appspot.com/9129007/diff/59001/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_audio_config_shared.cc:92: // Otherwise, recommend a
conservative 30ms buffer based on sample rate.
On 2012/02/14 18:50:18, brettw wrote:
> ...and here as well.

Done.

[commit-bot: I haz the power, 2012-02-16 22:11:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nfullagar@google.com/9129007/77002

[commit-bot: I haz the power, 2012-02-16 22:11:24]

Can't apply patch for file ppapi/c/ppb_audio_config.h.
While running patch -p0 --forward --force;
patching file ppapi/c/ppb_audio_config.h
Hunk #1 FAILED at 3.
1 out of 6 hunks FAILED -- saving rejects to file ppapi/c/ppb_audio_config.h.rej

[commit-bot: I haz the power, 2012-02-17 01:28:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nfullagar@google.com/9129007/74005

[commit-bot: I haz the power, 2012-02-17 01:28:54]

Presubmit check for 9129007-74005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_client.cc, line 204, 85
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 2956, 109
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 2959, 95
chars

License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_audio_config.h

Presubmit checks took 5.8s to calculate.

[nfullagar, 2012-02-18 01:11:35]

manually committed -
Committed revision 122653.

[viettrungluu, 2012-02-22 19:17:12]

I filed http://crbug.com/115305 .

http://codereview.chromium.org/9129007/diff/66012/ppapi/api/ppb_audio_config.idl
File ppapi/api/ppb_audio_config.idl (right):

http://codereview.chromium.org/9129007/diff/66012/ppapi/api/ppb_audio_config....
ppapi/api/ppb_audio_config.idl:13: M18 = 1.1
This is wrong, unless you merged it into M18 (which seems unlikely).

[nfullagar, 2012-02-22 19:35:32]

good catch, CL was started for m18 but didn't make the cutoff.  Sending out
the fix shortly.

On Wed, Feb 22, 2012 at 11:17 AM, <viettrungluu@chromium.org> wrote:

> I filed http://crbug.com/115305 .
>
>
> http://codereview.chromium.**org/9129007/diff/66012/ppapi/**
>
api/ppb_audio_config.idl<http://codereview.chromium.org/9129007/diff/66012/ppapi/api/ppb_audio_config.idl>
> File ppapi/api/ppb_audio_config.idl (right):
>
> http://codereview.chromium.**org/9129007/diff/66012/ppapi/**
>
api/ppb_audio_config.idl#**newcode13<http://codereview.chromium.org/9129007/diff/66012/ppapi/api/ppb_audio_config.idl#newcode13>
> ppapi/api/ppb_audio_config.**idl:13: M18 = 1.1
> This is wrong, unless you merged it into M18 (which seems unlikely).
>
>
http://codereview.chromium.**org/9129007/<http://codereview.chromium.org/9129...
>