Supporting a playbackTime attribute on AudioprocessingEvent.

Supporting a playbackTime attribute on AudioprocessingEvent.

According to specification, onaudioprocess() evenhandler should delever playbackTime attribute to JS.
The time when the audio will be played in the same time coordinate system as AudioContext.currentTime.
playbackTime allows for very tight synchronization between processing directly in JavaScript
with the other events in the context's rendering graph.

BUG=332782
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170505

[KhNo, 2014-03-25 17:55:00]

Raymond,
Could you review this patch?

[KhNo, 2014-03-25 17:56:19]

rtoy@chromium.org: Please review changes in

[Raymond Toy, 2014-03-26 17:57:36]

On 2014/03/25 17:56:19, KhNo wrote:
> mailto:rtoy@chromium.org: Please review changes in

In general, this looks good, but I need to look discuss this with Chris Wilson
to figure out exactly what the playbackTime should be.

[KhNo, 2014-03-27 00:47:40]

On 2014/03/26 17:57:36, Raymond Toy wrote:
> On 2014/03/25 17:56:19, KhNo wrote:
> > mailto:rtoy@chromium.org: Please review changes in
> 
> In general, this looks good, but I need to look discuss this with Chris Wilson
> to figure out exactly what the playbackTime should be.

I'm not sure exactly what spec is intended from playbackTime.
But in my guessing about one of use case, I think that is kind of debugging tool
for JS developer.
When onaudioprocess event handler is called, developer might want checking
how long processing of inputbuffer takes to avoid unexpected glitch sound since
JS processing is too heavy.
It means a JS developer can arrange scriptprocessnode.buffersize to smaller with
checking middle of handler.
It is just my guessing. Please share Chris's idea simply to me. :)

[Raymond Toy, 2014-03-27 18:45:47]

On 2014/03/27 00:47:40, KhNo wrote:
> On 2014/03/26 17:57:36, Raymond Toy wrote:
> > On 2014/03/25 17:56:19, KhNo wrote:
> > > mailto:rtoy@chromium.org: Please review changes in
> > 
> > In general, this looks good, but I need to look discuss this with Chris
Wilson
> > to figure out exactly what the playbackTime should be.
> 
> I'm not sure exactly what spec is intended from playbackTime.
> But in my guessing about one of use case, I think that is kind of debugging
tool
> for JS developer.
> When onaudioprocess event handler is called, developer might want checking
> how long processing of inputbuffer takes to avoid unexpected glitch sound
since
> JS processing is too heavy.
> It means a JS developer can arrange scriptprocessnode.buffersize to smaller
with
> checking middle of handler.
> It is just my guessing. Please share Chris's idea simply to me. :)

Ok, I talked this over with Chris. We didn't discuss how users can use
playbackTime; we mostly looked at what the correct value should be.  We decided
that playbackTime is the time we expect the current data would be expected to
reach the destination node. 

Because of the double buffering that we do for both the input and the output, we
think playbackTime is actually current time + 2 * buffersize.

[Raymond Toy, 2014-03-27 18:52:20]

https://codereview.chromium.org/210973002/diff/20001/LayoutTests/webaudio/scr...
File LayoutTests/webaudio/scriptprocessornode.html (right):

https://codereview.chromium.org/210973002/diff/20001/LayoutTests/webaudio/scr...
LayoutTests/webaudio/scriptprocessornode.html:46:
shouldBeCloseTo("playbackTime", expectedTime, precision, true);
Is there a reason playbackTime this is not EXACTLY equal expectedTime?

And this is not a particularly good test since it's just testing that you
computed what you thought you did.

Ideally, we would want to look at the real delay caused by the script processor
node and compare that with the value returned by playbackTime.  Unfortunately, I
don't know how to do this because ScriptProcessorNodes don't work so well in an
OfflineAudioContext.

[KhNo, 2014-03-28 03:02:06]

> Ok, I talked this over with Chris. We didn't discuss how users can use
> playbackTime; we mostly looked at what the correct value should be.  We
decided
> that playbackTime is the time we expect the current data would be expected to
> reach the destination node. 
> 
> Because of the double buffering that we do for both the input and the output,
we
> think playbackTime is actually current time + 2 * buffersize.

Well, in my investigation with temp debug log, playbackTime should be (current
frame + buffersize) / sampleRate.

