Bug 90357

Summary: DelayNode doesn't work if delayTime.value == delayTime.maxValue
Product: WebKit Reporter: Wang Jiajun <Amesists>
Component: Web AudioAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Normal CC: Amesists, crogers, eric.carlson, feature-media-reviews, kbr, rtoy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Wang Jiajun 2012-07-01 23:58:48 PDT
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.
Comment 1 Raymond Toy 2012-07-10 15:47:36 PDT
Created attachment 151542 [details]
Patch
Comment 2 Chris Rogers 2012-07-10 17:29:23 PDT
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"
Comment 3 Raymond Toy 2012-07-10 20:12:56 PDT
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.
Comment 4 Raymond Toy 2012-07-11 11:12:20 PDT
Created attachment 151735 [details]
Patch
Comment 5 Raymond Toy 2012-07-11 11:15:38 PDT
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.
Comment 6 Raymond Toy 2012-07-12 15:50:16 PDT
Ken,

Can you review this?
Comment 7 Kenneth Russell 2012-07-12 16:17:42 PDT
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.
Comment 8 Raymond Toy 2012-07-13 09:53:51 PDT
Created attachment 152281 [details]
Patch
Comment 9 Raymond Toy 2012-07-13 09:56:12 PDT
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.
Comment 10 Kenneth Russell 2012-07-13 10:36:48 PDT
Comment on attachment 152281 [details]
Patch

Looks good. r=me
Comment 11 Raymond Toy 2012-07-13 12:35:54 PDT
Comment on attachment 152281 [details]
Patch

Thanks Ken!
Comment 12 WebKit Review Bot 2012-07-13 14:10:48 PDT
Comment on attachment 152281 [details]
Patch

Clearing flags on attachment: 152281

Committed r122630: <http://trac.webkit.org/changeset/122630>
Comment 13 WebKit Review Bot 2012-07-13 14:10:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Adam Barth 2012-07-27 01:15:01 PDT
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).