Bug 223380 – Regression(r273542) Degraded ScriptProcessorNode performance/quality for (14.0.3+ and iOS 14.4.1+)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Jack Schaedler]

After updating to Safari 14.0.3 or iOS 14.4.1, the audio quality on pages that use ScriptProcessorNode has degraded quite dramatically. This can be observed by visiting a page like  https://learningsynths.ableton.com/playground.

To reproduce: Grab one of the sliders and slowly drag it back and forth.

Prior to 14.0.3 and iOS14.4.1, the audio playback on this site was very smooth, even for devices like an iPhone 5s. After updating, desktop Safari (MacBook Pro 2015) crackles and pops intermittently (~every 1-3 seconds). The situation is a bit worse for iOS devices, where the audio is almost constantly glitchy on a 2020 iPhone SE.

An interesting finding:
Increasing the ScriptProcessorNode's buffer size does not mitigate the problem. Rather, at certain buffer sizes the glitching becomes very repetitive and almost periodic, leading one to believe that the issue is likely not due to the DSP failing to complete processing on time, but may have to do with the implementation of ScriptProcessorNode.

The site works well/smoothly on other browsers which support AudioWorklet: Firefox, Chrome, and Safari Tech Preview. Viewing the site in 14.0.3 or iOS 14.4.1 will trigger a fallback to ScriptProcessorNode since AudioWorklet is not yet supported there, and this will lead to the issues described above.

If the fallback to ScriptProcessorNode is forced on Chrome, similar glitching occurs with the latest stable version.

When considering mitigation strategies, it would be helpful to know if there's a timeframe for the release of AudioWorklet in stable builds of Safari. However, I do understand that this sort of information is usually not disclosed.

[Chris Dumez]

Potential regression from http://trac.webkit.org/r273542.

[Chris Dumez]

Created attachment 423499 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75530910>

[Geoffrey Garen]

Comment on attachment 423499 [details]
Patch

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

r=me

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:162
>      // Get input and output buffers. We double-buffer both the input and output sides.
>      unsigned doubleBufferIndex = this->doubleBufferIndex();
>      bool isDoubleBufferIndexGood = doubleBufferIndex < 2 && doubleBufferIndex < m_inputBuffers.size() && doubleBufferIndex < m_outputBuffers.size();
>      ASSERT(isDoubleBufferIndexGood);
>      if (!isDoubleBufferIndexGood)
>          return;

This code made it hard for me to understand that doubleBufferIndex is always 0 or 1, and is a simple buffer swapper. It made me think that there are some conditions under which we choose to set doubleBufferIndex to some other value, and those conditions are just not reachable at this point of this function. Which is not true. Since we already have a helper function for doubleBufferIndex(), if we really want to do validation, let's do it in doubleBufferIndex(). For example, we can change doubleBufferIndex() to return std::min(m_doubleBufferIndex, 1). Or we can just remove this nonsense because it is nonsense.

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:225
>              callOnMainThread([this, doubleBufferIndex = m_doubleBufferIndex, protector = makeRef(*this)] {
> -                auto locker = holdLock(m_processLock);
> +                auto locker = holdLock(m_bufferLocks[doubleBufferIndex]);
>                  fireProcessEvent(doubleBufferIndex);

It's not ideal to re-read m_doubleBufferIndex here (and a few lines above). Better to capture the local variable doubleBufferIndex instead. They'll always be the same in practice - but capturing the local helps to clarify that we're definitely holding the same lock. (Also kinda funny that we do all that nonsense validation of doubleBufferIndex above and then ignore it here!)

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.h:93
>      Vector<RefPtr<AudioBuffer>> m_inputBuffers;
>      Vector<RefPtr<AudioBuffer>> m_outputBuffers;

Let's change these Vectors to optional<std::array<2>>, to clarify their relationship to the 2 locks and their use in double buffering.

[Chris Dumez]

Created attachment 423503 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> Created attachment 423503 [details]
> Patch

Requesting review again due to substantial changes based on Geoff's feedback.

[Geoffrey Garen]

Comment on attachment 423503 [details]
Patch

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

r=me

I really like this.

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:132
> +    for (unsigned i = 0; i < bufferCount; ++i) {
> +        auto locker = holdLock(m_bufferLocks[i]);
> +        m_inputBuffers[i] = nullptr;
> +        m_outputBuffers[i] = nullptr;
> +    }

Nice!

[Chris Dumez]

Comment on attachment 423503 [details]
Patch

Clearing flags on attachment: 423503

Committed r274573 (235418@main): <https://commits.webkit.org/235418@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Eyal Cohen]

This is a critical issue for us, our web application makes extensive use of ScriptProcessorNode. 

Do you have an ETA for this fix?

[Chris Dumez]

(In reply to Eyal Cohen from comment #10)
> This is a critical issue for us, our web application makes extensive use of
> ScriptProcessorNode. 
> 
> Do you have an ETA for this fix?

I am not 100% sure but I think this script processor node fix is indeed in iOS 14.5 beta 7.