Bug 90357 – DelayNode doesn't work if delayTime.value == delayTime.maxValue

• Component: Web Audio
• Status: RESOLVED FIXED

[Wang Jiajun]

If we create a DelayNode, and then set the delayTime.value to delayTime.maxValue. The output stream actually has no delay.

Ways to reproduce:
In the delaynode.html layout test, if you set delayTimeSeconds to 1, the test fails.

Expected result:
The test should pass and the output should have a delay for 1 second.

[Raymond Toy]

Created attachment 151542 [details]
Patch

[Chris Rogers]

Comment on attachment 151542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151542&action=review

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:40
> +// TODO(rtoy): Find out why 1 is not enough for a delay of 1.3333 sec.

WebKit style is FIXME:

Shouldn't be that hard to find out why?  It seems worth finding out what's going on right now rather than having a FIXME

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:76
> +    m_buffer.allocate(extraBufferFrames + bufferLength);

You should calculate bufferLength to include the "extraBufferFrames + " and not put it here, because in fact the real buffer length includes this extra amount.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:120
> +        ASSERT(readIndex1 != m_writeIndex);

How can you ASSERT this - doesn't this condition come up if the delay time is exactly 0?

> LayoutTests/webaudio/delaynode-max-default-delay.html:16
> +description("Tests DelayNode with delay set to default max delay.");

"max" -> "maximum"

> LayoutTests/webaudio/delaynode-max-nondefault-delay-expected.txt:1
> +Tests DelayNode with delay set to nondefault max delay.

"nondefault" -> "non-default"
"max" -> "maximum"

[Raymond Toy]

Comment on attachment 151542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151542&action=review

>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:40
>> +// TODO(rtoy): Find out why 1 is not enough for a delay of 1.3333 sec.
> 
> WebKit style is FIXME:
> 
> Shouldn't be that hard to find out why?  It seems worth finding out what's going on right now rather than having a FIXME

I believe this is a roundoff error.  See, for example, https://bugs.webkit.org/show_bug.cgi?id=78854.

>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:120
>> +        ASSERT(readIndex1 != m_writeIndex);
> 
> How can you ASSERT this - doesn't this condition come up if the delay time is exactly 0?

I tested this on a debug build and it didn't show up.  (Perhaps this also indicates a bug in the delay-testing.js)  I need to revisit this calculation and add a test for 0 delay.

[Raymond Toy]

Created attachment 151735 [details]
Patch

[Raymond Toy]

Comment on attachment 151542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151542&action=review

Also added a check for m_maxDelayTime > 0 to prevent a crash when allocating the buffer with an incorrect size.

>>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:40
>>> +// TODO(rtoy): Find out why 1 is not enough for a delay of 1.3333 sec.
>> 
>> WebKit style is FIXME:
>> 
>> Shouldn't be that hard to find out why?  It seems worth finding out what's going on right now rather than having a FIXME
> 
> I believe this is a roundoff error.  See, for example, https://bugs.webkit.org/show_bug.cgi?id=78854.

Fixed by using AudioUtilities::timeToSampleFrames to compute the buffer length.  But bug 78854 still exists.

FIXME removed.

>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:76
>> +    m_buffer.allocate(extraBufferFrames + bufferLength);
> 
> You should calculate bufferLength to include the "extraBufferFrames + " and not put it here, because in fact the real buffer length includes this extra amount.

Done.

>>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:120
>>> +        ASSERT(readIndex1 != m_writeIndex);
>> 
>> How can you ASSERT this - doesn't this condition come up if the delay time is exactly 0?
> 
> I tested this on a debug build and it didn't show up.  (Perhaps this also indicates a bug in the delay-testing.js)  I need to revisit this calculation and add a test for 0 delay.

Testing was incorrect.  ASSERT removed.

>> LayoutTests/webaudio/delaynode-max-default-delay.html:16
>> +description("Tests DelayNode with delay set to default max delay.");
> 
> "max" -> "maximum"

Done.

>> LayoutTests/webaudio/delaynode-max-nondefault-delay-expected.txt:1
>> +Tests DelayNode with delay set to nondefault max delay.
> 
> "nondefault" -> "non-default"
> "max" -> "maximum"

Done.

[Raymond Toy]

Ken,

Can you review this?

[Kenneth Russell]

Comment on attachment 151735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151735&action=review

The code looks good to me overall; r=me. A couple of minor comments. You can decide whether or not they need addressing.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:40
> +const size_t extraBufferFrames = 1;

Should this and the other constant above be in an anonymous namespace? Also, will one additional sample always be enough (i.e., sure you don't need something like 1 per channel)?

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:74
> +    size_t bufferLength = extraBufferFrames + AudioUtilities::timeToSampleFrame(maxDelayTime, sampleRate);

This is the same arithmetic expression as above -- I wonder whether you should have a helper function which computes this value given the maxDelayTime and sample rate.

[Raymond Toy]

Created attachment 152281 [details]
Patch

[Raymond Toy]

Comment on attachment 151735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151735&action=review

>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:40
>> +const size_t extraBufferFrames = 1;
> 
> Should this and the other constant above be in an anonymous namespace? Also, will one additional sample always be enough (i.e., sure you don't need something like 1 per channel)?

Removed extraBufferFrames (in new function) and moved the other constant to WebCore namespace.

One sample should be enough.  Multiple channels are handled at a higher layer in AudioDSPKernelProcessor so we only need to deal with one channel here.

>> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:74
>> +    size_t bufferLength = extraBufferFrames + AudioUtilities::timeToSampleFrame(maxDelayTime, sampleRate);
> 
> This is the same arithmetic expression as above -- I wonder whether you should have a helper function which computes this value given the maxDelayTime and sample rate.

Yes, a helper is better.  Done.

[Kenneth Russell]

Comment on attachment 152281 [details]
Patch

Looks good. r=me

[Raymond Toy]

Comment on attachment 152281 [details]
Patch

Thanks Ken!

[WebKit Review Bot]

Comment on attachment 152281 [details]
Patch

Clearing flags on attachment: 152281

Committed r122630: <http://trac.webkit.org/changeset/122630>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Comment on attachment 151735 [details]
Patch

Cleared review? from obsolete attachment 151735 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).