Fix issue with failing to call AudioBufferSource.onended in some cases

Fix issue with failing to call AudioBufferSource.onended in some cases.

AudioBufferSource.onended was not being called because we thought the
source should have already stopped but had not actually stopped.  This
happened when starting the source via start().  The current context
time was recorded as the start time of the source, but in some cases,
this was already too late, and the source would get started at the
next rendering quantum.

However, the stop time was computed using the recorded start time, so we
would expect the source to stop one rendering quantum sooner than it
actually would.  Because of this, we would stop the source but NOT fire
the onended event.

To fix this, change the check to add a little extra time before
stopping the source.  We don't need to be extra precise here; just
stop the source some time after it would have stopped.

This can't be tested using an offline context because start() always
starts at the correct time.

Run the manual test and watch the results (in the window) and the
console.log.  Both tones should be played if everything is working.

BUG=478301
TEST=ManualTests/webaudio/audiobuffersource-onended.html

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

[Raymond Toy, 2015-04-21 21:30:33]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2015-04-21 21:30:33]

PTAL

[hongchan, 2015-04-21 21:39:08]

LGTM with nits.

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:548: // start time and the
duration, so we end up stopping one quantum too early.  Thus, add one
I think 'one quantum early' reads better.

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:552: // FIXME: Expose the
rendering quantom somehow instead of hardwiring a value here.
quantom -> quantum

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:553: double extraStopTime =
256 / static_cast<double>(context()->sampleRate());
Can we use an extra variable for '256' at the top like what we do for the
smoothing constant and etc.?

[Raymond Toy, 2015-04-21 22:00:05]

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:548: // start time and the
duration, so we end up stopping one quantum too early.  Thus, add one
On 2015/04/21 21:39:08, hoch wrote:
> I think 'one quantum early' reads better.

Done.

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:552: // FIXME: Expose the
rendering quantom somehow instead of hardwiring a value here.
On 2015/04/21 21:39:08, hoch wrote:
> quantom -> quantum

Done.

https://codereview.chromium.org/1097373003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:553: double extraStopTime =
256 / static_cast<double>(context()->sampleRate());
On 2015/04/21 21:39:08, hoch wrote:
> Can we use an extra variable for '256' at the top like what we do for the
> smoothing constant and etc.?

Done.

[Raymond Toy, 2015-04-21 22:02:43]

rtoy@chromium.org changed reviewers:
+ kbr@chromium.org

[Raymond Toy, 2015-04-21 22:02:44]

kbr@, PTAL when you're back.

[Ken Russell (switch to Gerrit), 2015-04-22 19:01:50]

LGTM with a couple of things to think about.

https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
File ManualTests/webaudio/audiobuffersource-resampling-onended.html (right):

https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
ManualTests/webaudio/audiobuffersource-resampling-onended.html:36: var context =
new AudioContext();
Could you please invest a little more time thinking about how this might be
tested with an automated test? You can add methods to window.testRunner that
access private functionality. For example, you could add a backdoor that would
let you gather samples from a non-offline AudioContext.

https://codereview.chromium.org/1097373003/diff/80001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/1097373003/diff/80001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:558: double stopTime =
m_startTime + buffer()->duration() + extraStopTime;
This seems like a classic rounding error. Does the sound play in its entirety,
or are the first quantum's frames dropped when it starts playing? If the latter,
should the if-test below be changed rather than adding on this extraStopTime?

[Raymond Toy, 2015-04-22 20:19:46]

Good questions.  Does this answer your concerns?

https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
File ManualTests/webaudio/audiobuffersource-resampling-onended.html (right):

https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
ManualTests/webaudio/audiobuffersource-resampling-onended.html:36: var context =
new AudioContext();
On 2015/04/22 19:01:50, Ken Russell wrote:
> Could you please invest a little more time thinking about how this might be
> tested with an automated test? You can add methods to window.testRunner that
> access private functionality. For example, you could add a backdoor that would
> let you gather samples from a non-offline AudioContext.

This is fairly hard to test, even with an online context because it depends on
start() being called after the audio thread has started rendering a quantum so
that the m_startTime is actually one quantum behind the actual start of playing.
 With an offline context, start() always starts at a rendering quantum so this
issue doesn't happen.  We can try to do some kind of timeout to call start(),
but that would be flaky as well.

This manual test doesn't reliably fail either without this fix.  On my linux
box, about 1/3 of the time this test passes.  Hongchan says it passes most of
the time on his OSX machine.  But with the fix, I believe this test should never
fail.

https://codereview.chromium.org/1097373003/diff/80001/Source/modules/webaudio...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/1097373003/diff/80001/Source/modules/webaudio...
Source/modules/webaudio/AudioBufferSourceNode.cpp:558: double stopTime =
m_startTime + buffer()->duration() + extraStopTime;
On 2015/04/22 19:01:50, Ken Russell wrote:
> This seems like a classic rounding error. Does the sound play in its entirety,
> or are the first quantum's frames dropped when it starts playing? If the
latter,
> should the if-test below be changed rather than adding on this extraStopTime?

Without this fix, it's the last few frames for the last quantum that don't play
because we prematurely stopped it.

