[webkit-reviews] review granted: [Bug 226358] DelayDSPKernel::process() is slow : [Attachment 429943] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226358: DelayDSPKernel::process() is slow
https://bugs.webkit.org/show_bug.cgi?id=226358

Attachment 429943: Patch

https://bugs.webkit.org/attachment.cgi?id=429943&action=review




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 429943
  --> https://bugs.webkit.org/attachment.cgi?id=429943
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429943&action=review

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:40
> +    ASSERT(bufferLength >= framesToProcess);

I’m sure this is indeed safe. But Wwe could also just use std::clamp, std::min,
or std::max to be robust against this mistake instead o asserting.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:44
> +    float* writePointer = &buffer[writeIndex];

auto?

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:45
> +    int remainder = bufferLength - writeIndex;

If we want a signed type so make the negative number part work, maybe ptrdiff_t
would be a better choice.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:48
> +    memcpy(writePointer, source, sizeof(float) *
std::min(static_cast<int>(framesToProcess), remainder));

Might be better to use ptrdiff_t than int here.

I think that sizeof(*writePointer) would be even better than sizeof(float).
Could even create template function that guarantees this is done right, like a
memcpy that takes a number of items to copy instead of a number of bytes, and
does the multiplication with overflow checking done carefully.

This relies on framesToProcess/reminder not being huge numbers that would
overflow an int when multiplied by 4. That’s probably not a risk, but did you
think about where that overflow guarantee comes from?

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:49
> +    memcpy(buffer, source + remainder, sizeof(float) * std::max(0,
static_cast<int>(framesToProcess) - remainder));

Same issues.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:97
> +    if (UNLIKELY(!m_buffer.size() || !source || !destination))

isEmpty better than calling size?

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:110
> +    int bufferLength = m_buffer.size();

Seems like a bad patten to use int for sizes. I’d like to see auto here to
guarantee I am not chopping a bigger size down to a smaller one, or size_t
because that’s a natural for a size.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.cpp:141
> +    int bufferLength = m_buffer.size();

Ditto.