Looping AudioBufferSourceNodes stop only if duration is explicitly given.

Looping AudioBufferSourceNodes stop only if duration is explicitly given.

An AudioBufferSourceNode that is looping should loop forever unless a
grain duration is explicitly given.  In particular, start(0, 0) should
still loop forever.

New test added, and updated one test in
audiobuffersource-loop-comprehensive to make it clear that the looping
source with duration stops at the right time.

Manual test added for handling the case where the AudioBufferSourceNode buffer is assigned after start() has been called. This is difficult to test properly using an offline context.


BUG=457099
TEST=audiobuffersource-loop-grain-no-duration.html

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

[Raymond Toy, 2015-02-12 18:20:24]

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

[Raymond Toy, 2015-02-12 18:20:25]

PTAL.

[hongchan, 2015-02-12 18:36:58]

LGTM with nits.

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-457099.html (right):

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-457099.html:11: <script>
I think it's better to include the URL to the crbug issue inside of HTML body or
as a JS comment, not the file name. WDYT?

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-457099.html:68: runTest();
I see this runTest() pattern a lot in our layout tests, but it seems to be a bit
unnecessary. Is there a reason for this? If this structure is needed, we can
replace this with IIFE pattern like (function() { /** task **/ })();

https://codereview.chromium.org/912803005/diff/20001/ManualTests/webaudio/aud...
File ManualTests/webaudio/audiobuffersource-loop-grain.html (right):

https://codereview.chromium.org/912803005/diff/20001/ManualTests/webaudio/aud...
ManualTests/webaudio/audiobuffersource-loop-grain.html:71: <address><a
href="mailto:rtoy@google.com">Raymond Toy</a></address>
What does this part do? Is this needed for the manual test?

https://codereview.chromium.org/912803005/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/912803005/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.h:101: void startSource(double
when, double grainOffset, double grainDuration, bool durationGiven,
ExceptionState&);
Can |durationGiven| be |isDurationGiven|?

https://codereview.chromium.org/912803005/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.h:138: bool m_explicitDuration;
Can this be |m_isDurationGiven|?

[Raymond Toy, 2015-02-12 21:36:43]

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-457099.html (right):

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-457099.html:11: <script>
On 2015/02/12 18:36:58, hoch wrote:
> I think it's better to include the URL to the crbug issue inside of HTML body
or
> as a JS comment, not the file name. WDYT?

Because I ran out of ideas for a good descriptive but reasonably short filename.

But certainly, the crbug should be listed in the file too.

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-457099.html:68: runTest();
On 2015/02/12 18:36:58, hoch wrote:
> I see this runTest() pattern a lot in our layout tests, but it seems to be a
bit
> unnecessary. Is there a reason for this? If this structure is needed, we can
> replace this with IIFE pattern like (function() { /** task **/ })();

I was mostly following Chris's existing pattern.

Plus, I often run these tests in a browser, so typing runTest() in the console
is sometimes convenient, especially if I modify some of the globals.

https://codereview.chromium.org/912803005/diff/20001/ManualTests/webaudio/aud...
File ManualTests/webaudio/audiobuffersource-loop-grain.html (right):

https://codereview.chromium.org/912803005/diff/20001/ManualTests/webaudio/aud...
ManualTests/webaudio/audiobuffersource-loop-grain.html:71: <address><a
href="mailto:rtoy@google.com">Raymond Toy</a></address>
On 2015/02/12 18:36:58, hoch wrote:
> What does this part do? Is this needed for the manual test?

Oops. My editor does that automatically.  I'll remove it.

[Raymond Toy, 2015-02-12 22:12:50]

New patchsets have been uploaded after l-g-t-m from hongchan@chromium.org

[Raymond Toy, 2015-02-13 20:07:06]

PTAL.

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-457099.html (right):

https://codereview.chromium.org/912803005/diff/20001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-457099.html:11: <script>
On 2015/02/12 18:36:58, hoch wrote:
> I think it's better to include the URL to the crbug issue inside of HTML body
or
> as a JS comment, not the file name. WDYT?

Renamed file.

https://codereview.chromium.org/912803005/diff/20001/ManualTests/webaudio/aud...
File ManualTests/webaudio/audiobuffersource-loop-grain.html (right):

https://codereview.chromium.org/912803005/diff/20001/ManualTests/webaudio/aud...
ManualTests/webaudio/audiobuffersource-loop-grain.html:71: <address><a
href="mailto:rtoy@google.com">Raymond Toy</a></address>
On 2015/02/12 18:36:58, hoch wrote:
> What does this part do? Is this needed for the manual test?

Removed.

