Changeset 261063 in webkit

Timestamp:

May 3, 2020 11:16:31 AM (4 years ago)

Author:

youenn@apple.com

Message:

AudioMediaStreamTrackRendererCocoa should create/start/stop its remote unit on the main thread
​https://bugs.webkit.org/show_bug.cgi?id=211287

Reviewed by Eric Carlson.

Creating/starting/stopping audio units in different threads is error prone.
Now that we have an observer model where we have observers for when to play in the main thread and
based on that, we decide to receive audio samples in a background thread, we can simplify the logic of AudioMediaStreamTrackRendererCocoa.
We do this by creating/starting the unit in AudioMediaStreamTrackRendererCocoa::start.
At that point, AudioMediaStreamTrackRendererCocoa is not expected to receive any sample.
Just after starting, AudioTrackPrivateMediaStream will receive audio samples and forward them to AudioMediaStreamTrackRendererCocoa.
AudioMediaStreamTrackRendererCocoa will then create in a background thread the AudioSampleDataSource that is responsible to adapt the received audio samples to the unit.

Manually tested.

• platform/audio/mac/AudioSampleDataSource.h:

(WebCore::AudioSampleDataSource::inputDescription const):

• platform/audio/mac/CAAudioStreamDescription.h:
• platform/mediastream/AudioTrackPrivateMediaStream.cpp:

(WebCore::AudioTrackPrivateMediaStream::startRenderer):
Ensure to start the unit and then start gettting audio samples.

• platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp:

(WebCore::AudioMediaStreamTrackRendererCocoa::start):
(WebCore::AudioMediaStreamTrackRendererCocoa::stop):
(WebCore::AudioMediaStreamTrackRendererCocoa::clear):
(WebCore::AudioMediaStreamTrackRendererCocoa::pushSamples):
(WebCore::AudioMediaStreamTrackRendererCocoa::render):

• platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/audio/mac/AudioSampleDataSource.h (modified) (1 diff)
• platform/audio/mac/CAAudioStreamDescription.h (modified) (2 diffs)
• platform/mediastream/AudioMediaStreamTrackRenderer.h (modified) (1 diff)
• platform/mediastream/AudioTrackPrivateMediaStream.cpp (modified) (1 diff)
• platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp (modified) (3 diffs)
• platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-05-03  Youenn Fablet  <youenn@apple.com>
+
+        AudioMediaStreamTrackRendererCocoa should create/start/stop its remote unit on the main thread
+        https://bugs.webkit.org/show_bug.cgi?id=211287
+
+        Reviewed by Eric Carlson.
+
+        Creating/starting/stopping audio units in different threads is error prone.
+        Now that we have an observer model where we have observers for when to play in the main thread and
+        based on that, we decide to receive audio samples in a background thread, we can simplify the logic of AudioMediaStreamTrackRendererCocoa.
+        We do this by creating/starting the unit in AudioMediaStreamTrackRendererCocoa::start.
+        At that point, AudioMediaStreamTrackRendererCocoa is not expected to receive any sample.
+        Just after starting, AudioTrackPrivateMediaStream will receive audio samples and forward them to AudioMediaStreamTrackRendererCocoa.
+        AudioMediaStreamTrackRendererCocoa will then create in a background thread the AudioSampleDataSource that is responsible to adapt the received audio samples to the unit.
+
+        Manually tested.
+
+        * platform/audio/mac/AudioSampleDataSource.h:
+        (WebCore::AudioSampleDataSource::inputDescription const):
+        * platform/audio/mac/CAAudioStreamDescription.h:
+        * platform/mediastream/AudioTrackPrivateMediaStream.cpp:
+        (WebCore::AudioTrackPrivateMediaStream::startRenderer):
+        Ensure to start the unit and then start gettting audio samples.
+        * platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp:
+        (WebCore::AudioMediaStreamTrackRendererCocoa::start):
+        (WebCore::AudioMediaStreamTrackRendererCocoa::stop):
+        (WebCore::AudioMediaStreamTrackRendererCocoa::clear):
+        (WebCore::AudioMediaStreamTrackRendererCocoa::pushSamples):
+        (WebCore::AudioMediaStreamTrackRendererCocoa::render):
+        * platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.h:
+
 2020-05-03  Rob Buis  <rbuis@igalia.com>
 

trunk/Source/WebCore/platform/audio/mac/AudioSampleDataSource.h

@@ -74 +74 @@
     bool muted() const { return m_muted; }
 
