AudioMediaStreamTrackRendererCocoa should create/start/stop its remote unit on the...
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 3 May 2020 18:16:31 +0000 (18:16 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 3 May 2020 18:16:31 +0000 (18:16 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@261063 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/audio/mac/AudioSampleDataSource.h
Source/WebCore/platform/audio/mac/CAAudioStreamDescription.h
Source/WebCore/platform/mediastream/AudioMediaStreamTrackRenderer.h
Source/WebCore/platform/mediastream/AudioTrackPrivateMediaStream.cpp
Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.cpp
Source/WebCore/platform/mediastream/mac/AudioMediaStreamTrackRendererCocoa.h

index dba792d..7e0860d 100644 (file)
@@ -1,3 +1,34 @@
+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>
 
         atob() should not accept a vertical tab
index 326c9bc..0f89ef9 100644 (file)
@@ -73,6 +73,8 @@ public:
     void setMuted(bool muted) { m_muted = muted; }
     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; }
     const void* logIdentifier() const final { return m_logIdentifier; }
index 7782bdd..7a73929 100644 (file)
@@ -64,16 +64,16 @@ public:
     uint32_t bytesPerPacket() const { return m_streamDescription.mBytesPerPacket; }
     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)
             return false;
 
         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; }
     AudioStreamBasicDescription& streamDescription() { return m_streamDescription; }
index 1462e32..070e9f1 100644 (file)
@@ -47,7 +47,7 @@ public:
     virtual void start() = 0;
     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;
 
     virtual void setMuted(bool);
index 0355140..7b4f552 100644 (file)
@@ -139,8 +139,8 @@ void AudioTrackPrivateMediaStream::startRenderer()
         return;
 
     m_isPlaying = true;
-    m_audioSource->addAudioSampleObserver(*this);
     m_renderer->start();
+    m_audioSource->addAudioSampleObserver(*this);
 }
 
 void AudioTrackPrivateMediaStream::stopRenderer()
index 19dc4a9..41c5a76 100644 (file)
@@ -47,37 +47,36 @@ void AudioMediaStreamTrackRendererCocoa::start()
 {
     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;
 }
 
 AudioComponentInstance AudioMediaStreamTrackRendererCocoa::createAudioUnit(CAAudioStreamDescription& outputDescription)
@@ -148,78 +147,42 @@ AudioComponentInstance AudioMediaStreamTrackRendererCocoa::createAudioUnit(CAAud
 void AudioMediaStreamTrackRendererCocoa::pushSamples(const MediaTime& sampleTime, const PlatformAudioData& audioData, const AudioStreamDescription& description, size_t sampleCount)
 {
     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_remoteIOUnit) {
-            AudioOutputUnitStop(m_remoteIOUnit);
-            AudioComponentInstanceDispose(m_remoteIOUnit);
-            m_remoteIOUnit = nullptr;
-        }
 
-        m_inputDescription = nullptr;
-        m_outputDescription = nullptr;
+    if (!m_dataSource || !m_dataSource->inputDescription() || *m_dataSource->inputDescription() != description) {
+        auto dataSource = AudioSampleDataSource::create(description.sampleRate() * 2, *this);
 
-        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;
         return 0;
     }
 
-    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;
 }
 
index 154dfa9..27cfcab 100644 (file)
@@ -57,15 +57,11 @@ private:
 
     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 };
 };
 
 }