[webkit-reviews] review granted: [Bug 222098] Regression: Raw AudioBufferSourceNode playback causes repeated crackling sound : [Attachment 430049] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 28 15:13:44 PDT 2021


Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 222098: Regression: Raw AudioBufferSourceNode playback causes repeated
crackling sound
https://bugs.webkit.org/show_bug.cgi?id=222098

Attachment 430049: Patch

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




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

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

The WinCairo build failure looks real.

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:629
> +    // Heap allocations are forbidden on the audio thread for performance
reasons so we need to
> +    // explicitly allow the following allocation(s).

What makes the following allocation OK? Just that it’s one of only a few?
Comment should make that clear too. This restriction isn’t great if we are
constantly disabling it. I’d like clearer explanation when we do of what’s
special.

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:661
> +	   // Heap allocations are forbidden on the audio thread for
performance reasons so we need to
> +	   // explicitly allow the following allocation(s).
> +	   DisableMallocRestrictionsForCurrentThreadScope
disableMallocRestrictions;

Ditto.

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:677
> +    // Heap allocations are forbidden on the audio thread for performance
reasons so we need to
> +    // explicitly allow the following allocation(s).
> +    DisableMallocRestrictionsForCurrentThreadScope
disableMallocRestrictions;

Ditto.

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:325
> +	   TailProcessingNode(TailProcessingNode&& other)
> +	       : m_node(std::exchange(other.m_node, nullptr))
> +	   { }

If we used Ref instead of RefPtr, I think this exact move constructor would be
generated for us, automatically without us having to write this code.

We probably need to delete the copy constructor, assignment operator, and
move-assignment operator. If any of them were used, setIsTailProcessing would
get out of whack. Maybe the copy constructor is automatically deleted because
we are explicitly providing a move constructor. The same is not true of the
assignment operators, though.

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:329
> +	       if (m_node)
> +		   m_node->setIsTailProcessing(false);

This would be a little harder to write if we did that, but we could just use
m_node.ptr(). I think it’s good on balance to switch to Ref.


More information about the webkit-reviews mailing list