[webkit-reviews] review granted: [Bug 90357] DelayNode doesn't work if delayTime.value == delayTime.maxValue : [Attachment 151735] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Raymond Toy <rtoy at chromium.org>'s
request for review:
Bug 90357: DelayNode doesn't work if delayTime.value == delayTime.maxValue
https://bugs.webkit.org/show_bug.cgi?id=90357

Attachment 151735: Patch
https://bugs.webkit.org/attachment.cgi?id=151735&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.