WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
138739
start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no args
https://bugs.webkit.org/show_bug.cgi?id=138739
Summary
start/stop method for AudioBufferSourceNodes and OscillatorNodes can take no ...
Philippe Normand
Reported
2014-11-14 04:38:54 PST
Port Blink patch:
https://codereview.chromium.org/26913005
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Philippe Normand
Comment 1
2014-11-14 04:43:23 PST
Created
attachment 241579
[details]
patch
Philippe Normand
Comment 2
2014-11-14 08:09:02 PST
Eric, Jer, can you please review this patch?
Eric Carlson
Comment 3
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.
Philippe Normand
Comment 4
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 :)
Philippe Normand
Comment 5
2014-11-17 05:34:04 PST
Created
attachment 241704
[details]
patch Cleaned up the test a bit.
Darin Adler
Comment 6
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.
Philippe Normand
Comment 7
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.
Philippe Normand
Comment 8
2014-11-18 10:09:45 PST
Created
attachment 241793
[details]
patch for landing
Philippe Normand
Comment 9
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
>
Philippe Normand
Comment 10
2014-11-18 23:49:32 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug