⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 278233 in webkit


Ignore:
Timestamp:
May 28, 2021, 6:28:12 PM (5 years ago)
Author:
Chris Dumez
Message:

Regression: Raw AudioBufferSourceNode playback causes repeated crackling sound
​https://bugs.webkit.org/show_bug.cgi?id=222098
<rdar://problem/74546471>

Reviewed by Darin Adler.

The issue was due to certain audio nodes (such as AudioBufferSourceNode) staying in the audio
graph after they were no longer needed. As a result, the audio graph was becoming larger and
larger and the audio performance should degrade due to us traversing this increasingly large
graph.

The audio nodes that had trouble getting removed from the graphs were tail processing nodes.
Those are nodes that may still produce output for a while after they no longer have any
inputs. PannerNode and DelayNode are example of such nodes.

When a Node is no longer useful (the node no longer has any connections, m_connectionRefCount
is 0), we call disableOutputsIfNecessary() on it to disable its outputs and avoid doing any
processing of these outputs since they no longer have an input. disableOutputsIfNecessary()
would normally call disableOutputs() (if not already disabled) but there was an exception
in the case where the node requires tail processing. For such nodes, you wouldn't want to
disable them until they've finished processing their tail.

The issue was that once those nodes had finished processing their tail, we wouldn't come
back to them and disable their outputs later on. As a result, we would process more and
more (silent) nodes and the audio performance would seriously deteriorate.

To address this, we now add the node to the context's list of tail processing nodes in
disableOutputsIfNecessary() if the node requires tail processing. After each rendering
quantum, we go through those tail processing nodes and check if they have finished
processing their tail. If they have, we go ahead and disable their outputs at this point
and remove them from the list.

This is modeled after what Blink is doing for tail processing nodes in:

I have verified that on the following demo, the performance no longer deteriorates after
a while:

  • Modules/webaudio/AudioNode.cpp:

(WebCore::AudioNode::enableOutputsIfNecessary):
(WebCore::AudioNode::disableOutputsIfNecessary):

  • Modules/webaudio/AudioNode.h:

(WebCore::AudioNode::isTailProcessing const):
(WebCore::AudioNode::setIsTailProcessing):

  • Modules/webaudio/BaseAudioContext.cpp:

(WebCore::BaseAudioContext::uninitialize):
(WebCore::BaseAudioContext::handlePostRenderTasks):
(WebCore::BaseAudioContext::addTailProcessingNode):
(WebCore::BaseAudioContext::removeTailProcessingNode):
(WebCore::BaseAudioContext::updateTailProcessingNodes):
(WebCore::BaseAudioContext::disableOutputsForFinishedTailProcessingNodes):
(WebCore::BaseAudioContext::finishTailProcessing):

  • Modules/webaudio/BaseAudioContext.h:

