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
Attachments
patch (10.16 KB, patch)
2014-11-14 04:43 PST, Philippe Normand
eric.carlson: review+
patch (19.71 KB, patch)
2014-11-17 05:31 PST, Philippe Normand
no flags
patch (19.60 KB, patch)
2014-11-17 05:34 PST, Philippe Normand
darin: review+
patch for landing (12.66 KB, patch)
2014-11-18 10:09 PST, Philippe Normand
no flags
Philippe Normand
Comment 1 2014-11-14 04:43:23 PST
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.