Bug 90357 - DelayNode doesn't work if delayTime.value == delayTime.maxValue
Summary: DelayNode doesn't work if delayTime.value == delayTime.maxValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-01 23:58 PDT by Wang Jiajun
Modified: 2012-07-27 01:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.38 KB, patch)
2012-07-10 15:47 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2012-07-11 11:12 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2012-07-13 09:53 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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).