Bug 138739 – start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no args

• Component: Web Audio
• Status: RESOLVED FIXED

[Philippe Normand]

Port Blink patch: https://codereview.chromium.org/26913005

[Philippe Normand]

Created attachment 241579 [details]
patch

[Philippe Normand]

Eric, Jer, can you please review this patch?

[Eric Carlson]

Comment on attachment 241579 [details]
patch

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

The Blink patch includes changes to a test as well. I see we don't have dom-exceptions.html at all, but could you create it with just the start() tests?

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:375
> +    startPlaying(false, 0, 0, buffer()->duration(), ec);

Looking a startPlaying call site I have no idea what the first parameter means. I think an enum would be much better.

[Philippe Normand]

Created attachment 241703 [details]
patch

Eric, I'm asking another review because I'm not sure I've chosen the
best name for the added enum. Please let me know what you think or if
you have better suggestions :)

[Philippe Normand]

Created attachment 241704 [details]
patch

Cleaned up the test a bit.

[Darin Adler]

Comment on attachment 241704 [details]
patch

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

Not sure this code has enough test coverage.

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:375
> +    startPlaying(BufferPlaybackMode::Entire, 0, 0, buffer() ? buffer()->duration() : 0, ec);

The syntax here doesn’t seem quite right. Since BufferPlaybackMode is just an enum, not an enum class, we don’t need the enum name qualifier. Maybe it was intended to be an enum class?

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:428
> +        grainOffset = std::max(0.0, grainOffset);

This std::max is not needed since we check above for grainOffset < 0.

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:430
> +        m_grainOffset = grainOffset;

This should just be written in a single line:

    m_grainOffset = std::min(bufferDuration, grainOffset);

It’s also slightly clearer not to modify the passed in arguments.

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:433
> +        grainDuration = std::max(0.0, grainDuration);

This std::max is not needed since we check above for grainDuration < 0.

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:435
> +        m_grainDuration = grainDuration;

This should just be written in a single line:

    m_grainDuration = std::min(bufferDuration - m_grainOffset, grainDuration);

It’s also slightly clearer not to modify the passed in arguments.

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:457
> +    // Handle unspecified duration where 0 means the rest of the buffer.
> +    if (!grainDuration)
> +        grainDuration = buffer()->duration();

I don’t understand the mention of “unspecified” here and “0”. Where is the code that turns unspecified into 0? Or is 0 a magic value?

> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:184
>      when = std::max<double>(0, when);

This std::max is not needed now that we check "when < 0" above, and should be removed.

[Philippe Normand]

(In reply to comment #6)
> Comment on attachment 241704 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241704&action=review
> 
> Not sure this code has enough test coverage.
> 

I think we need a major update of our WebAudio implementation in general. We've fallen quite behind the spec :(

> > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:375
> > +    startPlaying(BufferPlaybackMode::Entire, 0, 0, buffer() ? buffer()->duration() : 0, ec);
> 
> The syntax here doesn’t seem quite right. Since BufferPlaybackMode is just
> an enum, not an enum class, we don’t need the enum name qualifier. Maybe it
> was intended to be an enum class?
> 

I somehow got the habit of using enums in C++ as enum classes but yeah I can drop the enum qualifier.

> > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:457
> > +    // Handle unspecified duration where 0 means the rest of the buffer.
> > +    if (!grainDuration)
> > +        grainDuration = buffer()->duration();
> 
> I don’t understand the mention of “unspecified” here and “0”. Where is the
> code that turns unspecified into 0? Or is 0 a magic value?
> 

A 0-duration means the whole buffer should be played, if I understand the spec correctly.

Thanks for the review Darin, I'll upload the patch-for-landing so EWS can check it.

[Philippe Normand]

Created attachment 241793 [details]
patch for landing

[Philippe Normand]

Comment on attachment 241793 [details]
patch for landing

Clearing flags on attachment: 241793

Committed r176311: <http://trac.webkit.org/changeset/176311>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.