Bug 100170 – Implement AudioBufferSourceNode .loopStart and .loopEnd attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Implement AudioBufferSourceNode .loopStart and .loopEnd attributes

[Chris Rogers]

Created attachment 170261 [details]
Patch

[Kenneth Russell]

Comment on attachment 170261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170261&action=review

One question and one comment. Not touching the r? bit yet.

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:227
> +        double loopStartFrame = m_loopStart * buffer()->sampleRate();

Does m_virtualReadIndex need to be clamped to m_loopStart? How does this code avoid sampling the buffer before m_loopStart?

> LayoutTests/webaudio/audiobuffersource-loop-points.html:68
> +    source.loopEnd = buffer.duration;

I think it would be a good idea to add two more tests: (1) loopStart is at the beginning of the buffer but loopEnd is before the end of the buffer; and (2) both loopStart and loopEnd are somewhere inside the buffer. Negative tests might be a good idea too.

[Chris Rogers]

Created attachment 171048 [details]
Patch

[Chris Rogers]

(In reply to comment #2)
> (From update of attachment 170261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170261&action=review
> 
> One question and one comment. Not touching the r? bit yet.
> 
> > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:227
> > +        double loopStartFrame = m_loopStart * buffer()->sampleRate();
> 
> Does m_virtualReadIndex need to be clamped to m_loopStart? How does this code avoid sampling the buffer before m_loopStart?

The idea is that playback normally starts before the loop, and that the loop-points are somewhere in the middle of the sample data, and are usually well-chosen to blend seamlessly at the loop transition (sample editors have tools to create these loop points).

> 
> > LayoutTests/webaudio/audiobuffersource-loop-points.html:68
> > +    source.loopEnd = buffer.duration;
> 
> I think it would be a good idea to add two more tests: (1) loopStart is at the beginning of the buffer but loopEnd is before the end of the buffer; and (2) both loopStart and loopEnd are somewhere inside the buffer. Negative tests might be a good idea too.

In the latest patch, I've added a bunch of new tests including the ones you suggest in: webaudio/audiobuffersource-loop-comprehensive.html

Thanks for having a look!

[Kenneth Russell]

Comment on attachment 171048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171048&action=review

Very nice. r=me

> LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:112
> +        if (expected[j] != renderedData[offsetFrame + j]) {

No epsilon needed in these comparisons?

[Chris Rogers]

(In reply to comment #5)
> (From update of attachment 171048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171048&action=review
> 
> Very nice. r=me
> 
> > LayoutTests/webaudio/audiobuffersource-loop-comprehensive.html:112
> > +        if (expected[j] != renderedData[offsetFrame + j]) {
> 
> No epsilon needed in these comparisons?

Doesn't seem to be necessary.

[WebKit Review Bot]

Comment on attachment 171048 [details]
Patch

Clearing flags on attachment: 171048

Committed r132720: <http://trac.webkit.org/changeset/132720>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.