It's not clear but the purpose of this function is to handle the case where you
have a source node with a buffer, and you start it. This creates a reference. 
However, if it's never connected to the destination, it never gets pulled and
thus stays around forever.  But if we know when the source would end playing
(because we know the duration and it's not looping) and the context time has
passed the end time, we can stop the node, allowing it to be collected.  So we
don't have to be super-precise about when to stop it; we just want it to stop
eventually.

For the typical case where the node is connected, normal processing will stop
the node, so this code shouldn't be called.  The bug is that we didn't correctly
account for the fact that the m_startTime may actually be one quantum behind the
actual play start time so the endTime computation was off, prematurely stopping
the source without an event.

(I should probably add a comment on what handleStoppableSourceNode is really
meant to do.)

[hongchan, 2015-04-22 22:30:32]

https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
File ManualTests/webaudio/audiobuffersource-resampling-onended.html (right):

https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
ManualTests/webaudio/audiobuffersource-resampling-onended.html:36: var context =
new AudioContext();
On 2015/04/22 19:01:50, Ken Russell wrote:
> Could you please invest a little more time thinking about how this might be
> tested with an automated test? You can add methods to window.testRunner that
> access private functionality. For example, you could add a backdoor that would
> let you gather samples from a non-offline AudioContext.

With the testRunner, capturing the actual audio stream from the real-time
context is beyond my knowledge. I guess the main issue here is that 'onended'
event gets fired at the non-sample-accurate timing. That means we have to
capture the sample-accurate audio stream, expect the event to be fired at the
arbitrary moment, and then we need to arrange the rendered(captured) data for
the verification.

The timing of change will be consistently fluctuating (due to 'onended' timing),
so the verification needs a lot of care. We have such examples in our layout
tests already and those tests are very flaky. This manual test is the conclusion
we have reached to avoid that situation.

With that said, I agree with the idea that adding some verification framework
for web audio to the testRunner could be a long term solution.

[Raymond Toy, 2015-04-23 00:09:24]

On 2015/04/22 22:30:32, hoch wrote:
>
https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
> File ManualTests/webaudio/audiobuffersource-resampling-onended.html (right):
> 
>
https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
> ManualTests/webaudio/audiobuffersource-resampling-onended.html:36: var context
=
> new AudioContext();
> On 2015/04/22 19:01:50, Ken Russell wrote:
> > Could you please invest a little more time thinking about how this might be
> > tested with an automated test? You can add methods to window.testRunner that
> > access private functionality. For example, you could add a backdoor that
would
> > let you gather samples from a non-offline AudioContext.
> 
> With the testRunner, capturing the actual audio stream from the real-time
> context is beyond my knowledge. I guess the main issue here is that 'onended'
> event gets fired at the non-sample-accurate timing. That means we have to
> capture the sample-accurate audio stream, expect the event to be fired at the
> arbitrary moment, and then we need to arrange the rendered(captured) data for
> the verification.
> 
> The timing of change will be consistently fluctuating (due to 'onended'
timing),
> so the verification needs a lot of care. We have such examples in our layout
> tests already and those tests are very flaky. This manual test is the
conclusion
> we have reached to avoid that situation.
> 
> With that said, I agree with the idea that adding some verification framework
> for web audio to the testRunner could be a long term solution.

Just occurred to me that there's a different (additional) fix we might consider.

In start(t), we currently do m_startTime = max(t, context()->currentTime()). 
Maybe it would be better to do

max(t, 128/sampleRate + context()->currentTime()).  

If t is scheduled far enough in advance, there's no change.  Only if t is less
than or equal to the current time will there be a slight change.  We schedule it
to start at the next quantum, which is what would most likely happen anyway. 
Only if start(t) is run just before the quantum starts will there be a change. 
This should be ok.  There was never any guarantee on behavior if t was too close
to currentTime().

[Raymond Toy, 2015-04-24 17:31:18]

On 2015/04/23 00:09:24, Raymond Toy wrote:
> On 2015/04/22 22:30:32, hoch wrote:
> >
>
https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
> > File ManualTests/webaudio/audiobuffersource-resampling-onended.html (right):
> > 
> >
>
https://codereview.chromium.org/1097373003/diff/80001/ManualTests/webaudio/au...
> > ManualTests/webaudio/audiobuffersource-resampling-onended.html:36: var
context
> =
> > new AudioContext();
> > On 2015/04/22 19:01:50, Ken Russell wrote:
> > > Could you please invest a little more time thinking about how this might
be
> > > tested with an automated test? You can add methods to window.testRunner
that
> > > access private functionality. For example, you could add a backdoor that
> would
> > > let you gather samples from a non-offline AudioContext.
> > 
> > With the testRunner, capturing the actual audio stream from the real-time
> > context is beyond my knowledge. I guess the main issue here is that
'onended'
> > event gets fired at the non-sample-accurate timing. That means we have to
> > capture the sample-accurate audio stream, expect the event to be fired at
the
> > arbitrary moment, and then we need to arrange the rendered(captured) data
for
> > the verification.
> > 
> > The timing of change will be consistently fluctuating (due to 'onended'
> timing),
> > so the verification needs a lot of care. We have such examples in our layout
> > tests already and those tests are very flaky. This manual test is the
> conclusion
> > we have reached to avoid that situation.
> > 
> > With that said, I agree with the idea that adding some verification
framework
> > for web audio to the testRunner could be a long term solution.
> 
> Just occurred to me that there's a different (additional) fix we might
consider.
> 
> In start(t), we currently do m_startTime = max(t, context()->currentTime()). 
> Maybe it would be better to do
> 
> max(t, 128/sampleRate + context()->currentTime()).  
> 
> If t is scheduled far enough in advance, there's no change.  Only if t is less
> than or equal to the current time will there be a slight change.  We schedule
it
> to start at the next quantum, which is what would most likely happen anyway. 
> Only if start(t) is run just before the quantum starts will there be a change.

> This should be ok.  There was never any guarantee on behavior if t was too
close
> to currentTime().

I'm not going to do this; it requires special casing for offline contexts.

[Raymond Toy, 2015-04-24 17:32:16]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2015-04-24 17:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1097373003/#ps100001
(title: "Rebase and clarify some comments.")

[commit-bot: I haz the power, 2015-04-24 17:33:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097373003/100001

[commit-bot: I haz the power, 2015-04-24 18:45:31]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194413