[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