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

Changeset 278287 in webkit


Ignore:
Timestamp:
May 31, 2021, 5:07:14 PM (5 years ago)
Author:
Chris Dumez
Message:

Fix thread safety issues in AudioBufferSourceNode
​https://bugs.webkit.org/show_bug.cgi?id=226448

Reviewed by Darin Adler.

Adopt thread safety analysis annotations in AudioBufferSourceNode and fix the bugs
that were found by clang:

  • We were failing to grab the processLock when setting the loop / loopStart / loopEnd attributes on the main thread (Those are set by JS at any time).
  • propagatesSilence() was failing to grab the lock when checking m_buffer on the audio thread.
  • Modules/webaudio/AudioBufferSourceNode.cpp:

(WebCore::AudioBufferSourceNode::process):
(WebCore::AudioBufferSourceNode::renderSilenceAndFinishIfNotLooping):
(WebCore::AudioBufferSourceNode::renderFromBuffer):
(WebCore::AudioBufferSourceNode::setBuffer):
(WebCore::AudioBufferSourceNode::setLoop):
(WebCore::AudioBufferSourceNode::setLoopStart):
(WebCore::AudioBufferSourceNode::setLoopEnd):
(WebCore::AudioBufferSourceNode::adjustGrainParameters):
(WebCore::AudioBufferSourceNode::totalPitchRate):
(WebCore::AudioBufferSourceNode::propagatesSilence const):

  • Modules/webaudio/AudioBufferSourceNode.h:
