Changeset 267094 in webkit


Ignore:
Timestamp:
Sep 15, 2020 11:57:14 AM (4 years ago)
Author:
Chris Dumez
Message:

Improve thread-safety in ScriptProcessorNode implementation
https://bugs.webkit.org/show_bug.cgi?id=216555

Reviewed by Geoffrey Garen.

Improve thread-safety in ScriptProcessorNode implementation:

  • Use a Lock instead of a volatile bool to determine if the main thread is still busy firing the event.
  • Pass the doubleBufferIndex to the main thread by capturing it in the lambda instead of relying on a data member.
  • Modules/webaudio/ScriptProcessorNode.cpp:

(WebCore::ScriptProcessorNode::ScriptProcessorNode):
(WebCore::ScriptProcessorNode::process):
(WebCore::ScriptProcessorNode::fireProcessEvent):

  • Modules/webaudio/ScriptProcessorNode.h:
Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r267090 r267094  
     12020-09-15  Chris Dumez  <cdumez@apple.com>
     2
     3        Improve thread-safety in ScriptProcessorNode implementation
     4        https://bugs.webkit.org/show_bug.cgi?id=216555
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        Improve thread-safety in ScriptProcessorNode implementation:
     9        - Use a Lock instead of a volatile bool to determine if the main thread
     10          is still busy firing the event.
     11        - Pass the doubleBufferIndex to the main thread by capturing it in the
     12          lambda instead of relying on a data member.
     13
     14        * Modules/webaudio/ScriptProcessorNode.cpp:
     15        (WebCore::ScriptProcessorNode::ScriptProcessorNode):
     16        (WebCore::ScriptProcessorNode::process):
     17        (WebCore::ScriptProcessorNode::fireProcessEvent):
     18        * Modules/webaudio/ScriptProcessorNode.h:
     19
    1202020-09-15  Alex Christensen  <achristensen@webkit.org>
    221
  • trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp

    r266417 r267094  
    5555    , ActiveDOMObject(context.scriptExecutionContext())
    5656    , m_doubleBufferIndex(0)
    57     , m_doubleBufferIndexForEvent(0)
    5857    , m_bufferSize(bufferSize)
    5958    , m_bufferReadWriteIndex(0)
    60     , m_isRequestOutstanding(false)
    6159    , m_numberOfInputChannels(numberOfInputChannels)
    6260    , m_numberOfOutputChannels(numberOfOutputChannels)
     
    186184    // When this happens, fire an event and swap buffers.
    187185    if (!m_bufferReadWriteIndex) {
    188         // Avoid building up requests on the main thread to fire process events when they're not being handled.
    189         // This could be a problem if the main thread is very busy doing other things and is being held up handling previous requests.
    190         if (m_isRequestOutstanding) {
    191             // We're late in handling the previous request. The main thread must be very busy.
    192             // The best we can do is clear out the buffer ourself here.
    193             outputBuffer->zero();           
     186        // Reference ourself so we don't accidentally get deleted before fireProcessEvent() gets called.
     187        auto protector = makeRef(*this);
     188
     189        // We only wait for script code execution when the context is an offline one for performance reasons.
     190        if (context().isOfflineContext()) {
     191            BinarySemaphore semaphore;
     192            callOnMainThread([this, &semaphore, doubleBufferIndex = m_doubleBufferIndex] {
     193                fireProcessEvent(doubleBufferIndex);
     194                semaphore.signal();
     195            });
     196            semaphore.wait();
    194197        } else {
    195             // Reference ourself so we don't accidentally get deleted before fireProcessEvent() gets called.
    196             ref();
    197            
    198             // Fire the event on the main thread, not this one (which is the realtime audio thread).
    199             m_doubleBufferIndexForEvent = m_doubleBufferIndex;
    200             m_isRequestOutstanding = true;
    201 
    202             // We only wait for script code execution when the context is an offline one for performance reasons.
    203             if (context().isOfflineContext()) {
    204                 BinarySemaphore semaphore;
    205                 callOnMainThread([this, &semaphore] {
    206                     fireProcessEvent();
    207                     semaphore.signal();
    208                 });
    209                 semaphore.wait();
    210                 // De-reference to match the ref() call in process().
    211                 deref();
    212             } else {
    213                 callOnMainThread([this] {
    214                     fireProcessEvent();
    215                     // De-reference to match the ref() call in process().
    216                     deref();
    217                 });
     198            std::unique_lock<Lock> lock(m_processLock, std::try_to_lock);
     199            if (!lock.owns_lock()) {
     200                // We're late in handling the previous request. The main thread must be
     201                // very busy. The best we can do is clear out the buffer ourself here.
     202                outputBuffer->zero();
     203                return;
    218204            }
     205
     206            callOnMainThread([this, doubleBufferIndex = m_doubleBufferIndex, protector = WTFMove(protector)] {
     207                auto locker = holdLock(m_processLock);
     208                fireProcessEvent(doubleBufferIndex);
     209            });
    219210        }
    220211
     
    223214}
    224215
    225 void ScriptProcessorNode::fireProcessEvent()
    226 {
    227     ASSERT(isMainThread() && m_isRequestOutstanding);
     216void ScriptProcessorNode::fireProcessEvent(unsigned doubleBufferIndex)
     217{
     218    ASSERT(isMainThread());
    228219   
    229     bool isIndexGood = m_doubleBufferIndexForEvent < 2;
     220    bool isIndexGood = doubleBufferIndex < 2;
    230221    ASSERT(isIndexGood);
    231222    if (!isIndexGood)
    232223        return;
    233224       
    234     AudioBuffer* inputBuffer = m_inputBuffers[m_doubleBufferIndexForEvent].get();
    235     AudioBuffer* outputBuffer = m_outputBuffers[m_doubleBufferIndexForEvent].get();
     225    AudioBuffer* inputBuffer = m_inputBuffers[doubleBufferIndex].get();
     226    AudioBuffer* outputBuffer = m_outputBuffers[doubleBufferIndex].get();
    236227    ASSERT(outputBuffer);
    237228    if (!outputBuffer)
     
    240231    // Avoid firing the event if the document has already gone away.
    241232    if (!context().isStopped()) {
    242         // Let the audio thread know we've gotten to the point where it's OK for it to make another request.
    243         m_isRequestOutstanding = false;
    244 
    245233        // Calculate playbackTime with the buffersize which needs to be processed each time when onaudioprocess is called.
    246234        // The outputBuffer being passed to JS will be played after exhausting previous outputBuffer by double-buffering.
  • trunk/Source/WebCore/Modules/webaudio/ScriptProcessorNode.h

    r266417 r267094  
    7575    ScriptProcessorNode(BaseAudioContext&, size_t bufferSize, unsigned numberOfInputChannels, unsigned numberOfOutputChannels);
    7676
    77     void fireProcessEvent();
     77    void fireProcessEvent(unsigned doubleBufferIndex);
    7878
    7979    // Double buffering
     
    8181    void swapBuffers() { m_doubleBufferIndex = 1 - m_doubleBufferIndex; }
    8282    unsigned m_doubleBufferIndex;
    83     unsigned m_doubleBufferIndexForEvent;
    8483    Vector<RefPtr<AudioBuffer>> m_inputBuffers;
    8584    Vector<RefPtr<AudioBuffer>> m_outputBuffers;
     
    8786    size_t m_bufferSize;
    8887    unsigned m_bufferReadWriteIndex;
    89     volatile bool m_isRequestOutstanding;
    9088
    9189    unsigned m_numberOfInputChannels;
     
    9492    RefPtr<AudioBus> m_internalInputBus;
    9593    RefPtr<PendingActivity<ScriptProcessorNode>> m_pendingActivity;
     94    Lock m_processLock;
    9695};
    9796
Note: See TracChangeset for help on using the changeset viewer.