https://codereview.chromium.org/912803005/diff/20001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/912803005/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.h:101: void startSource(double
when, double grainOffset, double grainDuration, bool durationGiven,
ExceptionState&);
On 2015/02/12 18:36:58, hoch wrote:
> Can |durationGiven| be |isDurationGiven|?

Done.

https://codereview.chromium.org/912803005/diff/20001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.h:138: bool m_explicitDuration;
On 2015/02/12 18:36:58, hoch wrote:
> Can this be |m_isDurationGiven|?

Done.

[Raymond Toy, 2015-02-17 19:13:38]

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

[Raymond Toy, 2015-02-17 19:13:38]

PTAL

[Ken Russell (switch to Gerrit), 2015-02-19 01:52:36]

LGTM based on the code and test. I read through the spec again but wasn't able
to trivially verify this is the behavior defined when calling
AudioBufferSourceNode.start(0, 0).

Please update the CL description; the new tests' names are wrong there.

Also, please triple-check that there are already tests where the duration is
greater than zero but smaller than the grain's length, for a looping source.

[Raymond Toy, 2015-02-19 19:25:32]

On 2015/02/19 01:52:36, Ken Russell wrote:
> LGTM based on the code and test. I read through the spec again but wasn't able
> to trivially verify this is the behavior defined when calling
> AudioBufferSourceNode.start(0, 0).

This is specified in http://webaudio.github.io/web-audio-api/#looping.

> 
> Please update the CL description; the new tests' names are wrong there.
> 
> Also, please triple-check that there are already tests where the duration is
> greater than zero but smaller than the grain's length, for a looping source.

[Raymond Toy, 2015-02-19 22:43:41]

On 2015/02/19 01:52:36, Ken Russell wrote:
> LGTM based on the code and test. I read through the spec again but wasn't able
> to trivially verify this is the behavior defined when calling
> AudioBufferSourceNode.start(0, 0).
> 
> Please update the CL description; the new tests' names are wrong there.
> 
> Also, please triple-check that there are already tests where the duration is
> greater than zero but smaller than the grain's length, for a looping source.

Excellent suggestion. There was no test where the duration was less than the
grain's length. I've added that test. Also found out that there are several
mistakes in the loop-comprehensive test implementation that will be fixed.

Uploading new patch soon.

[Raymond Toy, 2015-02-20 22:11:51]

New patchsets have been uploaded after l-g-t-m from kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-02-20 22:24:58]

Please ask hoch@ to carefully review the new test. Still LGTM overall but I
didn't study it in detail.

[hongchan, 2015-02-20 22:49:46]

LGTM with nits.

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html (right):

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:14: <div
id="console"></div>
These two <div>s should be removed.

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:149: // Start a
loop with a duration less than the grain length.  The output should be the data
from
By 'grain length', do you mean the playback duration?

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:169: expected:
[7, 3, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0] },
This is the last item in the array. The comma should be removed.

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:233:
context.startRendering();
Can we use |.then()| rather than |oncomplete|?

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html (right):

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:21: var
loopCount = 4;
Out of curiosity, why 4?

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:53: context =
new OfflineAudioContext(1, renderFrames, sampleRate);
Can you provide informative comments here? What does it generate and compare?

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:66:
.then(function () { finishJSTest(); });
Can't this be |.then(finishJSTest);| ?

[Raymond Toy, 2015-02-20 23:06:19]

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html (right):

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:149: // Start a
loop with a duration less than the grain length.  The output should be the data
from
On 2015/02/20 22:49:45, hoch wrote:
> By 'grain length', do you mean the playback duration?

No. It's the length of the looping region from loopStartFrame to loopEndFrame.

I'll rephrase.

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:233:
context.startRendering();
On 2015/02/20 22:49:45, hoch wrote:
> Can we use |.then()| rather than |oncomplete|?

That would require changing checkAllTests and all the tests (1) that use it.  I
think that's not relevant to this test.

And besides, we need to have some tests that still use oncomplete since that is
not going to be removed in favor of the promise. :-)

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html (right):

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:53: context =
new OfflineAudioContext(1, renderFrames, sampleRate);
On 2015/02/20 22:49:46, hoch wrote:
> Can you provide informative comments here? What does it generate and compare?

Is the description in lines 59-61 not enough? Or are you saying those comments
should be moved here to make it clearer what the test is doing?

[hongchan, 2015-02-20 23:15:44]

> And besides, we need to have some tests that still use oncomplete since
> that is not going to be removed in favor of the promise. :-)

I agree. But maybe we can at least update tests we're currently working on. :)

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
File LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html (right):

