Bug 138739 - start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no args
Summary: start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-14 04:38 PST by Philippe Normand
Modified: 2014-11-18 23:49 PST (History)
4 users (show)

See Also:


Attachments
patch (10.16 KB, patch)
2014-11-14 04:43 PST, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff
patch (19.71 KB, patch)
2014-11-17 05:31 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (19.60 KB, patch)
2014-11-17 05:34 PST, Philippe Normand
darin: review+
Details | Formatted Diff | Diff
patch for landing (12.66 KB, patch)
2014-11-18 10:09 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2014-11-14 04:38:54 PST
Port Blink patch: https://codereview.chromium.org/26913005
Comment 1 Philippe Normand 2014-11-14 04:43:23 PST
Created attachment 241579 [details]
patch
Comment 2 Philippe Normand 2014-11-14 08:09:02 PST
Eric, Jer, can you please review this patch?
Comment 3 Eric Carlson 2014-11-14 09:03:07 PST
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.
Comment 4 Philippe Normand 2014-11-17 05:31:18 PST
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 :)
Comment 5 Philippe Normand 2014-11-17 05:34:04 PST
Created attachment 241704 [details]
patch

Cleaned up the test a bit.
Comment 6 Darin Adler 2014-11-18 09:16:38 PST
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.
Comment 7 Philippe Normand 2014-11-18 09:40:12 PST
(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.
Comment 8 Philippe Normand 2014-11-18 10:09:45 PST
Created attachment 241793 [details]
patch for landing
Comment 9 Philippe Normand 2014-11-18 23:49:24 PST
Comment on attachment 241793 [details]
patch for landing

Clearing flags on attachment: 241793

Committed r176311: <http://trac.webkit.org/changeset/176311>
Comment 10 Philippe Normand 2014-11-18 23:49:32 PST
All reviewed patches have been landed.  Closing bug.