(WebCore::BaseAudioContext::TailProcessingNode::TailProcessingNode):
(WebCore::BaseAudioContext::TailProcessingNode::~TailProcessingNode):
(WebCore::BaseAudioContext::TailProcessingNode::operator-> const):
(WebCore::BaseAudioContext::TailProcessingNode::operator== const):

Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r278231 r278233  
     12021-05-28  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression: Raw AudioBufferSourceNode playback causes repeated crackling sound
     4        https://bugs.webkit.org/show_bug.cgi?id=222098
     5        <rdar://problem/74546471>
     6
     7        Reviewed by Darin Adler.
     8
     9        The issue was due to certain audio nodes (such as AudioBufferSourceNode) staying in the audio
     10        graph after they were no longer needed. As a result, the audio graph was becoming larger and
     11        larger and the audio performance should degrade due to us traversing this increasingly large
     12        graph.
     13
     14        The audio nodes that had trouble getting removed from the graphs were tail processing nodes.
     15        Those are nodes that may still produce output for a while after they no longer have any
     16        inputs. PannerNode and DelayNode are example of such nodes.
     17
     18        When a Node is no longer useful (the node no longer has any connections, m_connectionRefCount
     19        is 0), we call disableOutputsIfNecessary() on it to disable its outputs and avoid doing any
     20        processing of these outputs since they no longer have an input. disableOutputsIfNecessary()
     21        would normally call disableOutputs() (if not already disabled) but there was an exception
     22        in the case where the node requires tail processing. For such nodes, you wouldn't want to
     23        disable them until they've finished processing their tail.
     24
     25        The issue was that once those nodes had finished processing their tail, we wouldn't come
     26        back to them and disable their outputs later on. As a result, we would process more and
     27        more (silent) nodes and the audio performance would seriously deteriorate.
     28
     29        To address this, we now add the node to the context's list of tail processing nodes in
     30        disableOutputsIfNecessary() if the node requires tail processing. After each rendering
     31        quantum, we go through those tail processing nodes and check if they have finished
     32        processing their tail. If they have, we go ahead and disable their outputs at this point
     33        and remove them from the list.
     34
     35        This is modeled after what Blink is doing for tail processing nodes in:
     36        - https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc
     37
     38        I have verified that on the following demo, the performance no longer deteriorates after
     39        a while:
     40        - https://jsfiddle.net/KrisJohnson/s5vL24o1/123/
     41
     42        * Modules/webaudio/AudioNode.cpp:
     43        (WebCore::AudioNode::enableOutputsIfNecessary):
     44        (WebCore::AudioNode::disableOutputsIfNecessary):
     45        * Modules/webaudio/AudioNode.h:
     46        (WebCore::AudioNode::isTailProcessing const):
     47        (WebCore::AudioNode::setIsTailProcessing):
     48        * Modules/webaudio/BaseAudioContext.cpp:
     49        (WebCore::BaseAudioContext::uninitialize):
     50        (WebCore::BaseAudioContext::handlePostRenderTasks):
     51        (WebCore::BaseAudioContext::addTailProcessingNode):
     52        (WebCore::BaseAudioContext::removeTailProcessingNode):
     53        (WebCore::BaseAudioContext::updateTailProcessingNodes):
     54        (WebCore::BaseAudioContext::disableOutputsForFinishedTailProcessingNodes):
     55        (WebCore::BaseAudioContext::finishTailProcessing):
     56        * Modules/webaudio/BaseAudioContext.h:
     57        (WebCore::BaseAudioContext::TailProcessingNode::TailProcessingNode):
     58        (WebCore::BaseAudioContext::TailProcessingNode::~TailProcessingNode):
     59        (WebCore::BaseAudioContext::TailProcessingNode::operator-> const):
     60        (WebCore::BaseAudioContext::TailProcessingNode::operator== const):
     61
    1622021-05-28  Chris Dumez  <cdumez@apple.com>
    263

  • trunk/Source/WebCore/Modules/webaudio/AudioNode.cpp

    r278185 r278233  
    522522void AudioNode::enableOutputsIfNecessary()
    523523{
     524    Locker locker { context().graphLock() };
     525    if (isTailProcessing())
     526        context().removeTailProcessingNode(*this);
     527
    524528    if (m_isDisabled && m_connectionRefCount > 0) {
    525529        ASSERT(isMainThread());
    526         Locker locker { context().graphLock() };
    527530
    528531        m_isDisabled = false;
    … …  
    546549        // disable() can recursively deref connections (and call disable()) down a whole chain of connected nodes.
    547550
    548         // If a node requires tail processing, we defer the disabling of
    549         // the outputs so that the tail for the node can be output.
     551        // If a node requires tail processing, we defer the disabling of the outputs so that the tail for the node can be output.
    550552        // Otherwise, we can disable the outputs right away.
    551         if (!requiresTailProcessing())
     553        if (requiresTailProcessing())
     554            context().addTailProcessingNode(*this);
     555        else
    552556            disableOutputs();
    553557    }

  • trunk/Source/WebCore/Modules/webaudio/AudioNode.h

    r277530 r278233  
    192192    void setIsFinishedSourceNode() { m_isFinishedSourceNode = true; }
    193193
     194    // Flag indicating the node is in the context's m_tailProcessingNodes or m_finishTailProcessingNodes.
     195    // We rely on this flag to avoid unnecessary linear searches in those vectors.
     196    bool isTailProcessing() const { return m_isTailProcessing; }
     197    void setIsTailProcessing(bool isTailProcessing) { m_isTailProcessing = isTailProcessing; }
     198
    194199protected:
    195200    // Inputs and outputs must be created before the AudioNode is initialized.
    … …  
    254259    bool m_isDisabled { false };
    255260    bool m_isFinishedSourceNode { false };
     261    bool m_isTailProcessing { false };
    256262
    257263#if DEBUG_AUDIONODE_REFERENCES

  • trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.cpp

    r277709 r278233  
    199199    m_isAudioThreadFinished = true;
    200200
     201    finishTailProcessing();
     202
    201203    {
    202204        Locker locker { graphLock() };
    … …  
    604606
    605607    updateAutomaticPullNodes();
     608    updateTailProcessingNodes();
    606609}
    607610
    … …  
    613616   
    614617    m_deferredBreakConnectionList.clear();
     618}
     619
     620void BaseAudioContext::addTailProcessingNode(AudioNode& node)
     621{
     622    ASSERT(isGraphOwner());
     623    if (node.isTailProcessing()) {
     624        ASSERT(m_tailProcessingNodes.contains(node));
     625        return;
     626    }
     627
     628    // Ideally we'd find a way to avoid this vector append since we try to avoid potential heap allocations
     629    // on the audio thread for performance reasons.
     630    DisableMallocRestrictionsForCurrentThreadScope disableMallocRestrictions;
     631    ASSERT(!m_tailProcessingNodes.contains(node));
     632    m_tailProcessingNodes.append(node);
     633}
     634
     635void BaseAudioContext::removeTailProcessingNode(AudioNode& node)
     636{
     637    ASSERT(isGraphOwner());
     638    ASSERT(node.isTailProcessing());
     639
     640    if (m_tailProcessingNodes.removeFirst(node))
     641        return;
     642
     643    // Remove the node from finished tail processing nodes so we don't end up disabling its outputs later on the main thread.
     644    ASSERT(m_finishedTailProcessingNodes.contains(node));
     645    m_finishedTailProcessingNodes.removeFirst(node);
     646}
     647
     648void BaseAudioContext::updateTailProcessingNodes()
     649{
     650    ASSERT(isAudioThread());
     651    ASSERT(isGraphOwner());
     652    // Go backwards as the current node may be removed from m_tailProcessingNodes as we iterate.
     653    // We are on the audio thread so we want to avoid allocations as much as possible.
     654    for (auto i = m_tailProcessingNodes.size(); i > 0; --i) {
     655        auto& node = m_tailProcessingNodes[i - 1];
     656        if (!node->propagatesSilence())
     657            continue; // Node is not done processing its tail.
     658
     659        // Ideally we'd find a way to avoid this vector append since we try to avoid potential heap allocations
     660        // on the audio thread for performance reasons.
     661        DisableMallocRestrictionsForCurrentThreadScope disableMallocRestrictions;
     662
     663        // Disabling of outputs should happen on the main thread we add the node to m_finishedTailProcessingNodes
     664        // for disableOutputsForFinishedTailProcessingNodes() to process later on the main thread.
     665        ASSERT(!m_finishedTailProcessingNodes.contains(node));
     666        m_finishedTailProcessingNodes.append(WTFMove(node));
     667        m_tailProcessingNodes.remove(i - 1);
     668    }
     669
     670    if (m_finishedTailProcessingNodes.isEmpty() || m_disableOutputsForTailProcessingScheduled)
     671        return;
     672
     673    m_disableOutputsForTailProcessingScheduled = true;
     674
     675    // We try to avoid heap allocations on the audio thread but there is no way to do a main thread dispatch
     676    // without one.
     677    DisableMallocRestrictionsForCurrentThreadScope disableMallocRestrictions;
     678    callOnMainThread([this, protectedThis = makeRef(*this)]() mutable {
     679        Locker locker { graphLock() };
     680        disableOutputsForFinishedTailProcessingNodes();
     681        m_disableOutputsForTailProcessingScheduled = false;
     682    });
     683}
     684
     685void BaseAudioContext::disableOutputsForFinishedTailProcessingNodes()
     686{
     687    ASSERT(isMainThread());
     688    ASSERT(isGraphOwner());
     689    for (auto& finishedTailProcessingNode : std::exchange(m_finishedTailProcessingNodes, { }))
     690        finishedTailProcessingNode->disableOutputs();
     691}
     692
     693void BaseAudioContext::finishTailProcessing()
     694{
     695    ASSERT(isMainThread());
     696    Locker locker { graphLock() };
     697
     698    // disableOutputs() can cause new nodes to start tail processing so we need to loop until both vectors are empty.
     699    while (!m_tailProcessingNodes.isEmpty() || !m_finishedTailProcessingNodes.isEmpty()) {
     700        for (auto& tailProcessingNode : std::exchange(m_tailProcessingNodes, { }))
     701            tailProcessingNode->disableOutputs();
     702        disableOutputsForFinishedTailProcessingNodes();
     703    }
    615704}
    616705

  • trunk/Source/WebCore/Modules/webaudio/BaseAudioContext.h

    r278185 r278233  
    161161    void deleteMarkedNodes();
    162162
     163    void addTailProcessingNode(AudioNode&);
     164    void removeTailProcessingNode(AudioNode&);
     165
    163166    // AudioContext can pull node(s) at the end of each render quantum even when they are not connected to any downstream nodes.
    164167    // These two methods are called by the nodes who want to add/remove themselves into/from the automatic pull lists.
    … …  
    286289
    287290    void updateAutomaticPullNodes();
     291    void updateTailProcessingNodes();
     292    void finishTailProcessing();
     293    void disableOutputsForFinishedTailProcessingNodes();
    288294
    289295#if !RELEASE_LOG_DISABLED
    … …  
    306312    // (when handlePostRenderTasks() has completed).
    307313    Vector<AudioNode*> m_nodesMarkedForDeletion;
     314
     315    class TailProcessingNode {
     316    public:
     317        TailProcessingNode(AudioNode& node)
     318            : m_node(&node)
     319        {
     320            ASSERT(!node.isTailProcessing());
     321            node.setIsTailProcessing(true);
     322        }
     323        TailProcessingNode(TailProcessingNode&& other)
     324            : m_node(std::exchange(other.m_node, nullptr))
     325        { }
     326        ~TailProcessingNode()
     327        {
     328            if (m_node)
     329                m_node->setIsTailProcessing(false);
     330        }
     331        TailProcessingNode& operator=(const TailProcessingNode&) = delete;
     332        TailProcessingNode& operator=(TailProcessingNode&&) = delete;
     333        AudioNode* operator->() const { return m_node.get(); }
     334        bool operator==(const TailProcessingNode& other) const { return m_node == other.m_node; }
     335        bool operator==(const AudioNode& node) const { return m_node == &node; }
     336    private:
     337        RefPtr<AudioNode> m_node;
     338    };
     339
     340    // Nodes that are currently processing their tail.
     341    Vector<TailProcessingNode> m_tailProcessingNodes;
     342
     343    // Nodes that have finished processing their tail and waiting for their outputs to get disabled on the main thread.
     344    Vector<TailProcessingNode> m_finishedTailProcessingNodes;
    308345
    309346    // They will be scheduled for deletion (on the main thread) at the end of a render cycle (in realtime thread).
    … …  
    349386    State m_state { State::Suspended };
    350387    bool m_isDeletionScheduled { false };
     388    bool m_disableOutputsForTailProcessingScheduled { false };
    351389    bool m_isStopScheduled { false };
    352390    bool m_isInitialized { false };
Note: See TracChangeset for help on using the changeset viewer.