Location:
trunk/Source/WebCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r278286 r278287  
     12021-05-31  Chris Dumez  <cdumez@apple.com>
     2
     3        Fix thread safety issues in AudioBufferSourceNode
     4        https://bugs.webkit.org/show_bug.cgi?id=226448
     5
     6        Reviewed by Darin Adler.
     7
     8        Adopt thread safety analysis annotations in AudioBufferSourceNode and fix the bugs
     9        that were found by clang:
     10        - We were failing to grab the processLock when setting the loop / loopStart / loopEnd attributes
     11          on the main thread (Those are set by JS at any time).
     12        - propagatesSilence() was failing to grab the lock when checking m_buffer on the audio thread.
     13
     14        * Modules/webaudio/AudioBufferSourceNode.cpp:
     15        (WebCore::AudioBufferSourceNode::process):
     16        (WebCore::AudioBufferSourceNode::renderSilenceAndFinishIfNotLooping):
     17        (WebCore::AudioBufferSourceNode::renderFromBuffer):
     18        (WebCore::AudioBufferSourceNode::setBuffer):
     19        (WebCore::AudioBufferSourceNode::setLoop):
     20        (WebCore::AudioBufferSourceNode::setLoopStart):
     21        (WebCore::AudioBufferSourceNode::setLoopEnd):
     22        (WebCore::AudioBufferSourceNode::adjustGrainParameters):
     23        (WebCore::AudioBufferSourceNode::totalPitchRate):
     24        (WebCore::AudioBufferSourceNode::propagatesSilence const):
     25        * Modules/webaudio/AudioBufferSourceNode.h:
     26
    1272021-05-31  Chris Dumez  <cdumez@apple.com>
    228

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

    r278253 r278287  
    6767    auto node = adoptRef(*new AudioBufferSourceNode(context));
    6868
    69     node->setBuffer(WTFMove(options.buffer));
     69    node->setBufferForBindings(WTFMove(options.buffer));
    7070    node->detune().setValue(options.detune);
    71     node->setLoop(options.loop);
    72     node->setLoopEnd(options.loopEnd);
    73     node->setLoopStart(options.loopStart);
     71    node->setLoopForBindings(options.loop);
     72    node->setLoopEndForBindings(options.loopEnd);
     73    node->setLoopStartForBindings(options.loopStart);
    7474    node->playbackRate().setValue(options.playbackRate);
    7575
    … …  
    111111    Locker locker { AdoptLock, m_processLock };
    112112
    113     if (!buffer()) {
     113    if (!m_buffer) {
    114114        outputBus.zero();
    115115        return;
    … …  
    119119    // before the output bus is updated to the new number of channels because of use of tryLocks() in the context's updating system.
    120120    // In this case, if the buffer has just been changed and we're not quite ready yet, then just output silence.
    121     if (numberOfChannels() != buffer()->numberOfChannels()) {
     121    if (numberOfChannels() != m_buffer->numberOfChannels()) {
    122122        outputBus.zero();
    123123        return;
    … …  
    149149bool AudioBufferSourceNode::renderSilenceAndFinishIfNotLooping(AudioBus*, unsigned index, size_t framesToProcess)
    150150{
    151     if (!loop()) {
     151    if (!m_isLooping) {
    152152        // If we're not looping, then stop playing when we get to the end.
    153153
    … …  
    172172    // Basic sanity checking
    173173    ASSERT(bus);
    174     ASSERT(buffer());
    175     if (!bus || !buffer())
     174    ASSERT(m_buffer);
     175    if (!bus || !m_buffer)
    176176        return false;
    177177
    … …  
    206206    unsigned writeIndex = destinationFrameOffset;
    207207
    208     size_t bufferLength = buffer()->length();
    209     double bufferSampleRate = buffer()->sampleRate();
     208    size_t bufferLength = m_buffer->length();
     209    double bufferSampleRate = m_buffer->sampleRate();
    210210    double pitchRate = totalPitchRate();
    211211    bool reverse = pitchRate < 0;
    … …  
    229229    double virtualDeltaFrames = maxFrame;
    230230
    231     if (loop() && (m_loopStart || m_loopEnd) && m_loopStart >= 0 && m_loopEnd > 0 && m_loopStart < m_loopEnd) {
     231    if (m_isLooping && (m_loopStart || m_loopEnd) && m_loopStart >= 0 && m_loopEnd > 0 && m_loopStart < m_loopEnd) {
    232232        // Convert from seconds to sample-frames.
    233         double loopMinFrame = m_loopStart * buffer()->sampleRate();
    234         double loopMaxFrame = m_loopEnd * buffer()->sampleRate();
     233        double loopMinFrame = m_loopStart * m_buffer->sampleRate();
     234        double loopMaxFrame = m_loopEnd * m_buffer->sampleRate();
    235235
    236236        virtualMaxFrame = std::min(loopMaxFrame, virtualMaxFrame);
    … …  
    241241    // If we're looping and the offset (virtualReadIndex) is past the end of the loop, wrap back to the
    242242    // beginning of the loop. For other cases, nothing needs to be done.
    243     if (loop() && m_virtualReadIndex >= virtualMaxFrame) {
    244         m_virtualReadIndex = (m_loopStart < 0) ? 0 : (m_loopStart * buffer()->sampleRate());
     243    if (m_isLooping && m_virtualReadIndex >= virtualMaxFrame) {
     244        m_virtualReadIndex = (m_loopStart < 0) ? 0 : (m_loopStart * m_buffer->sampleRate());
    245245        m_virtualReadIndex = std::min(m_virtualReadIndex, static_cast<double>(bufferLength - 1));
    246246    }
    … …  
    340340            unsigned readIndex2 = readIndex + 1;
    341341            if (readIndex2 >= maxFrame)
    342                 readIndex2 = loop() ? minFrame : readIndex;
     342                readIndex2 = m_isLooping ? minFrame : readIndex;
    343343
    344344            // Linear interpolation.
    … …  
    369369            unsigned readIndex2 = readIndex + 1;
    370370            if (readIndex2 >= bufferLength) {
    371                 if (loop()) {
     371                if (m_isLooping) {
    372372                    // Make sure to wrap around at the end of the buffer.
    373373                    readIndex2 = static_cast<unsigned>(virtualReadIndex + 1 - virtualDeltaFrames);
    … …  
    408408}
    409409
    410 ExceptionOr<void> AudioBufferSourceNode::setBuffer(RefPtr<AudioBuffer>&& buffer)
     410ExceptionOr<void> AudioBufferSourceNode::setBufferForBindings(RefPtr<AudioBuffer>&& buffer)
    411411{
    412412    ASSERT(isMainThread());
    413413    DEBUG_LOG(LOGIDENTIFIER);
    414 
    415     if (buffer && m_wasBufferSet)
    416         return Exception { InvalidStateError, "The buffer was already set"_s };
    417414
    418415    // The context must be locked since changing the buffer can re-configure the number of channels that are output.
    … …  
    421418    // This synchronizes with process().
    422419    Locker locker { m_processLock };
     420
     421    if (buffer && m_wasBufferSet)
     422        return Exception { InvalidStateError, "The buffer was already set"_s };
    423423   
    424424    if (buffer) {
    … …  
    456456{
    457457    return startPlaying(when, grainOffset, grainDuration);
     458}
     459
     460void AudioBufferSourceNode::setLoopForBindings(bool looping)
     461{
     462    ASSERT(isMainThread());
     463    Locker locker { m_processLock };
     464    m_isLooping = looping;
     465}
     466
     467void AudioBufferSourceNode::setLoopStartForBindings(double loopStart)
     468{
     469    ASSERT(isMainThread());
     470    Locker locker { m_processLock };
     471    m_loopStart = loopStart;
     472}
     473
     474void AudioBufferSourceNode::setLoopEndForBindings(double loopEnd)
     475{
     476    ASSERT(isMainThread());
     477    Locker locker { m_processLock };
     478    m_loopEnd = loopEnd;
    458479}
    459480
    … …  
    497518    ASSERT(m_processLock.isHeld());
    498519
    499     auto buffer = this->buffer();
    500     if (!buffer)
     520    if (!m_buffer)
    501521        return;
    502522
    503523    // Do sanity checking of grain parameters versus buffer size.
    504     double bufferDuration = buffer->duration();
     524    double bufferDuration = m_buffer->duration();
    505525
    506526    m_grainOffset = std::min(bufferDuration, m_grainOffset);
    … …  
    509529        m_grainDuration = bufferDuration - m_grainOffset;
    510530
    511     if (m_wasGrainDurationGiven && loop()) {
     531    if (m_wasGrainDurationGiven && m_isLooping) {
    512532        // We're looping a grain with a grain duration specified. Schedule the loop
    513533        // to stop after grainDuration seconds after starting, possibly running the
    … …  
    524544    // Since playbackRate == 1 is very common, it's worth considering quality.
    525545    if (playbackRate().value() < 0)
    526         m_virtualReadIndex = AudioUtilities::timeToSampleFrame(m_grainOffset + m_grainDuration, buffer->sampleRate()) - 1;
     546        m_virtualReadIndex = AudioUtilities::timeToSampleFrame(m_grainOffset + m_grainDuration, m_buffer->sampleRate()) - 1;
    527547    else
    528         m_virtualReadIndex = AudioUtilities::timeToSampleFrame(m_grainOffset, buffer->sampleRate());
     548        m_virtualReadIndex = AudioUtilities::timeToSampleFrame(m_grainOffset, m_buffer->sampleRate());
    529549}
    530550
    … …  
    534554    // Normally it's not an issue because buffers are loaded at the AudioContext's sample-rate, but we can handle it in any case.
    535555    double sampleRateFactor = 1.0;
    536     if (buffer())
    537         sampleRateFactor = buffer()->sampleRate() / static_cast<double>(sampleRate());
     556    if (m_buffer)
     557        sampleRateFactor = m_buffer->sampleRate() / static_cast<double>(sampleRate());
    538558   
    539559    double basePitchRate = playbackRate().finalValue();
    … …  
    554574bool AudioBufferSourceNode::propagatesSilence() const
    555575{
    556     return !isPlayingOrScheduled() || hasFinished() || !m_buffer;
     576    ASSERT(context().isAudioThread());
     577    if (!isPlayingOrScheduled() || hasFinished())
     578        return true;
     579
     580    if (!m_processLock.tryLock()) {
     581        // We weren't able to get the lock so we assume we have a buffer.
     582        return false;
     583    }
     584    Locker locker { AdoptLock, m_processLock };
     585    return !m_buffer;
    557586}
    558587

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

    r278253 r278287  
    5050    void process(size_t framesToProcess) final;
    5151
    52     // setBuffer() is called on the main thread.  This is the buffer we use for playback.
    53     ExceptionOr<void> setBuffer(RefPtr<AudioBuffer>&&);
    54     AudioBuffer* buffer() { return m_buffer.get(); }
     52    // setBufferForBindings() is called on the main thread. This is the buffer we use for playback.
     53    ExceptionOr<void> setBufferForBindings(RefPtr<AudioBuffer>&&);
     54
     55    // This function does not lock before accessing the buffer and should therefore only be called on the main thread.
     56    AudioBuffer* bufferForBindings() WTF_IGNORES_THREAD_SAFETY_ANALYSIS { ASSERT(isMainThread()); return m_buffer.get(); }
    5557
    5658    // numberOfChannels() returns the number of output channels.  This value equals the number of channels from the buffer.
    … …  
    6163    ExceptionOr<void> startLater(double when, double grainOffset, std::optional<double> grainDuration);
    6264
    63     // Note: the attribute was originally exposed as .looping, but to be more consistent in naming with <audio>
    64     // and with how it's described in the specification, the proper attribute name is .loop
    65     // The old attribute is kept for backwards compatibility.
    66     bool loop() const { return m_isLooping; }
    67     void setLoop(bool looping) { m_isLooping = looping; }
     65    // This function doesn't grab the lock before accessing m_isLooping and is thus only safe to call on the main thread.
     66    bool loopForBindings() const WTF_IGNORES_THREAD_SAFETY_ANALYSIS { ASSERT(isMainThread()); return m_isLooping; }
     67    void setLoopForBindings(bool);
    6868
    6969    // Loop times in seconds.
    70     double loopStart() const { return m_loopStart; }
    71     double loopEnd() const { return m_loopEnd; }
    72     void setLoopStart(double loopStart) { m_loopStart = loopStart; }
    73     void setLoopEnd(double loopEnd) { m_loopEnd = loopEnd; }
     70    double loopStartForBindings() const WTF_IGNORES_THREAD_SAFETY_ANALYSIS { ASSERT(isMainThread()); return m_loopStart; } // This function doesn't grab the lock and is thus only safe to call on the main thread.
     71    double loopEndForBindings() const WTF_IGNORES_THREAD_SAFETY_ANALYSIS { ASSERT(isMainThread()); return m_loopEnd; } // This function doesn't grab the lock and is thus only safe to call on the main thread.
     72    void setLoopStartForBindings(double);
     73    void setLoopEndForBindings(double);
    7474
    7575    AudioParam& detune() { return m_detune.get(); }
    … …  
    8989
    9090    ExceptionOr<void> startPlaying(double when, double grainOffset, std::optional<double> grainDuration);
    91     void adjustGrainParameters();
     91    void adjustGrainParameters() WTF_REQUIRES_LOCK(m_processLock);
    9292
    9393    // Returns true on success.
    94     bool renderFromBuffer(AudioBus*, unsigned destinationFrameOffset, size_t numberOfFrames, double startFrameOffset);
     94    bool renderFromBuffer(AudioBus*, unsigned destinationFrameOffset, size_t numberOfFrames, double startFrameOffset) WTF_REQUIRES_LOCK(m_processLock);
    9595
    9696    // Render silence starting from "index" frame in AudioBus.
    97     inline bool renderSilenceAndFinishIfNotLooping(AudioBus*, unsigned index, size_t framesToProcess);
     97    inline bool renderSilenceAndFinishIfNotLooping(AudioBus*, unsigned index, size_t framesToProcess) WTF_REQUIRES_LOCK(m_processLock);
    9898
    9999    // m_buffer holds the sample data which this node outputs.
    100     RefPtr<AudioBuffer> m_buffer;
     100    RefPtr<AudioBuffer> m_buffer WTF_GUARDED_BY_LOCK(m_processLock); // Only modified on the main thread but used on the audio thread.
    101101
    102102    // Pointers for the buffer and destination.
    … …  
    109109    // If m_isLooping is false, then this node will be done playing and become inactive after it reaches the end of the sample data in the buffer.
    110110    // If true, it will wrap around to the start of the buffer each time it reaches the end.
    111     bool m_isLooping { false };
     111    bool m_isLooping WTF_GUARDED_BY_LOCK(m_processLock) { false }; // Only modified on the main thread but queried on the audio thread.
    112112
    113113    bool m_wasBufferSet { false };
    114114
    115     double m_loopStart { 0 };
    116     double m_loopEnd { 0 };
     115    double m_loopStart WTF_GUARDED_BY_LOCK(m_processLock) { 0 }; // Only modified on the main thread but queried on the audio thread.
     116    double m_loopEnd WTF_GUARDED_BY_LOCK(m_processLock) { 0 }; // Only modified on the main thread but queried on the audio thread.
    117117
    118118    // m_virtualReadIndex is a sample-frame index into our buffer representing the current playback position.
    … …  
    121121
    122122    // Granular playback
    123     bool m_isGrain { false };
    124     double m_grainOffset { 0 }; // in seconds
    125     double m_grainDuration; // in seconds
    126     double m_wasGrainDurationGiven { false };
     123    bool m_isGrain WTF_GUARDED_BY_LOCK(m_processLock) { false };
     124    double m_grainOffset WTF_GUARDED_BY_LOCK(m_processLock) { 0 }; // in seconds
     125    double m_grainDuration WTF_GUARDED_BY_LOCK(m_processLock); // in seconds
     126    double m_wasGrainDurationGiven WTF_GUARDED_BY_LOCK(m_processLock) { false };
    127127
    128128    // totalPitchRate() returns the instantaneous pitch rate (non-time preserving).
    129129    // It incorporates the base pitch rate, any sample-rate conversion factor from the buffer.
    130     double totalPitchRate();
     130    double totalPitchRate() WTF_REQUIRES_LOCK(m_processLock);
    131131
    132132    // This synchronizes process() with setBuffer() which can cause dynamic channel count changes.

  • trunk/Source/WebCore/Modules/webaudio/AudioBufferSourceNode.idl

    r277530 r278287  
    3232    [EnabledBySetting=WebAudio] constructor (BaseAudioContext context, optional AudioBufferSourceOptions options);
    3333
    34     attribute AudioBuffer? buffer;
     34    [ImplementedAs=bufferForBindings] attribute AudioBuffer? buffer;
    3535
    3636    readonly attribute AudioParam playbackRate;
    3737    readonly attribute AudioParam detune;
    3838
    39     attribute boolean loop;
    40     attribute double loopStart;
    41     attribute double loopEnd;
     39    [ImplementedAs=loopForBindings] attribute boolean loop;
     40    [ImplementedAs=loopStartForBindings] attribute double loopStart;
     41    [ImplementedAs=loopEndForBindings] attribute double loopEnd;
    4242
    4343    [ImplementedAs=startLater] undefined start(optional double when = 0, optional double grainOffset = 0, optional double grainDuration);
Note: See TracChangeset for help on using the changeset viewer.