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

[bugzilla-daemon at webkit.org]

Raymond Toy <rtoy at chromium.org> has asked  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 Raymond Toy <rtoy at chromium.org>
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.