Summary: | DelayNode doesn't work if delayTime.value == delayTime.maxValue | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wang Jiajun <Amesists> | ||||||||
Component: | Web Audio | Assignee: | 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
Wang Jiajun
2012-07-01 23:58:48 PDT
Created attachment 151542 [details]
Patch
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 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. Created attachment 151735 [details]
Patch
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. Ken, Can you review this? 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. Created attachment 152281 [details]
Patch
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 on attachment 152281 [details]
Patch
Looks good. r=me
Comment on attachment 152281 [details]
Patch
Thanks Ken!
Comment on attachment 152281 [details] Patch Clearing flags on attachment: 152281 Committed r122630: <http://trac.webkit.org/changeset/122630> All reviewed patches have been landed. Closing bug. 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). |