+    const CAAudioStreamDescription* inputDescription() const { return m_inputDescription.get(); }
+
 #if !RELEASE_LOG_DISABLED
     const Logger& logger() const final { return m_logger; }

trunk/Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h

@@ -65 +65 @@
     uint32_t formatFlags() const { return m_streamDescription.mFormatFlags; }
 
-    bool operator==(const AudioStreamBasicDescription& other) { return m_streamDescription == other; }
-    bool operator!=(const AudioStreamBasicDescription& other) { return !operator == (other); }
-    bool operator==(const AudioStreamDescription& other)
+    bool operator==(const AudioStreamBasicDescription& other) const { return m_streamDescription == other; }
+    bool operator!=(const AudioStreamBasicDescription& other) const { return !operator == (other); }
+    bool operator==(const AudioStreamDescription& other) const
     {
         if (other.platformDescription().type != PlatformDescription::CAAudioStreamBasicType)
@@ -74 +74 @@
         return operator==(*WTF::get<const AudioStreamBasicDescription*>(other.platformDescription().description));
     }
-    bool operator!=(const AudioStreamDescription& other) { return !operator == (other); }
+    bool operator!=(const AudioStreamDescription& other) const { return !operator == (other); }
 
     const AudioStreamBasicDescription& streamDescription() const { return m_streamDescription; }

trunk/Source/WebCore/platform/mediastream/AudioMediaStreamTrackRenderer.h

@@ -48 +48 @@
     virtual void stop() = 0;
     virtual void clear() = 0;
-    // May be called on a background thread.
+    // May be called on a background thread. It should only be called after start/before stop is called.
     virtual void pushSamples(const WTF::MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) = 0;
 

trunk/Source/WebCore/platform/mediastream/AudioTrackPrivateMediaStream.cpp

@@ -140 +140 @@
 
     m_isPlaying = true;
+    m_renderer->start();
     m_audioSource->addAudioSampleObserver(*this);
-    m_renderer->start();
 }
 

trunk/Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp

@@ -48 +48 @@
     clear();
 
-    m_shouldPlay = true;
+    CAAudioStreamDescription outputDescription;
+    auto remoteIOUnit = createAudioUnit(outputDescription);
+    if (!remoteIOUnit)
+        return;
 
-    if (m_dataSource)
-        m_dataSource->setPaused(false);
+    if (auto error = AudioOutputUnitStart(remoteIOUnit)) {
+        ERROR_LOG(LOGIDENTIFIER, "AudioOutputUnitStart failed, error = ", error, " (", (const char*)&error, ")");
+        AudioComponentInstanceDispose(remoteIOUnit);
+        return;
+    }
+    m_outputDescription = makeUnique<CAAudioStreamDescription>(outputDescription);
+    m_remoteIOUnit = remoteIOUnit;
 }
 
 void AudioMediaStreamTrackRendererCocoa::stop()
 {
-    m_shouldPlay = false;
+    if (!m_remoteIOUnit)
+        return;
 
-    if (m_dataSource)
-        m_dataSource->setPaused(true);
+    AudioOutputUnitStop(m_remoteIOUnit);
+    AudioComponentInstanceDispose(m_remoteIOUnit);
+    m_remoteIOUnit = nullptr;
 }
 
 void AudioMediaStreamTrackRendererCocoa::clear()
 {
-    m_shouldPlay = false;
-
-    if (m_dataSource)
-        m_dataSource->setPaused(true);
-
-    if (m_remoteIOUnit) {
-        AudioOutputUnitStop(m_remoteIOUnit);
-        AudioComponentInstanceDispose(m_remoteIOUnit);
-        m_remoteIOUnit = nullptr;
-    }
+    stop();
 
     m_dataSource = nullptr;
-    m_inputDescription = nullptr;
     m_outputDescription = nullptr;
-    m_isAudioUnitStarted = false;
 }
 
