RESOLVED FIXED 76659
Round time to sample frame
https://bugs.webkit.org/show_bug.cgi?id=76659
Summary Round time to sample frame
Raymond Toy
Reported 2012-01-19 13:03:48 PST
Instead of truncating the time to the nearest sample frame, we should round to the nearest frame. This is a possible solution to Chromium bug http://code.google.com/p/chromium/issues/detail?id=110365 and is a follow-on to Webkit bug https://bugs.webkit.org/show_bug.cgi?id=76073. Converting to doubles for noteOn is not enough on Windows.
Attachments
Patch (156.50 KB, patch)
2012-01-21 10:12 PST, Raymond Toy
no flags
Patch (193.62 KB, patch)
2012-01-26 15:20 PST, Raymond Toy
no flags
Patch (194.29 KB, patch)
2012-01-26 21:05 PST, Raymond Toy
no flags
Patch (194.33 KB, patch)
2012-01-26 21:27 PST, Raymond Toy
no flags
Patch (195.70 KB, patch)
2012-01-27 14:10 PST, Raymond Toy
no flags
Raymond Toy
Comment 1 2012-01-21 10:12:10 PST
Raymond Toy
Comment 2 2012-01-21 10:15:43 PST
In addition to enabling the offset test in the equalpower panner test (which passes on all three platforms now), I tested this with the upcoming distance model test. Those tests pass too on all three platforms. The updated sample-accurate-playback test also passes on all three platforms.
Chris Rogers
Comment 3 2012-01-25 15:58:29 PST
Comment on attachment 123451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123451&action=review > Source/WebCore/ChangeLog:9 > + like on Mac and Linux.) Please make more generic (talking about Visual Studio and gcc instead of Windows, Mac, Linux) > Source/WebCore/platform/audio/AudioUtilities.cpp:60 > +// When compiling on Windows with MSVC without SSE/SSE2, we want very please delete "on Windows" "without SSE/SSE2 -> with x87 using 80-bit floats" > Source/WebCore/platform/audio/AudioUtilities.cpp:61 > +// precise control over the arithmetic so that rounding is done as we so that rounding is done according to IEEE 754 single and double-precision floats > Source/WebCore/platform/audio/AudioUtilities.cpp:63 > +// using the x87 registers with 80-bit floats. We want each operation we can now delete "This is important since on Windows we are using the x87 registers with 80-bit floats." > Source/WebCore/platform/audio/AudioUtilities.cpp:75 > +unsigned timeToSampleFrame(double time, float sampleRate) unsigned -> size_t to be consistent > Source/WebCore/platform/audio/AudioUtilities.cpp:78 > + // Windows compiler to round each operation to double precision Maybe could be worded: DO NOT CONSOLIDATE THESE INSTRUCTIONS INTO A SINGLE LINE > Source/WebCore/platform/audio/AudioUtilities.cpp:83 > + return static_cast<unsigned>(r); unsigned -> size_t to be consistent > Source/WebCore/platform/audio/AudioUtilities.cpp:88 > + // DO NOT CHANGE OR DELETE THIS ASSIGNMENT! It forces the Windows Maybe could be worded: DO NOT CONSOLIDATE THESE INSTRUCTIONS INTO A SINGLE LINE
Raymond Toy
Comment 4 2012-01-26 15:20:52 PST
Raymond Toy
Comment 5 2012-01-26 15:26:18 PST
As discussed with Chris (in private), rather than make one small change, we decided to take a bigger step that can drastically reduce the round-off issues. We no longer keep track of time in seconds (using a floating-point value), but keep track using samples. Code is updated appropriately for that change, and we also change AudioBufferSourceNode to do as much arithmetic with integers as possible, only converting from floating point to integer when absolutely required by the Javascript API, which is not changed. I think I've addressed the review comments in the first patch.
Raymond Toy
Comment 6 2012-01-26 15:28:17 PST
Oh, I tested this on Linux, Mac, and Windows, and all of the tests pass, especially the new note-grain-on-timing test. One issue: on Windows, the audiobuffersource-playbackrate test is expected to fail but passes now, which is correct.
Chris Rogers
Comment 7 2012-01-26 16:31:57 PST
Comment on attachment 124187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review Looks great overall - definitely the right direction for better timing and rounding. > Source/WebCore/platform/audio/AudioUtilities.cpp:65 > +// to 80 bits automatically. "specified arithmetic precision and rounding" -> "specified arithmetic precision and rounding consistent with gcc" > Source/WebCore/platform/audio/AudioUtilities.cpp:68 > +// it for these functions here. (Using fp:strict everywhere can have "these functions" -> "this function" > Source/WebCore/platform/audio/AudioUtilities.cpp:79 > + // Visual Studio to round each operation to double precision "It forces the Visual Studio to round each operation to double precision" -> "When compiling with Visual Studio, it forces the rounding of each operation according to IEEE 754" > Source/WebCore/webaudio/AudioBufferSourceNode.cpp:109 > + size_t endSample = m_endTime == UnknownTime ? 0 : AudioUtilities::timeToSampleFrame(m_endTime, sampleRate); nit: it's better to explicitly use the phrase "sampleFrame" instead of "sample" since we're talking about units of time. quantumStartSample -> quantumStartSampleFrame quantumEndSample -> quantumEndSampleFrame startSample -> startSampleFrame endSample -> endSampleFrame It seems ok also to simply use "frame" instead of "sampleFrame" similar to how we name the variable "quantumFrameOffset" below Please check elsewhere in this patch for similar uses of unadorned "sample" in variable names and comments > Source/WebCore/webaudio/AudioBufferSourceNode.cpp:243 > + // TODO(rtoy): Should the endframe be computed as here or should it be computed WebKit style nit: "TODO(rtoy):" -> "FIXME:" > Source/WebCore/webaudio/AudioBufferSourceNode.cpp:244 > + // as timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate)? Actually, timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate) seems better. Does this work with the layout tests? > Source/WebCore/webaudio/AudioContext.h:96 > + size_t currentSample() { return m_destinationNode->currentSample(); } naming nit: "currentSample" -> "currentSampleFrame" > Source/WebCore/webaudio/AudioDestinationNode.cpp:42 > + , m_currentSample(0) "m_currentSample" -> "m_currentSampleFrame" > Source/WebCore/webaudio/AudioDestinationNode.cpp:86 > + // Advance current sample. sample frame > Source/WebCore/webaudio/AudioDestinationNode.h:49 > + size_t currentSample() { return m_currentSample; } currentSampleFrame > Source/WebCore/webaudio/AudioDestinationNode.h:59 > + // The current time, in sample frames. I wouldn't use the word "time" here, but say: // Counts the number of sample-frames processed by the destination. > LayoutTests/webaudio/note-grain-on-timing-expected.txt:8 > +PASS NoteGrainOn timing tests passed. nit: "NoteGrainOn" -> "noteGrainOn" > LayoutTests/webaudio/note-grain-on-timing.html:20 > + var extraFramesHRTF = 512; Yuck - we should fix this soon :) Could you please write up a bug? > LayoutTests/webaudio/note-grain-on-timing.html:37 > + var square; how about calling "square" -> "squareBuffer" > LayoutTests/webaudio/note-grain-on-timing.html:40 > + function createNote(context) { "createNote" doesn't seem like a great name -- how about "createSquarePulse"? > LayoutTests/webaudio/note-grain-on-timing.html:41 > + // Create a square pulse. Can we say "Create a one-second square pulse" > LayoutTests/webaudio/note-grain-on-timing.html:67 > + if (renderedData[k] > 0) { It seems like we're detecting "any" non-zero value. Can we fail if the value is not exactly 1 (the value in the square-pulse buffer) > LayoutTests/webaudio/note-grain-on-timing.html:112 > + if (errorCountStart == 0) { nit: WebKit style is if (!errorCountStart) here and below > LayoutTests/webaudio/note-grain-on-timing.html:120 > + testFailed(errorCountEnd + " out of " + numberOfTests + " square pulses ended at the wrong time."); isn't this error message wrong? Should be "errorCountStart" and "started at the wrong time." > LayoutTests/webaudio/note-grain-on-timing.html:150 > + bufferSource.noteGrainOn(time, 0, duration); nit: don't need to fix now, but definitely should add a "FIXME:" comment here We're not really exercising noteGrainOn() very fully here because we're always passing 0 in for grainOffset. A more comprehensive test might create a linear-ramp buffer instead of a square-pulse. Then test various noteGrainOn() calls with differing grainOffsets, and validate that the appropriate sub-section of the buffer is correctly rendered. > LayoutTests/webaudio/resources/audio-testing.js:123 > +// This should be the same as AudioUtilities::timeToSampleFrame. I would remove this comment. These layout tests shouldn't reference a specific implementation, C++ methods, etc. Ideally we should be testing the specification, where we can define this rounding behavior.
Chris Rogers
Comment 8 2012-01-26 16:35:54 PST
(In reply to comment #6) > Oh, I tested this on Linux, Mac, and Windows, and all of the tests pass, especially the new note-grain-on-timing test. One issue: on Windows, the audiobuffersource-playbackrate test is expected to fail but passes now, which is correct. Ok, we can update the test expectations file after this lands...
Raymond Toy
Comment 9 2012-01-26 16:51:19 PST
Comment on attachment 124187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review >> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:244 >> + // as timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate)? > > Actually, timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate) seems better. > > Does this work with the layout tests? Could possibly cause some layout tests to fail. Will test soon. The current note-grain-on test may fail because it currently computes the expected end point the other way. >> LayoutTests/webaudio/note-grain-on-timing.html:20 >> + var extraFramesHRTF = 512; > > Yuck - we should fix this soon :) > > Could you please write up a bug? Yes, that was on my list of things to do. (I was certainly surprised to find every end point test failing because I didn't expect the extra 512 samples.) >> LayoutTests/webaudio/note-grain-on-timing.html:67 >> + if (renderedData[k] > 0) { > > It seems like we're detecting "any" non-zero value. Can we fail if the value is not exactly 1 (the value in the square-pulse buffer) I suppose it's possible. But this test is a timing test, not a general test of noteGrainOn. There should be more tests for that. >> LayoutTests/webaudio/note-grain-on-timing.html:150 >> + bufferSource.noteGrainOn(time, 0, duration); > > nit: don't need to fix now, but definitely should add a "FIXME:" comment here > > We're not really exercising noteGrainOn() very fully here because we're always passing 0 in for grainOffset. > A more comprehensive test might create a linear-ramp buffer instead of a square-pulse. > Then test various noteGrainOn() calls with differing grainOffsets, and validate that the appropriate sub-section of the buffer is correctly rendered. That's why the test is called note-grain-on-timing. I certainly agree that we need more tests for note-grain-on, but I'd like to keep this test focussed on just this timing aspect. Can we write a bug saying we need more tests for noteGrainOn and perhaps update the test then? Or add a new test, as appropriate. >> LayoutTests/webaudio/resources/audio-testing.js:123 >> +// This should be the same as AudioUtilities::timeToSampleFrame. > > I would remove this comment. These layout tests shouldn't reference a specific implementation, C++ methods, etc. Ideally we should be testing the specification, > where we can define this rounding behavior. Is there a specification for this? This certainly needs to be consistent, otherwise the timing tests will be wrong because the Javascript reference is wrong.
Chris Rogers
Comment 10 2012-01-26 17:05:25 PST
(In reply to comment #9) > (From update of attachment 124187 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review > > >> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:244 > >> + // as timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate)? > > > > Actually, timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate) seems better. > > > > Does this work with the layout tests? > > Could possibly cause some layout tests to fail. Will test soon. The current note-grain-on test may fail because it currently computes the expected end point the other way. Ok, then just make sure to change the TODO -> FIXME > > >> LayoutTests/webaudio/note-grain-on-timing.html:20 > >> + var extraFramesHRTF = 512; > > > > Yuck - we should fix this soon :) > > > > Could you please write up a bug? > > Yes, that was on my list of things to do. (I was certainly surprised to find every end point test failing because I didn't expect the extra 512 samples.) Sounds good - seems like a good one to fix pretty soon. Sorry about that... > > >> LayoutTests/webaudio/note-grain-on-timing.html:67 > >> + if (renderedData[k] > 0) { > > > > It seems like we're detecting "any" non-zero value. Can we fail if the value is not exactly 1 (the value in the square-pulse buffer) > > I suppose it's possible. But this test is a timing test, not a general test of noteGrainOn. There should be more tests for that. Ok, maybe a short comment then describing that. > > >> LayoutTests/webaudio/note-grain-on-timing.html:150 > >> + bufferSource.noteGrainOn(time, 0, duration); > > > > nit: don't need to fix now, but definitely should add a "FIXME:" comment here > > > > We're not really exercising noteGrainOn() very fully here because we're always passing 0 in for grainOffset. > > A more comprehensive test might create a linear-ramp buffer instead of a square-pulse. > > Then test various noteGrainOn() calls with differing grainOffsets, and validate that the appropriate sub-section of the buffer is correctly rendered. > > That's why the test is called note-grain-on-timing. I certainly agree that we need more tests for note-grain-on, but I'd like to keep this test focussed on just this timing aspect. Can we write a bug saying we need more tests for noteGrainOn and perhaps update the test then? Or add a new test, as appropriate. Ok, that makes sense. > > >> LayoutTests/webaudio/resources/audio-testing.js:123 > >> +// This should be the same as AudioUtilities::timeToSampleFrame. > > > > I would remove this comment. These layout tests shouldn't reference a specific implementation, C++ methods, etc. Ideally we should be testing the specification, > > where we can define this rounding behavior. > > Is there a specification for this? This certainly needs to be consistent, otherwise the timing tests will be wrong because the Javascript reference is wrong. It's not written down in the specification yet (but should be). In some sense, these layout tests "are" providing some details of the specification which haven't yet been recorded. The layout tests are expected to be a test suite which any browser implementation could be tested by. So this method in the .js file *is* the definition of how timing rounding should be handled. If there's a comment anywhere, it should be a comment in AudioUtilities::timeToSampleFrame() which refers to the corresponding function in the layout test file. So the reference goes the other way. The C++ implementation file points to the layout test and says it needs to round in this fashion. Does that make sense?
Raymond Toy
Comment 11 2012-01-26 21:03:29 PST
Comment on attachment 124187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review >>>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:244 >>>> + // as timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate)? >>> >>> Actually, timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate) seems better. >>> >>> Does this work with the layout tests? >> >> Could possibly cause some layout tests to fail. Will test soon. The current note-grain-on test may fail because it currently computes the expected end point the other way. > > Ok, then just make sure to change the TODO -> FIXME Here is an example where the answers are different. Let bufferSampleRate = 1 for simplicity. Let m_grainOffset = m_grainDuration = 0.4. Then startFrame = 0. With current method, endFrame = 0. With the new method, endFrame = timeToSampleFrame(0.8, 1) = 1. I like this result better.
Raymond Toy
Comment 12 2012-01-26 21:05:56 PST
Raymond Toy
Comment 13 2012-01-26 21:17:57 PST
Comment on attachment 124187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review >> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:109 >> + size_t endSample = m_endTime == UnknownTime ? 0 : AudioUtilities::timeToSampleFrame(m_endTime, sampleRate); > > nit: it's better to explicitly use the phrase "sampleFrame" instead of "sample" since we're talking about units of time. > > quantumStartSample -> quantumStartSampleFrame > quantumEndSample -> quantumEndSampleFrame > startSample -> startSampleFrame > endSample -> endSampleFrame > > It seems ok also to simply use "frame" instead of "sampleFrame" similar to how we name the variable "quantumFrameOffset" below > > Please check elsewhere in this patch for similar uses of unadorned "sample" in variable names and comments Done. >>>>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:244 >>>>> + // as timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate)? >>>> >>>> Actually, timeToSampleFrame(m_grainOffset + m_grainDuration, bufferSampleRate) seems better. >>>> >>>> Does this work with the layout tests? >>> >>> Could possibly cause some layout tests to fail. Will test soon. The current note-grain-on test may fail because it currently computes the expected end point the other way. >> >> Ok, then just make sure to change the TODO -> FIXME > > Here is an example where the answers are different. Let bufferSampleRate = 1 for simplicity. Let m_grainOffset = m_grainDuration = 0.4. Then startFrame = 0. With current method, endFrame = 0. With the new method, endFrame = timeToSampleFrame(0.8, 1) = 1. I like this result better. Updated to use new method. I left a comment in there, but perhaps it should be removed? >> Source/WebCore/webaudio/AudioContext.h:96 >> + size_t currentSample() { return m_destinationNode->currentSample(); } > > naming nit: "currentSample" -> "currentSampleFrame" Done. >> Source/WebCore/webaudio/AudioDestinationNode.cpp:42 >> + , m_currentSample(0) > > "m_currentSample" -> "m_currentSampleFrame" Done. >> Source/WebCore/webaudio/AudioDestinationNode.cpp:86 >> + // Advance current sample. > > sample frame Done. >> Source/WebCore/webaudio/AudioDestinationNode.h:49 >> + size_t currentSample() { return m_currentSample; } > > currentSampleFrame Done. >> Source/WebCore/webaudio/AudioDestinationNode.h:59 >> + // The current time, in sample frames. > > I wouldn't use the word "time" here, but say: > > // Counts the number of sample-frames processed by the destination. Done. >> LayoutTests/webaudio/note-grain-on-timing-expected.txt:8 >> +PASS NoteGrainOn timing tests passed. > > nit: "NoteGrainOn" -> "noteGrainOn" Fixed. >>>> LayoutTests/webaudio/note-grain-on-timing.html:20 >>>> + var extraFramesHRTF = 512; >>> >>> Yuck - we should fix this soon :) >>> >>> Could you please write up a bug? >> >> Yes, that was on my list of things to do. (I was certainly surprised to find every end point test failing because I didn't expect the extra 512 samples.) > > Sounds good - seems like a good one to fix pretty soon. Sorry about that... Now problem. >> LayoutTests/webaudio/note-grain-on-timing.html:37 >> + var square; > > how about calling "square" -> "squareBuffer" Changed to squarePulseBuffer. >> LayoutTests/webaudio/note-grain-on-timing.html:41 >> + // Create a square pulse. > > Can we say "Create a one-second square pulse" Done. >>>> LayoutTests/webaudio/note-grain-on-timing.html:67 >>>> + if (renderedData[k] > 0) { >>> >>> It seems like we're detecting "any" non-zero value. Can we fail if the value is not exactly 1 (the value in the square-pulse buffer) >> >> I suppose it's possible. But this test is a timing test, not a general test of noteGrainOn. There should be more tests for that. > > Ok, maybe a short comment then describing that. Done. >> LayoutTests/webaudio/note-grain-on-timing.html:112 >> + if (errorCountStart == 0) { > > nit: WebKit style is > > if (!errorCountStart) > > here and below Fixed. >> LayoutTests/webaudio/note-grain-on-timing.html:120 >> + testFailed(errorCountEnd + " out of " + numberOfTests + " square pulses ended at the wrong time."); > > isn't this error message wrong? > > Should be "errorCountStart" and "started at the wrong time." Fixed. (Too much copy/paste.) >>>> LayoutTests/webaudio/resources/audio-testing.js:123 >>>> +// This should be the same as AudioUtilities::timeToSampleFrame. >>> >>> I would remove this comment. These layout tests shouldn't reference a specific implementation, C++ methods, etc. Ideally we should be testing the specification, >>> where we can define this rounding behavior. >> >> Is there a specification for this? This certainly needs to be consistent, otherwise the timing tests will be wrong because the Javascript reference is wrong. > > It's not written down in the specification yet (but should be). In some sense, these layout tests "are" providing some details of the specification which haven't yet been recorded. The layout tests are expected to be a test suite which any browser implementation could be tested by. So this method in the .js file *is* the definition of how timing rounding should be handled. If there's a comment anywhere, it should be a comment in AudioUtilities::timeToSampleFrame() which refers to the corresponding function in the layout test file. So the reference goes the other way. The C++ implementation file points to the layout test and says it needs to round in this fashion. Does that make sense? Yes, that makes sense. (Well, except that I don't know what guarantees Javascript makes about floating point operations. That could be a problem if we're using that as the specification.) I've removed the comment.
Raymond Toy
Comment 14 2012-01-26 21:27:10 PST
Raymond Toy
Comment 15 2012-01-26 21:34:21 PST
All review comments should be covered now. Everything still passes on OSX. Still need to test on Linux and Windows.
Chris Rogers
Comment 16 2012-01-27 10:48:04 PST
Comment on attachment 124253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124253&action=review Looks great overall. I added a few nit comments: > Source/WebCore/platform/audio/AudioUtilities.cpp:79 > + // with Visual Studio, It forces the rounding of each operation nit: "It" -> "it" > Source/WebCore/platform/audio/AudioUtilities.cpp:81 > + // in 80-bit precision which messes up rounding. "messes up rounding" -> "is not consistent with gcc" > Source/WebCore/platform/audio/AudioUtilities.cpp:87 > +// Restore normal floating-point semantics, just in case. "normal" -> "default" I'd remove the ", just in case" clause > Source/WebCore/webaudio/AudioBufferSourceNode.cpp:105 > + double sampleRate = static_cast<double>(this->sampleRate()); We don't need to static_cast<double> here, do we? > Source/WebCore/webaudio/AudioBufferSourceNode.cpp:249 > + // is 1.) We choose the latter method. This comment seems a bit wordy. I would try to simplify it to something like: // Avoid converting from time to sample-frame twice, by first computing grain end time directly in seconds. > Source/WebCore/webaudio/AudioDestinationNode.cpp:86 > + // Advance current sample frame. consistency nit: you use "sample-frame" in the header file > LayoutTests/webaudio/note-grain-on-timing.html:20 > + var extraFramesHRTF = 512; Please add comment with URL to the bug you created for this. > LayoutTests/webaudio/note-grain-on-timing.html:29 > + // add a little bit of silence and also account for the extra "add a little bit of silence" -> "add a little bit of silence so we can detect pulse boundaries" > LayoutTests/webaudio/note-grain-on-timing.html:66 > + // Find a non-zero point and record it. We're note typo: "note" -> "not" > LayoutTests/webaudio/note-grain-on-timing.html:155 > + // the correct part of the source is being played at the grammar nit: "is" seems like it should be taken out
Raymond Toy
Comment 17 2012-01-27 10:51:40 PST
Comment on attachment 124187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review >>>> LayoutTests/webaudio/note-grain-on-timing.html:150 >>>> + bufferSource.noteGrainOn(time, 0, duration); >>> >>> nit: don't need to fix now, but definitely should add a "FIXME:" comment here >>> >>> We're not really exercising noteGrainOn() very fully here because we're always passing 0 in for grainOffset. >>> A more comprehensive test might create a linear-ramp buffer instead of a square-pulse. >>> Then test various noteGrainOn() calls with differing grainOffsets, and validate that the appropriate sub-section of the buffer is correctly rendered. >> >> That's why the test is called note-grain-on-timing. I certainly agree that we need more tests for note-grain-on, but I'd like to keep this test focussed on just this timing aspect. Can we write a bug saying we need more tests for noteGrainOn and perhaps update the test then? Or add a new test, as appropriate. > > Ok, that makes sense. I decided to take a closer look at this. I changed it to say noteGrainOn(time, k * .001, duration), where k is the k'th pulse being played. Half of the pulses now end at the wrong time. Investigating the cause of this now.
Raymond Toy
Comment 18 2012-01-27 13:20:38 PST
(In reply to comment #17) > (From update of attachment 124187 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124187&action=review > > >>>> LayoutTests/webaudio/note-grain-on-timing.html:150 > >>>> + bufferSource.noteGrainOn(time, 0, duration); > >>> > >>> nit: don't need to fix now, but definitely should add a "FIXME:" comment here > >>> > >>> We're not really exercising noteGrainOn() very fully here because we're always passing 0 in for grainOffset. > >>> A more comprehensive test might create a linear-ramp buffer instead of a square-pulse. > >>> Then test various noteGrainOn() calls with differing grainOffsets, and validate that the appropriate sub-section of the buffer is correctly rendered. > >> > >> That's why the test is called note-grain-on-timing. I certainly agree that we need more tests for note-grain-on, but I'd like to keep this test focussed on just this timing aspect. Can we write a bug saying we need more tests for noteGrainOn and perhaps update the test then? Or add a new test, as appropriate. > > > > Ok, that makes sense. > > I decided to take a closer look at this. I changed it to say noteGrainOn(time, k * .001, duration), where k is the k'th pulse being played. Half of the pulses now end at the wrong time. Investigating the cause of this now. Found the problem. noteGrainOn needs to use timeToSampleFrame to set the virtual index correctly. The original version used floor, some about half the time the virtual index would be different, causing the pulse to end early. Tests updated to use a variable grain offset.
Raymond Toy
Comment 19 2012-01-27 14:10:44 PST
Raymond Toy
Comment 20 2012-01-27 14:13:55 PST
Comment on attachment 124253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124253&action=review >> Source/WebCore/platform/audio/AudioUtilities.cpp:79 >> + // with Visual Studio, It forces the rounding of each operation > > nit: "It" -> "it" I see that "it" is too ambiguous here. Replaced with "these assigments force" >> Source/WebCore/platform/audio/AudioUtilities.cpp:81 >> + // in 80-bit precision which messes up rounding. > > "messes up rounding" -> "is not consistent with gcc" Done. >> Source/WebCore/platform/audio/AudioUtilities.cpp:87 >> +// Restore normal floating-point semantics, just in case. > > "normal" -> "default" > > I'd remove the ", just in case" clause Done. >> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:105 >> + double sampleRate = static_cast<double>(this->sampleRate()); > > We don't need to static_cast<double> here, do we? Removed. >> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:249 >> + // is 1.) We choose the latter method. > > This comment seems a bit wordy. I would try to simplify it to something like: > > // Avoid converting from time to sample-frame twice, by first computing grain end time directly in seconds. Replaced with shorter text. >> Source/WebCore/webaudio/AudioDestinationNode.cpp:86 >> + // Advance current sample frame. > > consistency nit: you use "sample-frame" in the header file Fixed. >> LayoutTests/webaudio/note-grain-on-timing.html:20 >> + var extraFramesHRTF = 512; > > Please add comment with URL to the bug you created for this. Done. >> LayoutTests/webaudio/note-grain-on-timing.html:29 >> + // add a little bit of silence and also account for the extra > > "add a little bit of silence" -> "add a little bit of silence so we can detect pulse boundaries" Done. >> LayoutTests/webaudio/note-grain-on-timing.html:66 >> + // Find a non-zero point and record it. We're note > > typo: "note" -> "not" Fixed. >> LayoutTests/webaudio/note-grain-on-timing.html:155 >> + // the correct part of the source is being played at the > > grammar nit: "is" seems like it should be taken out Removed this section with a URL to the bug saying we need more tests.
Raymond Toy
Comment 21 2012-01-27 14:14:46 PST
Review comments should be addressed. I ran tests on Linux, Mac, and Windows. note-grain-on-timing passes on all three.
Chris Rogers
Comment 22 2012-01-27 14:31:08 PST
Looks good. I've asked Ken to have one last look at the #pragma compiler part.
Kenneth Russell
Comment 23 2012-01-27 14:31:16 PST
Comment on attachment 124365 [details] Patch The MSVC pragmas look OK to me.
Kenneth Russell
Comment 24 2012-01-27 14:33:02 PST
Comment on attachment 124365 [details] Patch rs=me then.
Raymond Toy
Comment 25 2012-01-27 14:37:20 PST
Thanks for reviewing this patch. I will send a note on IRC to the gardeners that this patch will cause one of the windows tests (audiobuffersource-playback) to pass which was previously marked as fail in the test expectations. With this patch, the test should pass now.
Kenneth Russell
Comment 26 2012-01-27 14:50:42 PST
(In reply to comment #25) > Thanks for reviewing this patch. > > I will send a note on IRC to the gardeners that this patch will cause one of the windows tests (audiobuffersource-playback) to pass which was previously marked as fail in the test expectations. With this patch, the test should pass now. Why not just make life easier on the gardeners and update LayoutTests/platform/chromium/test_expectations.txt in this patch?
Raymond Toy
Comment 27 2012-01-27 14:53:31 PST
(In reply to comment #26) > (In reply to comment #25) > > Thanks for reviewing this patch. > > > > I will send a note on IRC to the gardeners that this patch will cause one of the windows tests (audiobuffersource-playback) to pass which was previously marked as fail in the test expectations. With this patch, the test should pass now. > > Why not just make life easier on the gardeners and update LayoutTests/platform/chromium/test_expectations.txt in this patch? Is that possible? Doesn't that change so fast that I can't possibly land the patch (via commit-queue because I'm not a committer)? I guess that that probably won't conflict with any one else's change.
WebKit Review Bot
Comment 28 2012-01-27 15:31:52 PST
Comment on attachment 124365 [details] Patch Clearing flags on attachment: 124365 Committed r106162: <http://trac.webkit.org/changeset/106162>
WebKit Review Bot
Comment 29 2012-01-27 15:31:59 PST
All reviewed patches have been landed. Closing bug.
Kenneth Russell
Comment 30 2012-01-27 15:42:28 PST
(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > Thanks for reviewing this patch. > > > > > > I will send a note on IRC to the gardeners that this patch will cause one of the windows tests (audiobuffersource-playback) to pass which was previously marked as fail in the test expectations. With this patch, the test should pass now. > > > > Why not just make life easier on the gardeners and update LayoutTests/platform/chromium/test_expectations.txt in this patch? > > Is that possible? Doesn't that change so fast that I can't possibly land the patch (via commit-queue because I'm not a committer)? I guess that that probably won't conflict with any one else's change. If the expectation is somewhere in the middle of the file, I doubt you'd get a conflict when landing via the commit queue.
Note You need to log in before you can comment on or make changes to this bug.