Bug 76659 – Round time to sample frame

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Raymond Toy]

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.

[Raymond Toy]

Created attachment 123451 [details]
Patch

[Raymond Toy]

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 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]

Created attachment 124187 [details]
Patch

[Raymond Toy]

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]

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 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]

(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 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]

(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 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]

Created attachment 124252 [details]
Patch

[Raymond Toy]

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]

Created attachment 124253 [details]
Patch

[Raymond Toy]

All review comments should be covered now.

Everything still passes on OSX.  Still need to test on Linux and Windows.

[Chris Rogers]

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 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]

(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]

Created attachment 124365 [details]
Patch

[Raymond Toy]

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]

Review comments should be addressed.

I ran tests on Linux, Mac, and Windows.  note-grain-on-timing passes on all three.

[Chris Rogers]

Looks good.  I've asked Ken to have one last look at the #pragma compiler part.

[Kenneth Russell]

Comment on attachment 124365 [details]
Patch

The MSVC pragmas look OK to me.

[Kenneth Russell]

Comment on attachment 124365 [details]
Patch

rs=me then.

[Raymond Toy]

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]

(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]

(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 on attachment 124365 [details]
Patch

Clearing flags on attachment: 124365

Committed r106162: <http://trac.webkit.org/changeset/106162>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

(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.