@@ -149 +148 @@
 {
     ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
-    if (!m_shouldPlay) {
-        if (m_isAudioUnitStarted) {
-            if (m_remoteIOUnit)
-                AudioOutputUnitStop(m_remoteIOUnit);
-            m_isAudioUnitStarted = false;
-        }
+    if (!m_remoteIOUnit)
         return;
-    }
 
-    if (!m_inputDescription || *m_inputDescription != description) {
-        m_isAudioUnitStarted = false;
+    if (!m_dataSource || !m_dataSource->inputDescription() || *m_dataSource->inputDescription() != description) {
+        auto dataSource = AudioSampleDataSource::create(description.sampleRate() * 2, *this);
 
-        if (m_remoteIOUnit) {
-            AudioOutputUnitStop(m_remoteIOUnit);
-            AudioComponentInstanceDispose(m_remoteIOUnit);
-            m_remoteIOUnit = nullptr;
-        }
-
-        m_inputDescription = nullptr;
-        m_outputDescription = nullptr;
-
-        CAAudioStreamDescription inputDescription = toCAAudioStreamDescription(description);
-        CAAudioStreamDescription outputDescription;
-
-        auto remoteIOUnit = createAudioUnit(outputDescription);
-        if (!remoteIOUnit)
-            return;
-
-        m_inputDescription = makeUnique<CAAudioStreamDescription>(inputDescription);
-        m_outputDescription = makeUnique<CAAudioStreamDescription>(outputDescription);
-
-        m_dataSource = AudioSampleDataSource::create(description.sampleRate() * 2, *this);
-
-        if (m_dataSource->setInputFormat(inputDescription) || m_dataSource->setOutputFormat(outputDescription)) {
-            AudioComponentInstanceDispose(remoteIOUnit);
+        if (dataSource->setInputFormat(toCAAudioStreamDescription(description))) {
+            ERROR_LOG(LOGIDENTIFIER, "Unable to set the input format of data source");
             return;
         }
 
-        if (auto error = AudioOutputUnitStart(remoteIOUnit)) {
-            ERROR_LOG(LOGIDENTIFIER, "AudioOutputUnitStart failed, error = ", error, " (", (const char*)&error, ")");
-            AudioComponentInstanceDispose(remoteIOUnit);
-            m_inputDescription = nullptr;
+        if (dataSource->setOutputFormat(*m_outputDescription)) {
+            ERROR_LOG(LOGIDENTIFIER, "Unable to set the output format of data source");
             return;
         }
 
-        m_isAudioUnitStarted = true;
+        dataSource->setPaused(false);
+        dataSource->setVolume(volume());
 
-        m_dataSource->setVolume(volume());
-        m_remoteIOUnit = remoteIOUnit;
+        m_dataSource = WTFMove(dataSource);
     }
 
     m_dataSource->pushSamples(sampleTime, audioData, sampleCount);
-
-    if (!m_isAudioUnitStarted) {
-        if (auto error = AudioOutputUnitStart(m_remoteIOUnit)) {
-            ERROR_LOG(LOGIDENTIFIER, "AudioOutputUnitStart failed, error = ", error, " (", (const char*)&error, ")");
-            return;
-        }
-        m_isAudioUnitStarted = true;
-    }
 }
 
+// May get called on a background thread.
 OSStatus AudioMediaStreamTrackRendererCocoa::render(UInt32 sampleCount, AudioBufferList& ioData, UInt32 /*inBusNumber*/, const AudioTimeStamp& timeStamp, AudioUnitRenderActionFlags& actionFlags)
 {
-    if (isMuted() || !m_shouldPlay || !m_dataSource) {
+    auto dataSource = m_dataSource;
+    if (!dataSource) {
         AudioSampleBufferList::zeroABL(ioData, static_cast<size_t>(sampleCount * m_outputDescription->bytesPerFrame()));
         actionFlags = kAudioUnitRenderAction_OutputIsSilence;
@@ -219 +183 @@
     }
 
-    m_dataSource->pullSamples(ioData, static_cast<size_t>(sampleCount), timeStamp.mSampleTime, timeStamp.mHostTime, AudioSampleDataSource::Copy);
-
+    dataSource->pullSamples(ioData, static_cast<size_t>(sampleCount), timeStamp.mSampleTime, timeStamp.mHostTime, AudioSampleDataSource::Copy);
     return 0;
 }

trunk/Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.h

@@ -58 +58 @@
     AudioComponentInstance createAudioUnit(CAAudioStreamDescription&);
 
-    // Audio thread members
     AudioComponentInstance m_remoteIOUnit { nullptr };
-    std::unique_ptr<CAAudioStreamDescription> m_inputDescription;
     std::unique_ptr<CAAudioStreamDescription> m_outputDescription;
 
-    // Cross thread members
+    // Audio threads member
     RefPtr<AudioSampleDataSource> m_dataSource;
-    bool m_isAudioUnitStarted { false };
-    bool m_shouldPlay { false };
 };