As you know, we are using double buffering.
One of buffer set(input,output) works that audio thread is just collecting datas
to input[buffersize] from upstream node and output[buffersize] is being rendered
to downstream node.
On same time, another buffer set is being processed by JS's onauidoprocess() to
making processed buffer that will be used for rendering output to downstream
node.
When input[] buffer is full, audio thread fires onaudioprocess() then swap
buffer index. The head of output buffer that was delivered to JS will be
rendered on firing next onaudioprocess() that is exactly same period just
collecting datas to input[buffersize].

That is my opinion. Is there anything I missed from my investigation?

[KhNo, 2014-03-28 04:41:37]

@Raymond. PTAL and Please refer my comment.

https://codereview.chromium.org/210973002/diff/20001/LayoutTests/webaudio/scr...
File LayoutTests/webaudio/scriptprocessornode.html (right):

https://codereview.chromium.org/210973002/diff/20001/LayoutTests/webaudio/scr...
LayoutTests/webaudio/scriptprocessornode.html:46:
shouldBeCloseTo("playbackTime", expectedTime, precision, true);
On 2014/03/27 18:52:20, Raymond Toy wrote:
> Is there a reason playbackTime this is not EXACTLY equal expectedTime?
> 
> And this is not a particularly good test since it's just testing that you
> computed what you thought you did.
> 
> Ideally, we would want to look at the real delay caused by the script
processor
> node and compare that with the value returned by playbackTime.  Unfortunately,
I
> don't know how to do this because ScriptProcessorNodes don't work so well in
an
> OfflineAudioContext.

The reason that is not exactly same with expectedTime and playbackTime is from
two thread operation.

The delta gap is from diff context.currentTime when audio thread fire
onaudioprocess() and context.currentTime when JS thread read.

When JS thread try to read context.currentTime to check on first line of
eventhandler, a few frameToProcess has been passed to destination already, so it
has a little gap.

I tried to make test case. But that was the best I could except the naming
"precision". I think there is no perfect condition to check pass or fail on JS.
A "precision" should be changed to "gap" and be added some comment about this
gap.
Do you have any good naming or proper way I missed.

[Raymond Toy, 2014-03-28 18:01:42]

On 2014/03/28 03:02:06, KhNo wrote:
> > Ok, I talked this over with Chris. We didn't discuss how users can use
> > playbackTime; we mostly looked at what the correct value should be.  We
> decided
> > that playbackTime is the time we expect the current data would be expected
to
> > reach the destination node. 
> > 
> > Because of the double buffering that we do for both the input and the
output,
> we
> > think playbackTime is actually current time + 2 * buffersize.
> 
> Well, in my investigation with temp debug log, playbackTime should be (current
> frame + buffersize) / sampleRate.
> 
> As you know, we are using double buffering.
> One of buffer set(input,output) works that audio thread is just collecting
datas
> to input[buffersize] from upstream node and output[buffersize] is being
rendered
> to downstream node.
> On same time, another buffer set is being processed by JS's onauidoprocess()
to
> making processed buffer that will be used for rendering output to downstream
> node.
> When input[] buffer is full, audio thread fires onaudioprocess() then swap
> buffer index. The head of output buffer that was delivered to JS will be
> rendered on firing next onaudioprocess() that is exactly same period just
> collecting datas to input[buffersize].
> 
> That is my opinion. Is there anything I missed from my investigation?

Yes, you are right. I misunderstood when the input was copied.

[Raymond Toy, 2014-03-28 18:14:13]

https://codereview.chromium.org/210973002/diff/130001/LayoutTests/webaudio/sc...
File LayoutTests/webaudio/scriptprocessornode.html (right):

https://codereview.chromium.org/210973002/diff/130001/LayoutTests/webaudio/sc...
LayoutTests/webaudio/scriptprocessornode.html:48: // There may be a littl time
gap which is from different thread operation
Typo: littl -> little.

https://codereview.chromium.org/210973002/diff/130001/Source/modules/webaudio...
File Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/210973002/diff/130001/Source/modules/webaudio...
Source/modules/webaudio/ScriptProcessorNode.cpp:270: double playbackTime =
(context()->currentSampleFrame() + m_bufferSize) / context()->sampleRate();
Cast context()->sampleRate() to double because it's a float. Playback time will
be slightly off then.

