Add a new parameter |latency| to PPB_Audio.

Add a new parameter |latency| to PPB_Audio.

This CL updates the version of PPB_Audio and PPB_Audio_Callback.

BUG=240900
TEST=updated test_audio.{h,cc}

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

[yzshen1, 2013-08-05 23:37:20]

Hi, Dale and David.

Would you please take a look?

Dale: please look at how latency is calculated and the API definition:
ppapi/shared_impl/* and ppapi/api/ppb_audio.idl.

David: please look at everything.

Thanks!

[DaleCurtis, 2013-08-05 23:52:23]

usage and api lgtm

[dmichael (off chromium), 2013-08-06 19:43:25]

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/ppb_a...
File content/renderer/pepper/ppb_audio_impl.cc (right):

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/ppb_a...
content/renderer/pepper/ppb_audio_impl.cc:161:
EnterResourceNoLock<PPB_AudioConfig_API> enter(config_, true);
I think this could fail if the plugin releases config_ between Init and now. We
should probably just hold a scoped_refptr<PPB_AudioConfig_shared>

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/resou...
File content/renderer/pepper/resource_creation_impl.cc (right):

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/resou...
content/renderer/pepper/resource_creation_impl.cc:48: return
PPB_Audio_Impl::Create(instance, config_id, audio_callback, user_data);
note: You're currently relying on an implicit conversion constructor for The
"AudioCallback"...  It would be better to explicitly convert to AudioCallback
here and below.

https://codereview.chromium.org/22320004/diff/1/ppapi/api/ppb_audio.idl
File ppapi/api/ppb_audio.idl (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/api/ppb_audio.idl#newco...
ppapi/api/ppb_audio.idl:31: [inout] mem_t user_data);
Should we consider defining 2 kinds of callbacks in stable, and having 2
Creates? (e.g., Create and CreateWithLatency). We're preserving ABI
compatibility here but breaking code compatibility for C. I guess it should be
fairly easy to adapt to, but it might be nice to strive for code compatibility.
WDYT?

https://codereview.chromium.org/22320004/diff/1/ppapi/cpp/audio.cc
File ppapi/cpp/audio.cc (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/cpp/audio.cc#newcode43
ppapi/cpp/audio.cc:43: }
nit? : For older code that runs on newer Chrome, this means we'll use the "1.0"
version of Create, but the "1.1" version of StartPlayback and StopPlayback. I
think that should work fine in practice, but it is a little bit weird. We could
try to create using 1.1 with a small passthrough callback that drops the
|latency| parameter.

As the code is written now, it may be harder to ever deprecate 1.0 (though
admittedly we may not want to do that).

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
ppapi/shared_impl/ppb_audio_shared.cc:45: void* user_data) const {
Should we DCHECK(callback_ || callback_1_0_)? Or else { NOTREACHED() }?

It seems like Run shouldn't happen for a default AudioCallback

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
ppapi/shared_impl/ppb_audio_shared.h:24: class PPAPI_SHARED_EXPORT AudioCallback
{
We used the suffix "Combined" in another place where we have >1 way to talk to
the plugin...  PPPInstanceCombined. It might be helpful to follow that
convention here.

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
ppapi/shared_impl/ppb_audio_shared.h:28: AudioCallback(PPB_Audio_Callback
callback);
nit: explicit for both of these

[yzshen1, 2013-08-07 20:51:06]

Thanks David!
Please take another look.

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/ppb_a...
File content/renderer/pepper/ppb_audio_impl.cc (right):

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/ppb_a...
content/renderer/pepper/ppb_audio_impl.cc:161:
EnterResourceNoLock<PPB_AudioConfig_API> enter(config_, true);
This class also holds a ref to config_, so it should be okay.
What do you think?


On 2013/08/06 19:43:25, dmichael wrote:
> I think this could fail if the plugin releases config_ between Init and now.
We
> should probably just hold a scoped_refptr<PPB_AudioConfig_shared>

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/resou...
File content/renderer/pepper/resource_creation_impl.cc (right):

https://codereview.chromium.org/22320004/diff/1/content/renderer/pepper/resou...
content/renderer/pepper/resource_creation_impl.cc:48: return
PPB_Audio_Impl::Create(instance, config_id, audio_callback, user_data);
On 2013/08/06 19:43:25, dmichael wrote:
> note: You're currently relying on an implicit conversion constructor for The
> "AudioCallback"...  It would be better to explicitly convert to AudioCallback
> here and below.

Done.

https://codereview.chromium.org/22320004/diff/1/ppapi/api/ppb_audio.idl
File ppapi/api/ppb_audio.idl (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/api/ppb_audio.idl#newco...
ppapi/api/ppb_audio.idl:31: [inout] mem_t user_data);
I am a little bit concerned about that, because making two Creates bloats the
API. And the preferred way has to have a longer name CreateWithLatency().

If we think the callback with latency information makes more sense, it seems
okay to deprecate the old callback in new versions of APIs.

IMO (I could be wrong), we don't promise that a new version of API is a superset
of the older APIs, right?

If a user decides to use PPB_AUDIO_INTERFACE (instead of
PPB_AUDIO_INTERFACE_1_0) to query for interface, it means they would like to
compile with the latest API, and it is their responsibility to make necessary
modifications when they update the SDK they compile with.
If they use PPB_AUDIO_INTERFACE_1_0 to query for interface, there won't be a
problem.

What do you think?

On 2013/08/06 19:43:25, dmichael wrote:
> Should we consider defining 2 kinds of callbacks in stable, and having 2
> Creates? (e.g., Create and CreateWithLatency). We're preserving ABI
> compatibility here but breaking code compatibility for C. I guess it should be
> fairly easy to adapt to, but it might be nice to strive for code
compatibility.
> WDYT?

https://codereview.chromium.org/22320004/diff/1/ppapi/cpp/audio.cc
File ppapi/cpp/audio.cc (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/cpp/audio.cc#newcode43
ppapi/cpp/audio.cc:43: }
On 2013/08/06 19:43:25, dmichael wrote:
> nit? : For older code that runs on newer Chrome, this means we'll use the
"1.0"
> version of Create, but the "1.1" version of StartPlayback and StopPlayback. I
> think that should work fine in practice, but it is a little bit weird.
Good point. Thanks for catching this!

I added a field to keep track of whether the resource is created using the 1.0
interface and then call the corresponding methods for StartPlayback() and
StopPlayback().

> We could
> try to create using 1.1 with a small passthrough callback that drops the
> |latency| parameter.

It seems hard to make a small passthrough callback to drop the |latency|
parameter: I have to create a data structure to hold information about the
original callback and user_data. It cannot be stored in the Audio object,
because others can hold refs to the same PP_Resource.
There is no way for us to tell when we can delete that data structure.
Do you have some good idea to solve this?

> 
> As the code is written now, it may be harder to ever deprecate 1.0 (though
> admittedly we may not want to do that).

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
ppapi/shared_impl/ppb_audio_shared.cc:45: void* user_data) const {
On 2013/08/06 19:43:25, dmichael wrote:
> Should we DCHECK(callback_ || callback_1_0_)? Or else { NOTREACHED() }?
> 
> It seems like Run shouldn't happen for a default AudioCallback

Done.

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
ppapi/shared_impl/ppb_audio_shared.h:24: class PPAPI_SHARED_EXPORT AudioCallback
{
On 2013/08/06 19:43:25, dmichael wrote:
> We used the suffix "Combined" in another place where we have >1 way to talk to
> the plugin...  PPPInstanceCombined. It might be helpful to follow that
> convention here.

Done.

https://codereview.chromium.org/22320004/diff/1/ppapi/shared_impl/ppb_audio_s...
ppapi/shared_impl/ppb_audio_shared.h:28: AudioCallback(PPB_Audio_Callback
callback);
On 2013/08/06 19:43:25, dmichael wrote:
> nit: explicit for both of these

Done.

[dmichael (off chromium), 2013-08-07 21:11:37]

lgtm, thanks!

https://chromiumcodereview.appspot.com/22320004/diff/1/content/renderer/peppe...
File content/renderer/pepper/ppb_audio_impl.cc (right):

https://chromiumcodereview.appspot.com/22320004/diff/1/content/renderer/peppe...
content/renderer/pepper/ppb_audio_impl.cc:161:
EnterResourceNoLock<PPB_AudioConfig_API> enter(config_, true);
On 2013/08/07 20:51:06, yzshen1 wrote:
> This class also holds a ref to config_, so it should be okay.
> What do you think?
> 
> 
> On 2013/08/06 19:43:25, dmichael wrote:
> > I think this could fail if the plugin releases config_ between Init and now.
> We
> > should probably just hold a scoped_refptr<PPB_AudioConfig_shared>
> 
Thanks, I missed that it's a ScopedPPResource instead of PP_Resource.

https://chromiumcodereview.appspot.com/22320004/diff/1/ppapi/cpp/audio.cc
File ppapi/cpp/audio.cc (right):

https://chromiumcodereview.appspot.com/22320004/diff/1/ppapi/cpp/audio.cc#new...
ppapi/cpp/audio.cc:43: }
On 2013/08/07 20:51:06, yzshen1 wrote:
> On 2013/08/06 19:43:25, dmichael wrote:
> > nit? : For older code that runs on newer Chrome, this means we'll use the
> "1.0"
> > version of Create, but the "1.1" version of StartPlayback and StopPlayback.
I
> > think that should work fine in practice, but it is a little bit weird.
> Good point. Thanks for catching this!
> 
> I added a field to keep track of whether the resource is created using the 1.0
> interface and then call the corresponding methods for StartPlayback() and
> StopPlayback().
> 
> > We could
> > try to create using 1.1 with a small passthrough callback that drops the
> > |latency| parameter.
> 
> It seems hard to make a small passthrough callback to drop the |latency|
> parameter: I have to create a data structure to hold information about the
> original callback and user_data. It cannot be stored in the Audio object,
> because others can hold refs to the same PP_Resource.
> There is no way for us to tell when we can delete that data structure.
> Do you have some good idea to solve this?
> 
No, the way you have it seems fine to me.

[dmichael (off chromium), 2013-08-07 21:13:02]

Oh, I think noelallen will want to look at this. I think he might be more
against breaking C source compatibility than I am. Also affects SDK in general.

[yzshen1, 2013-08-07 21:17:53]

On 2013/08/07 21:13:02, dmichael wrote:
> Oh, I think noelallen will want to look at this. I think he might be more
> against breaking C source compatibility than I am. Also affects SDK in
general.

Thanks! I will wait for Noel's comments.
And also I am going to send a mail to pepper-team@ because this is a stable API
change.

[noelallen_use_chromium, 2013-08-07 23:23:10]

WRT api...

The API seems okay.  If you are interested in latency data, you request v1_1, if
not v1_0.

But is Delta the correct information instead of an Absolute?

If my read of this is correct, we are sending the amount of data left in the
output Q across the sync socket?

If the thread gets interrupted between sampling pending_data, and when it send
it on the sync socket, this value could be off.

Now we need to wake the thread in the NaCl process.  How long does it take to
signal and schedule this thread?

So my question is, does it make more sense to share an absolute clock (if
possible) between the sender and receiver, and stamp absolute time, or does it
make more sense to send a delta, but not provide any information about how "off"
the delta is from moment at which you actually get the callback.

Is this fine enough accuracy?

[DaleCurtis, 2013-08-08 00:32:01]

Latency indicates the amount of time until any data returned during this call
will be played back. It's not simply the amount in the chrome side queue and
includes information from the hardware queues.

[noelallen_use_chromium, 2013-08-08 01:19:03]

To clarify, my question is not about the accuracy at the moment the delta is
computed.  My question is, does the latency and jitter experienced between when
the delta is computed and when it's seen by the callback in the other
process/thread invalidate the accuracy of the computed delta?

I don't have numbers for sync latency and jitter in our IPC for the various
platforms, nor numbers for expected accuracy.

How are we ensuring that the value we pass is usable?

https://codereview.chromium.org/22320004/diff/25001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/22320004/diff/25001/ppapi/tests/test_audio.cc...
ppapi/tests/test_audio.cc:355: if (latency < 0)
Is there a useful value we can check here?
For example, at the extreme, wouldn't it be an error for latency to be < 1
sample worth of time, so that it's impossible to fill the buffer before it must
be played?

Should we be getting at least 1/2 buffer_size_in_bytes worth of time?

[DaleCurtis, 2013-08-08 01:25:49]

The latency information is delivered via a SyncSocket and not an IPC, so it
should be really quick; ~ < 1ms. The latency value may range from ~0-42ms.

[DaleCurtis, 2013-08-08 01:27:07]

We don't have an automated test for latency, but it's pretty noticeable when it
goes wrong; i.e, audio and video playback are out of sync.

[yzshen1, 2013-08-08 17:23:57]

Thanks, Noel and Dale! (And sorry for late reply.)

WRT the question about delta v.s. absolute:
I think actually it is two questions:
(1) does the current implementation give an accurate-enough delta value?
I think Dale has answered that. Besides, I think WebRTC is using the same delta.
(2) is delta a better choice from a API-design perspective?
I personally think it is. It is harder for plugins if we give them an absolute
value and ask them to compare with some absolute clock.
If we think the current way of calculating delta is not accurate enough, we
could improve the internal implementation. For example, we could pass an
absolute timestamp across processes as you said, and right before we call into
the plugin, we convert it to a delta.

https://codereview.chromium.org/22320004/diff/25001/ppapi/tests/test_audio.cc
File ppapi/tests/test_audio.cc (right):

https://codereview.chromium.org/22320004/diff/25001/ppapi/tests/test_audio.cc...
ppapi/tests/test_audio.cc:355: if (latency < 0)
On 2013/08/08 01:19:03, noelallen wrote:
> Is there a useful value we can check here?
> For example, at the extreme, wouldn't it be an error for latency to be < 1
> sample worth of time, so that it's impossible to fill the buffer before it
must
> be played?

Could that happen if the browser process is too busy and the queue has been
empty before the browser gets a chance to ask data from the plugin?
In that case, the playback will be intermittent. But it is a case that could
happen in the real world.

I also wish I could do a better test. I will think about it. Thanks!

> 
> Should we be getting at least 1/2 buffer_size_in_bytes worth of time?

[commit-bot: I haz the power, 2013-09-05 23:27:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/22320004/85001

[commit-bot: I haz the power, 2013-09-05 23:36:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-06 17:23:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/22320004/102001

[commit-bot: I haz the power, 2013-09-06 21:46:39]

Change committed as 221788