https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:53: context =
new OfflineAudioContext(1, renderFrames, sampleRate);
On 2015/02/20 at 23:06:18, Raymond Toy wrote:
> On 2015/02/20 22:49:46, hoch wrote:
> > Can you provide informative comments here? What does it generate and
compare?
> 
> Is the description in lines 59-61 not enough? Or are you saying those comments
should be moved here to make it clearer what the test is doing?

I thought more explicit comments about how/why two buffers are being compared
would be nice. Actually the comparison is happening above, not down below.

[Raymond Toy, 2015-02-20 23:20:52]

On 2015/02/20 23:15:44, hoch wrote:
> > And besides, we need to have some tests that still use oncomplete since
> > that is not going to be removed in favor of the promise. :-)
> 
> I agree. But maybe we can at least update tests we're currently working on. :)

Since this test is after the startRendering promise was added, then yes, I'll
update.
And all new tests should use the promise.

But I am opposed to making old tests using the promise, even if we're modifying
the old tests.

> 
>
https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
> File LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html
(right):
> 
>
https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
> LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:53: context
=
> new OfflineAudioContext(1, renderFrames, sampleRate);
> On 2015/02/20 at 23:06:18, Raymond Toy wrote:
> > On 2015/02/20 22:49:46, hoch wrote:
> > > Can you provide informative comments here? What does it generate and
> compare?
> > 
> > Is the description in lines 59-61 not enough? Or are you saying those
comments
> should be moved here to make it clearer what the test is doing?
> 
> I thought more explicit comments about how/why two buffers are being compared
> would be nice. Actually the comparison is happening above, not down below.

[Raymond Toy, 2015-02-20 23:22:16]

On 2015/02/20 23:20:52, Raymond Toy wrote:
> On 2015/02/20 23:15:44, hoch wrote:
> > > And besides, we need to have some tests that still use oncomplete since
> > > that is not going to be removed in favor of the promise. :-)
> > 
> > I agree. But maybe we can at least update tests we're currently working on.
:)
> 
> Since this test is after the startRendering promise was added, then yes, I'll
> update.
> And all new tests should use the promise.

Oops. This is an old test, so I'm not changing it.
> 
> But I am opposed to making old tests using the promise, even if we're
modifying
> the old tests.
> 
> > 
> >
>
https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
> > File LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html
> (right):
> > 
> >
>
https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
> > LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:53:
context
> =
> > new OfflineAudioContext(1, renderFrames, sampleRate);
> > On 2015/02/20 at 23:06:18, Raymond Toy wrote:
> > > On 2015/02/20 22:49:46, hoch wrote:
> > > > Can you provide informative comments here? What does it generate and
> > compare?
> > > 
> > > Is the description in lines 59-61 not enough? Or are you saying those
> comments
> > should be moved here to make it clearer what the test is doing?
> > 
> > I thought more explicit comments about how/why two buffers are being
compared
> > would be nice. Actually the comparison is happening above, not down below.

[hongchan, 2015-02-20 23:22:17]

On 2015/02/20 at 23:20:52, rtoy wrote:
> On 2015/02/20 23:15:44, hoch wrote:
> > > And besides, we need to have some tests that still use oncomplete since
> > > that is not going to be removed in favor of the promise. :-)
> > 
> > I agree. But maybe we can at least update tests we're currently working on.
:)
> 
> Since this test is after the startRendering promise was added, then yes, I'll
update.
> And all new tests should use the promise.
> 
> But I am opposed to making old tests using the promise, even if we're
modifying the old tests.
> 
> > 
> >
https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
> > File LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html
(right):
> > 
> >
https://codereview.chromium.org/912803005/diff/60001/LayoutTests/webaudio/aud...
> > LayoutTests/webaudio/audiobuffersource-loop-grain-no-duration.html:53:
context =
> > new OfflineAudioContext(1, renderFrames, sampleRate);
> > On 2015/02/20 at 23:06:18, Raymond Toy wrote:
> > > On 2015/02/20 22:49:46, hoch wrote:
> > > > Can you provide informative comments here? What does it generate and
> > compare?
> > > 
> > > Is the description in lines 59-61 not enough? Or are you saying those
comments
> > should be moved here to make it clearer what the test is doing?
> > 
> > I thought more explicit comments about how/why two buffers are being
compared
> > would be nice. Actually the comparison is happening above, not down below.

I completely agree.

[Raymond Toy, 2015-02-20 23:38:25]

New patchsets have been uploaded after l-g-t-m from
kbr@chromium.org,hongchan@chromium.org

[Raymond Toy, 2015-02-23 17:27:21]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2015-02-23 17:28:16]

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

[commit-bot: I haz the power, 2015-02-23 19:48:14]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190684