[KhNo, 2014-03-29 00:24:46]

PTAL :) Thank you.

https://codereview.chromium.org/210973002/diff/130001/Source/modules/webaudio...
File Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/210973002/diff/130001/Source/modules/webaudio...
Source/modules/webaudio/ScriptProcessorNode.cpp:270: double playbackTime =
(context()->currentSampleFrame() + m_bufferSize) / context()->sampleRate();
On 2014/03/28 18:14:14, Raymond Toy wrote:
> Cast context()->sampleRate() to double because it's a float. Playback time
will
> be slightly off then.

I missed it. Thank you.

[Raymond Toy, 2014-03-31 16:40:53]

On 2014/03/29 00:24:46, KhNo wrote:
> PTAL :) Thank you.
> 
>
https://codereview.chromium.org/210973002/diff/130001/Source/modules/webaudio...
> File Source/modules/webaudio/ScriptProcessorNode.cpp (right):
> 
>
https://codereview.chromium.org/210973002/diff/130001/Source/modules/webaudio...
> Source/modules/webaudio/ScriptProcessorNode.cpp:270: double playbackTime =
> (context()->currentSampleFrame() + m_bufferSize) / context()->sampleRate();
> On 2014/03/28 18:14:14, Raymond Toy wrote:
> > Cast context()->sampleRate() to double because it's a float. Playback time
> will
> > be slightly off then.
> 
> I missed it. Thank you.

lgtm

[KhNo, 2014-03-31 17:15:04]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-03-31 17:15:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/210973002/190001

[commit-bot: I haz the power, 2014-03-31 18:27:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 18:27:15]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[KhNo, 2014-04-01 00:17:09]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-04-01 00:18:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/210973002/190001

[commit-bot: I haz the power, 2014-04-01 00:29:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:29:38]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[KhNo, 2014-04-01 02:13:57]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-04-01 02:14:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/210973002/190001

[commit-bot: I haz the power, 2014-04-01 02:58:50]

Change committed as 170505

[mickelsen.eric, 2014-10-07 02:05:41]

I think there may be an additional test needed here.  Not sure if it's possible.
 (Sorry, I'm just starting to familiarize myself with the implementation of
webaudio.)

https://codereview.chromium.org/210973002/diff/190001/LayoutTests/webaudio/sc...
File LayoutTests/webaudio/scriptprocessornode.html (right):

https://codereview.chromium.org/210973002/diff/190001/LayoutTests/webaudio/sc...
LayoutTests/webaudio/scriptprocessornode.html:51:
shouldBeCloseTo("playbackTime", expectedTime, allowedTimeGap, true);
I'm not sure how to track this down, but I have a strong suspicion that
different audio processing event handlers in the graph are receiving different
playbackTime values - closer to something like what time it is now, rather than
what the spec. requires, which is what time the audio will play, which causes
desynchronization.  I suspect this affects AudioContext and not
OfflineAudioContext.  Any idea how this might be testable?

[Raymond Toy, 2014-10-07 03:47:33]

On 2014/10/07 02:05:41, mickelsen.eric wrote:
> I think there may be an additional test needed here.  Not sure if it's
possible.
>  (Sorry, I'm just starting to familiarize myself with the implementation of
> webaudio.)
> 
>
https://codereview.chromium.org/210973002/diff/190001/LayoutTests/webaudio/sc...
> File LayoutTests/webaudio/scriptprocessornode.html (right):
> 
>
https://codereview.chromium.org/210973002/diff/190001/LayoutTests/webaudio/sc...
> LayoutTests/webaudio/scriptprocessornode.html:51:
> shouldBeCloseTo("playbackTime", expectedTime, allowedTimeGap, true);
> I'm not sure how to track this down, but I have a strong suspicion that
> different audio processing event handlers in the graph are receiving different
> playbackTime values - closer to something like what time it is now, rather
than
> what the spec. requires, which is what time the audio will play, which causes
> desynchronization.  I suspect this affects AudioContext and not
> OfflineAudioContext.  Any idea how this might be testable?

Rather than discuss this here on this closed CL, please open a new bug at
crbug.com/new. Be sure to cc me, and add the label cr-blink-webaudio.

Thanks!