Bug 222098

Summary: Regression: Raw AudioBufferSourceNode playback causes repeated crackling sound
Product: WebKit Reporter: jujjyl
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, anthony.bowker, cdumez, darin, eric.amram, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, keith_miller, kris, mark.lam, peng.liu6, philipj, saam, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac (Intel)   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226358
Attachments:
Description Flags
Patch
none
Patch none

Description jujjyl 2021-02-18 02:30:59 PST
STR: Visit and play https://smashkarts.io/webgl2/ for some minutes

Audio playback will begin to crackle.

Earlier Safari versions work ok.

More information can be found in this thread: https://forum.unity.com/threads/request-for-help-does-your-webgl-2-game-render-correctly-in-safari-tech-preview.1050890/#post-6839972 along with a video of the issue.

Unity uses Web Audio with uncompressed AudioBufferSourceNodes to play back audio, so scheduling audio samples is done inside the browser. The uncompressed audio nodes contain the "full" audio clips without JS/Wasm software mixing that would be applied.
Comment 1 Radar WebKit Bug Importer 2021-02-19 18:34:46 PST
<rdar://problem/74546471>
Comment 2 Chris Dumez 2021-02-22 15:44:43 PST
I can reproduce with the latest build on WebKit indeed.
Comment 3 Chris Dumez 2021-02-22 16:04:39 PST
It does not appear to be a super recent WebKit regression. I went as far back as r269041 (from Oct 2020) and could already reproduce the issue. Older WebKit builds do not run on my build of macOS.
Comment 4 jujjyl 2021-05-17 04:54:28 PDT
This issue has been since reported a number of times, it looks like it affects a lot of Unity users on the web. Any chance any updates on this?
Comment 5 Chris Dumez 2021-05-17 10:08:18 PDT
(In reply to jujjyl from comment #4)
> This issue has been since reported a number of times, it looks like it
> affects a lot of Unity users on the web. Any chance any updates on this?

A reduced test case would be very helpful.
Comment 6 jujjyl 2021-05-20 00:03:34 PDT
The projects at https://github.com/Unity-Technologies/ProjectTinySamples/ have also been reported to exhibit the same issue. Those are somewhat smaller than the classic Unity builds.
Comment 7 kris 2021-05-21 15:01:13 PDT
Our JS web game (not Unity-based) is also having this issues since Safari 14.1 on both Mac and IOS devices.

Here's a simple JSFiddle that shows the issue by playing a ton of sounds. Click on the 'Start Adding Sounds' button, and after roughly 10 seconds the crackling sounds begins. Note - this issue does not occur on MacOS Chrome browsers.

https://jsfiddle.net/KrisJohnson/s5vL24o1/123/

I've reduced the code as much as possible and it appears to happen when using panner objects. Let me know if I can be of help.
Comment 8 Eric 2021-05-27 10:23:09 PDT
Same on latest iOS 14.5.1
Comment 9 Chris Dumez 2021-05-27 10:26:24 PDT
(In reply to kris from comment #7)
> Our JS web game (not Unity-based) is also having this issues since Safari
> 14.1 on both Mac and IOS devices.
> 
> Here's a simple JSFiddle that shows the issue by playing a ton of sounds.
> Click on the 'Start Adding Sounds' button, and after roughly 10 seconds the
> crackling sounds begins. Note - this issue does not occur on MacOS Chrome
> browsers.
> 
> https://jsfiddle.net/KrisJohnson/s5vL24o1/123/
> 
> I've reduced the code as much as possible and it appears to happen when
> using panner objects. Let me know if I can be of help.

Thanks for the reduced test case, I'll look into it soon.
Comment 10 Chris Dumez 2021-05-27 10:37:29 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to kris from comment #7)
> > Our JS web game (not Unity-based) is also having this issues since Safari
> > 14.1 on both Mac and IOS devices.
> > 
> > Here's a simple JSFiddle that shows the issue by playing a ton of sounds.
> > Click on the 'Start Adding Sounds' button, and after roughly 10 seconds the
> > crackling sounds begins. Note - this issue does not occur on MacOS Chrome
> > browsers.
> > 
> > https://jsfiddle.net/KrisJohnson/s5vL24o1/123/
> > 
> > I've reduced the code as much as possible and it appears to happen when
> > using panner objects. Let me know if I can be of help.
> 
> Thanks for the reduced test case, I'll look into it soon.

I am totally able to reproduce with the provided test case on my system Safari. This should help me a lot.
Comment 11 Chris Dumez 2021-05-27 10:45:43 PDT
(In reply to Chris Dumez from comment #10)
> (In reply to Chris Dumez from comment #9)
> > (In reply to kris from comment #7)
> > > Our JS web game (not Unity-based) is also having this issues since Safari
> > > 14.1 on both Mac and IOS devices.
> > > 
> > > Here's a simple JSFiddle that shows the issue by playing a ton of sounds.
> > > Click on the 'Start Adding Sounds' button, and after roughly 10 seconds the
> > > crackling sounds begins. Note - this issue does not occur on MacOS Chrome
> > > browsers.
> > > 
> > > https://jsfiddle.net/KrisJohnson/s5vL24o1/123/
> > > 
> > > I've reduced the code as much as possible and it appears to happen when
> > > using panner objects. Let me know if I can be of help.
> > 
> > Thanks for the reduced test case, I'll look into it soon.
> 
> I am totally able to reproduce with the provided test case on my system
> Safari. This should help me a lot.

Interestingly, the issue seems to stop reproducing when dropping the PannerNode from the graph so maybe the issue is not strictly related to AudioBufferSourceNode.
Comment 12 Chris Dumez 2021-05-27 16:15:22 PDT
I initially see us destroying AudioBufferSourceNode / PannerNode objects as they finish playing. However, after a little while, we stop destroying them (or at least we can't keep up with the rate at which they are created). When we end up with ~2000 AudioBufferSourceNode, the audio gets bad.

I haven't figured out yet why the nodes aren't getting deallocated promptly but it is a start.
Comment 13 kris 2021-05-28 07:06:12 PDT
It makes me wonder if my test case identifies a separate issue or if it's another way to expose the problem, as our application and I imagine others are using far fewer than 2000 simultaneous AudioBufferSourceNodes. In our case (and it appears similar to the OP), we play various audio samples of varying lengths, some of which are looping. Then after roughly ~10 seconds, the crackling starts.
Comment 14 kris 2021-05-28 07:12:43 PDT
And to clarify, the crackling only happens when using PannerNodes.
Comment 15 Chris Dumez 2021-05-28 08:11:26 PDT
The javascript wrappers are no longer getting garbage collected in a timely fashion after a while. As a result, our implementation nodes aren't either.

It could be a JSC garbage collection issue or an issue with AudioScheduledSourceNode::virtualHasPendingActivity() as the wrapper stays alive as long as this function returns true.

The implementation of AudioScheduledSourceNode::virtualHasPendingActivity() seems sensible though:
```
bool AudioScheduledSourceNode::virtualHasPendingActivity() const
{
    return m_hasEndedEventListener && m_playbackState != FINISHED_STATE && !isMarkedForDeletion() && !context().isClosed();
}
```

And from my logging, I do see m_playbackState becoming FINISHED_STATE in a timely fashion.
Comment 16 Chris Dumez 2021-05-28 08:25:51 PDT
It definitely seems there is a garbage collection issue here. When I start playing, I see some calls to AudioScheduledSourceNode::virtualHasPendingActivity(), which return false and I see JS wrappers getting garbage collection. HOWEVER, after this initial period, we go for an extended period of time without AudioScheduledSourceNode::virtualHasPendingActivity() getting called (which seems to indicate the JS wrapper aren't getting visited). Since the JS is constructing JS wrappers very aggressively and since we are not collecting them, the number of JS wrappers grows pretty quickly, once we reach ~12k-15k JS wrappers, the audio quality starts to deteriorate and craters very quickly. 

I could use some help from our GC experts here to understand why our nodes aren't getting visited in a timely fashion, allowing the number of wrappers to grow so quickly.
Comment 17 Chris Dumez 2021-05-28 08:28:15 PDT
(In reply to Chris Dumez from comment #16)
> It definitely seems there is a garbage collection issue here. When I start
> playing, I see some calls to
> AudioScheduledSourceNode::virtualHasPendingActivity(), which return false
> and I see JS wrappers getting garbage collection. HOWEVER, after this
> initial period, we go for an extended period of time without
> AudioScheduledSourceNode::virtualHasPendingActivity() getting called (which
> seems to indicate the JS wrapper aren't getting visited). Since the JS is
> constructing JS wrappers very aggressively and since we are not collecting
> them, the number of JS wrappers grows pretty quickly, once we reach ~12k-15k
> JS wrappers, the audio quality starts to deteriorate and craters very
> quickly. 
> 
> I could use some help from our GC experts here to understand why our nodes
> aren't getting visited in a timely fashion, allowing the number of wrappers
> to grow so quickly.

The visitor logic is in generated bindings code, here:
```
bool JSAudioScheduledSourceNodeOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, AbstractSlotVisitor& visitor, const char** reason)
{
    auto* jsAudioScheduledSourceNode = jsCast<JSAudioScheduledSourceNode*>(handle.slot()->asCell());
    auto& wrapped = jsAudioScheduledSourceNode->wrapped();
    if (!wrapped.isContextStopped() && wrapped.hasPendingActivity()) {
        if (UNLIKELY(reason))
            *reason = "ActiveDOMObject with pending activity";
        return true;
     }
    if (jsAudioScheduledSourceNode->wrapped().isFiringEventListeners()) {
        if (UNLIKELY(reason))
            *reason = "EventTarget firing event listeners";
        return true;
    }
    UNUSED_PARAM(visitor);
    UNUSED_PARAM(reason);
    return false;
}
```
Comment 18 Chris Dumez 2021-05-28 08:37:52 PDT
(In reply to Chris Dumez from comment #17)
> (In reply to Chris Dumez from comment #16)
> > It definitely seems there is a garbage collection issue here. When I start
> > playing, I see some calls to
> > AudioScheduledSourceNode::virtualHasPendingActivity(), which return false
> > and I see JS wrappers getting garbage collection. HOWEVER, after this
> > initial period, we go for an extended period of time without
> > AudioScheduledSourceNode::virtualHasPendingActivity() getting called (which
> > seems to indicate the JS wrapper aren't getting visited). Since the JS is
> > constructing JS wrappers very aggressively and since we are not collecting
> > them, the number of JS wrappers grows pretty quickly, once we reach ~12k-15k
> > JS wrappers, the audio quality starts to deteriorate and craters very
> > quickly. 
> > 
> > I could use some help from our GC experts here to understand why our nodes
> > aren't getting visited in a timely fashion, allowing the number of wrappers
> > to grow so quickly.
> 
> The visitor logic is in generated bindings code, here:
> ```
> bool
> JSAudioScheduledSourceNodeOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::
> Unknown> handle, void*, AbstractSlotVisitor& visitor, const char** reason)
> {
>     auto* jsAudioScheduledSourceNode =
> jsCast<JSAudioScheduledSourceNode*>(handle.slot()->asCell());
>     auto& wrapped = jsAudioScheduledSourceNode->wrapped();
>     if (!wrapped.isContextStopped() && wrapped.hasPendingActivity()) {
>         if (UNLIKELY(reason))
>             *reason = "ActiveDOMObject with pending activity";
>         return true;
>      }
>     if (jsAudioScheduledSourceNode->wrapped().isFiringEventListeners()) {
>         if (UNLIKELY(reason))
>             *reason = "EventTarget firing event listeners";
>         return true;
>     }
>     UNUSED_PARAM(visitor);
>     UNUSED_PARAM(reason);
>     return false;
> }
> ```

Thinking about this more, it may be acceptable from GC's point of view to not collect if the memory usage doesn't get too high. I think part of the issue may be that our WebAudio implementation relies on the AudioNodes's JS wrappers to get collected promptly to maintain performance. I suspect the issue may be that AudioScheduledSourceNodes don't break their connections to other nodes when finishing (until their ref count goes to 0, which cannot happen while the JS wrapper is alive). As a result, the audio graph becomes very large very quickly and we have to traverse this very large audio graph for every render quantum, thus the big perf hit.

I'll see if I can take the AudioScheduledSourceNode out of the graph as soon as it's finished playing.
Comment 19 Chris Dumez 2021-05-28 09:38:53 PDT
(In reply to kris from comment #14)
> And to clarify, the crackling only happens when using PannerNodes.

Actually, you can actually replace the PannerNode in your demo with a DelayNode and the same issue will happen. I think it is about the AudioBufferSourceNode not being connected directly to the listenerGain somehow.
Comment 20 Chris Dumez 2021-05-28 10:01:10 PDT
(In reply to Chris Dumez from comment #19)
> (In reply to kris from comment #14)
> > And to clarify, the crackling only happens when using PannerNodes.
> 
> Actually, you can actually replace the PannerNode in your demo with a
> DelayNode and the same issue will happen. I think it is about the
> AudioBufferSourceNode not being connected directly to the listenerGain
> somehow.

Ok, some progress, if I make PannerNode::requiresTailProcessing() return false then the issue no longer reproduces. So the issue likely only reproduces if the AudioBufferSource is connected to a node that requires tail processing (Both Delay and Panner nodes) require tail processing.

I think it is related to this code in AudioNode::disableOutputsIfNecessary():
```
    if (m_connectionRefCount <= 1 && !m_isDisabled) {
        if (!requiresTailProcessing())
            disableOutputs();
    }
```

disableOutputs() is not getting called because requiresTailProcessing() returns true.
disableOutputsIfNecessary() gets called when the AudioNode loses its last connection.
Comment 21 Chris Dumez 2021-05-28 10:47:46 PDT
(In reply to Chris Dumez from comment #20)
> (In reply to Chris Dumez from comment #19)
> > (In reply to kris from comment #14)
> > > And to clarify, the crackling only happens when using PannerNodes.
> > 
> > Actually, you can actually replace the PannerNode in your demo with a
> > DelayNode and the same issue will happen. I think it is about the
> > AudioBufferSourceNode not being connected directly to the listenerGain
> > somehow.
> 
> Ok, some progress, if I make PannerNode::requiresTailProcessing() return
> false then the issue no longer reproduces. So the issue likely only
> reproduces if the AudioBufferSource is connected to a node that requires
> tail processing (Both Delay and Panner nodes) require tail processing.
> 
> I think it is related to this code in AudioNode::disableOutputsIfNecessary():
> ```
>     if (m_connectionRefCount <= 1 && !m_isDisabled) {
>         if (!requiresTailProcessing())
>             disableOutputs();
>     }
> ```
> 
> disableOutputs() is not getting called because requiresTailProcessing()
> returns true.
> disableOutputsIfNecessary() gets called when the AudioNode loses its last
> connection.

Seems to indicate we need to keep track of the nodes doing tail processing and disable their outputs once they're done processing their tail. I am working on a patch.
Comment 22 Chris Dumez 2021-05-28 13:56:03 PDT
Created attachment 430049 [details]
Patch
Comment 23 Chris Dumez 2021-05-28 14:01:31 PDT
A temporary workaround for people until this fix ships would likely be to manually remove nodes from the graph once they are no longer useful.
Comment 24 Eric 2021-05-28 14:12:24 PDT
(In reply to Chris Dumez from comment #22)
> Created attachment 430049 [details]
> Patch

Great news, congrats for the patch Chris!

Looking forward to testing it on iOS too.
Comment 25 Darin Adler 2021-05-28 15:13:44 PDT
Comment on attachment 430049 [details]
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.
Comment 26 Chris Dumez 2021-05-28 15:19:04 PDT
Comment on attachment 430049 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:629
>> +    // 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.

Vector appends don't always allocate. The assertions help us make sure we keep allocations to a minimum.

>> Source/WebCore/Modules/webaudio/BaseAudioContext.h:325
>> +        { }
> 
> 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.

Since we have a custom destructor so no move constructor would get generated by default unless I forced it. I guess `TailProcessingNode(TailProcessingNode&&) = default;` would do the trick.

>> Source/WebCore/Modules/webaudio/BaseAudioContext.h:329
>> +                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.

Are you suggested I use a Ref<> that's already been moved out here? If so, I don't like the idea and Ref::ptr() would hit an assertion.
Comment 27 Peng Liu 2021-05-28 15:29:34 PDT
Comment on attachment 430049 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        The issue was that once those nodes had finished processing their tail, we wouldn't come

Can we let the node which just finished processing its tail to put itself into some list? I guess that will make the job of clean up the list a bit easier. Not sure it is better/feasible though.
Comment 28 Darin Adler 2021-05-28 16:25:24 PDT
Comment on attachment 430049 [details]
Patch

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

>>> Source/WebCore/Modules/webaudio/BaseAudioContext.h:325
>>> +        { }
>> 
>> 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.
> 
> Since we have a custom destructor so no move constructor would get generated by default unless I forced it. I guess `TailProcessingNode(TailProcessingNode&&) = default;` would do the trick.

Makes sense. Maybe the copy constructor is not a problem, and the  assignment operator is the remaining problem. I think it should be deleted for completeness even though there’s little practical risk of it being used by accident. Not sure if you need to delete one or both of the assignment operators. Probably only one.

>>> Source/WebCore/Modules/webaudio/BaseAudioContext.h:329
>>> +                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.
> 
> Are you suggested I use a Ref<> that's already been moved out here? If so, I don't like the idea and Ref::ptr() would hit an assertion.

OK, then Ref<> makes the move constructor automatically correct, but makes the destructor impossible to write. Too bad!
Comment 29 Chris Dumez 2021-05-28 16:32:29 PDT
Comment on attachment 430049 [details]
Patch

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

>>>> Source/WebCore/Modules/webaudio/BaseAudioContext.h:325
>>>> +        { }
>>> 
>>> 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.
>> 
>> Since we have a custom destructor so no move constructor would get generated by default unless I forced it. I guess `TailProcessingNode(TailProcessingNode&&) = default;` would do the trick.
> 
> Makes sense. Maybe the copy constructor is not a problem, and the  assignment operator is the remaining problem. I think it should be deleted for completeness even though there’s little practical risk of it being used by accident. Not sure if you need to delete one or both of the assignment operators. Probably only one.

You said switching to Ref<> would make the generated move constructor correct. However, I don't understand what would be the issue the the default move constructor generated if my data member is a RefPtr<>. My understanding is that it would be identical to what I wrote. I still decided to write it out because I thought it was clearer.
Comment 30 Darin Adler 2021-05-28 16:42:59 PDT
Comment on attachment 430049 [details]
Patch

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

>>>>> Source/WebCore/Modules/webaudio/BaseAudioContext.h:325
>>>>> +        { }
>>>> 
>>>> 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.
>>> 
>>> Since we have a custom destructor so no move constructor would get generated by default unless I forced it. I guess `TailProcessingNode(TailProcessingNode&&) = default;` would do the trick.
>> 
>> Makes sense. Maybe the copy constructor is not a problem, and the  assignment operator is the remaining problem. I think it should be deleted for completeness even though there’s little practical risk of it being used by accident. Not sure if you need to delete one or both of the assignment operators. Probably only one.
> 
> You said switching to Ref<> would make the generated move constructor correct. However, I don't understand what would be the issue the the default move constructor generated if my data member is a RefPtr<>. My understanding is that it would be identical to what I wrote. I still decided to write it out because I thought it was clearer.

I agree that it’s clearer, because we require the "exchange" semantic here, where RefPtr is making a design choice that could change some day in theory (although in practice we won’t change it). I missed that angle before, but now I see it, thanks to your patient explanation.
Comment 31 kris 2021-05-28 17:07:05 PDT
(In reply to Chris Dumez from comment #23)
> A temporary workaround for people until this fix ships would likely be to
> manually remove nodes from the graph once they are no longer useful.

Thanks Chris, your workaround fixes the crackle for us!
Comment 32 Chris Dumez 2021-05-28 17:08:00 PDT
(In reply to kris from comment #31)
> (In reply to Chris Dumez from comment #23)
> > A temporary workaround for people until this fix ships would likely be to
> > manually remove nodes from the graph once they are no longer useful.
> 
> Thanks Chris, your workaround fixes the crackle for us!

Great news! I understand people want to get things working ASAP :)
Comment 33 Chris Dumez 2021-05-28 17:12:03 PDT
Created attachment 430076 [details]
Patch
Comment 34 EWS 2021-05-28 18:28:19 PDT
Committed r278233 (238270@main): <https://commits.webkit.org/238270@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430076 [details].
Comment 35 Chris Dumez 2021-05-30 17:58:19 PDT
Tiny follow-up assertion fix in <https://commits.webkit.org/238300@main>.
Comment 36 Aakash Jain 2021-05-31 19:40:08 PDT
(In reply to EWS from comment #34)
> Committed r278233 (238270@main): <https://commits.webkit.org/238270@main>
This broke imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-interface/active-processing.https.html which still seems to be broken on iOS after the follow-up fix.

History: https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fwebaudio%2Fthe-audio-api%2Fthe-convolvernode-interface%2Factive-processing.https.html
Comment 37 Chris Dumez 2021-05-31 19:40:55 PDT
(In reply to Aakash Jain from comment #36)
> (In reply to EWS from comment #34)
> > Committed r278233 (238270@main): <https://commits.webkit.org/238270@main>
> This broke
> imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-
> interface/active-processing.https.html which still seems to be broken on iOS
> after the follow-up fix.
> 
> History:
> https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> platform-tests%2Fwebaudio%2Fthe-audio-api%2Fthe-convolvernode-
> interface%2Factive-processing.https.html

Ok, looking now
Comment 38 Chris Dumez 2021-05-31 19:45:07 PDT
(In reply to Chris Dumez from comment #37)
> (In reply to Aakash Jain from comment #36)
> > (In reply to EWS from comment #34)
> > > Committed r278233 (238270@main): <https://commits.webkit.org/238270@main>
> > This broke
> > imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-
> > interface/active-processing.https.html which still seems to be broken on iOS
> > after the follow-up fix.
> > 
> > History:
> > https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> > platform-tests%2Fwebaudio%2Fthe-audio-api%2Fthe-convolvernode-
> > interface%2Factive-processing.https.html
> 
> Ok, looking now

Looks like a potential progression:
--- /Volumes/Data/worker/ios-simulator-14-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-interface/active-processing.https-expected.txt
+++ /Volumes/Data/worker/ios-simulator-14-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-interface/active-processing.https-actual.txt
@@ -8,7 +8,8 @@
 PASS < [initialize] All assertions passed. (total 1 assertions)
 PASS > [test]
 PASS   Test 0: Number of convolver output channels is equal to 2.
-FAIL X Number of distinct values is not equal to 2. Got 1. assert_true: expected true got false
-FAIL < [test] 1 out of 2 assertions were failed. assert_true: expected true got false
+FAIL X Test 1: Number of convolver output channels is not equal to 1. Got 0. assert_true: expected true got false
+PASS   Number of distinct values is equal to 2.
+FAIL < [test] 1 out of 3 assertions were failed. assert_true: expected true got false
 FAIL # AUDIT TASK RUNNER FINISHED: 1 out of 2 tasks were failed. assert_true: expected true got false
Comment 39 Chris Dumez 2021-05-31 19:46:38 PDT
(In reply to Chris Dumez from comment #38)
> (In reply to Chris Dumez from comment #37)
> > (In reply to Aakash Jain from comment #36)
> > > (In reply to EWS from comment #34)
> > > > Committed r278233 (238270@main): <https://commits.webkit.org/238270@main>
> > > This broke
> > > imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-
> > > interface/active-processing.https.html which still seems to be broken on iOS
> > > after the follow-up fix.
> > > 
> > > History:
> > > https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> > > platform-tests%2Fwebaudio%2Fthe-audio-api%2Fthe-convolvernode-
> > > interface%2Factive-processing.https.html
> > 
> > Ok, looking now
> 
> Looks like a potential progression:
> ---
> /Volumes/Data/worker/ios-simulator-14-debug-tests-wk2/build/layout-test-
> results/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-
> convolvernode-interface/active-processing.https-expected.txt
> +++
> /Volumes/Data/worker/ios-simulator-14-debug-tests-wk2/build/layout-test-
> results/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-
> convolvernode-interface/active-processing.https-actual.txt
> @@ -8,7 +8,8 @@
>  PASS < [initialize] All assertions passed. (total 1 assertions)
>  PASS > [test]
>  PASS   Test 0: Number of convolver output channels is equal to 2.
> -FAIL X Number of distinct values is not equal to 2. Got 1. assert_true:
> expected true got false
> -FAIL < [test] 1 out of 2 assertions were failed. assert_true: expected true
> got false
> +FAIL X Test 1: Number of convolver output channels is not equal to 1. Got
> 0. assert_true: expected true got false
> +PASS   Number of distinct values is equal to 2.
> +FAIL < [test] 1 out of 3 assertions were failed. assert_true: expected true
> got false
>  FAIL # AUDIT TASK RUNNER FINISHED: 1 out of 2 tasks were failed.
> assert_true: expected true got false

We didn't see it on Mac because this test is already marked as flaky on that platform:
LayoutTests/platform/mac/TestExpectations:webkit.org/b/225421 imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-interface/active-processing.https.html [ Pass Failure ]
Comment 40 Chris Dumez 2021-05-31 19:56:35 PDT
(In reply to Chris Dumez from comment #39)
> (In reply to Chris Dumez from comment #38)
> > (In reply to Chris Dumez from comment #37)
> > > (In reply to Aakash Jain from comment #36)
> > > > (In reply to EWS from comment #34)
> > > > > Committed r278233 (238270@main): <https://commits.webkit.org/238270@main>
> > > > This broke
> > > > imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-
> > > > interface/active-processing.https.html which still seems to be broken on iOS
> > > > after the follow-up fix.
> > > > 
> > > > History:
> > > > https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> > > > platform-tests%2Fwebaudio%2Fthe-audio-api%2Fthe-convolvernode-
> > > > interface%2Factive-processing.https.html
> > > 
> > > Ok, looking now
> > 
> > Looks like a potential progression:
> > ---
> > /Volumes/Data/worker/ios-simulator-14-debug-tests-wk2/build/layout-test-
> > results/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-
> > convolvernode-interface/active-processing.https-expected.txt
> > +++
> > /Volumes/Data/worker/ios-simulator-14-debug-tests-wk2/build/layout-test-
> > results/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-
> > convolvernode-interface/active-processing.https-actual.txt
> > @@ -8,7 +8,8 @@
> >  PASS < [initialize] All assertions passed. (total 1 assertions)
> >  PASS > [test]
> >  PASS   Test 0: Number of convolver output channels is equal to 2.
> > -FAIL X Number of distinct values is not equal to 2. Got 1. assert_true:
> > expected true got false
> > -FAIL < [test] 1 out of 2 assertions were failed. assert_true: expected true
> > got false
> > +FAIL X Test 1: Number of convolver output channels is not equal to 1. Got
> > 0. assert_true: expected true got false
> > +PASS   Number of distinct values is equal to 2.
> > +FAIL < [test] 1 out of 3 assertions were failed. assert_true: expected true
> > got false
> >  FAIL # AUDIT TASK RUNNER FINISHED: 1 out of 2 tasks were failed.
> > assert_true: expected true got false
> 
> We didn't see it on Mac because this test is already marked as flaky on that
> platform:
> LayoutTests/platform/mac/TestExpectations:webkit.org/b/225421
> imported/w3c/web-platform-tests/webaudio/the-audio-api/the-convolvernode-
> interface/active-processing.https.html [ Pass Failure ]

Rebaselined test in <https://commits.webkit.org/r278293>.
Comment 41 kris 2021-06-21 08:21:09 PDT
(In reply to Chris Dumez from comment #32)
> (In reply to kris from comment #31)
> > (In reply to Chris Dumez from comment #23)
> > > A temporary workaround for people until this fix ships would likely be to
> > > manually remove nodes from the graph once they are no longer useful.
> > 
> > Thanks Chris, your workaround fixes the crackle for us!
> 
> Great news! I understand people want to get things working ASAP :)

Hey Chris, we're hearing the crackling issue again using your workaround in our product. Was the originally reported link tested and working correctly with your patch? This problem is such a show stopper for us I just want to confirm it's not a separate issue. Thanks!

Original reported issue:

STR: Visit and play https://smashkarts.io/webgl2/ for some minutes