Title: [277921] trunk/Source/WebCore
Revision
277921
Author
cdu...@apple.com
Date
2021-05-22 13:10:40 -0700 (Sat, 22 May 2021)

Log Message

Use CheckedLock in SpeechRecognitionCaptureSourceImpl
https://bugs.webkit.org/show_bug.cgi?id=226131

Reviewed by Darin Adler.

Use CheckedLock in SpeechRecognitionCaptureSourceImpl to benefit from Clang Thread
Safety Analysis. Note that audioSamplesAvailable() does not grab the lock before using
m_dataSource, only when setting the data member. It is unclear why it is safe so I
used WTF_IGNORES_THREAD_SAFETY_ANALYSIS and added a FIXME comment.

* Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp:
(WebCore::SpeechRecognitionCaptureSourceImpl::updateDataSource):
(WebCore::SpeechRecognitionCaptureSourceImpl::pullSamplesAndCallDataCallback):
* Modules/speech/SpeechRecognitionCaptureSourceImpl.h:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (277920 => 277921)


--- trunk/Source/WebCore/ChangeLog	2021-05-22 16:49:42 UTC (rev 277920)
+++ trunk/Source/WebCore/ChangeLog	2021-05-22 20:10:40 UTC (rev 277921)
@@ -1,5 +1,22 @@
 2021-05-22  Chris Dumez  <cdu...@apple.com>
 
+        Use CheckedLock in SpeechRecognitionCaptureSourceImpl
+        https://bugs.webkit.org/show_bug.cgi?id=226131
+
+        Reviewed by Darin Adler.
+
+        Use CheckedLock in SpeechRecognitionCaptureSourceImpl to benefit from Clang Thread
+        Safety Analysis. Note that audioSamplesAvailable() does not grab the lock before using
+        m_dataSource, only when setting the data member. It is unclear why it is safe so I
+        used WTF_IGNORES_THREAD_SAFETY_ANALYSIS and added a FIXME comment.
+
+        * Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp:
+        (WebCore::SpeechRecognitionCaptureSourceImpl::updateDataSource):
+        (WebCore::SpeechRecognitionCaptureSourceImpl::pullSamplesAndCallDataCallback):
+        * Modules/speech/SpeechRecognitionCaptureSourceImpl.h:
+
+2021-05-22  Chris Dumez  <cdu...@apple.com>
+
         Replace LockHolder with Locker in local variables
         https://bugs.webkit.org/show_bug.cgi?id=226133
 

Modified: trunk/Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp (277920 => 277921)


--- trunk/Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp	2021-05-22 16:49:42 UTC (rev 277920)
+++ trunk/Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.cpp	2021-05-22 20:10:40 UTC (rev 277921)
@@ -80,8 +80,55 @@
     m_source->stop();
 }
 
-void SpeechRecognitionCaptureSourceImpl::audioSamplesAvailable(const MediaTime& time, const PlatformAudioData& data, const AudioStreamDescription& description, size_t sampleCount)
+#if PLATFORM(COCOA)
+bool SpeechRecognitionCaptureSourceImpl::updateDataSource(const CAAudioStreamDescription& audioDescription)
 {
+    ASSERT(!isMainThread());
+
+    if (!m_dataSourceLock.tryLock())
+        return false;
+
+    Locker locker { AdoptLockTag { }, m_dataSourceLock };
+
+    auto dataSource = AudioSampleDataSource::create(audioDescription.sampleRate() * 1, m_source.get());
+    if (dataSource->setInputFormat(audioDescription)) {
+        callOnMainThread([this, weakThis = makeWeakPtr(this)] {
+            if (weakThis)
+                m_stateUpdateCallback(SpeechRecognitionUpdate::createError(m_clientIdentifier, SpeechRecognitionError { SpeechRecognitionErrorType::AudioCapture, "Unable to set input format" }));
+        });
+        return false;
+    }
+
+    if (dataSource->setOutputFormat(audioDescription)) {
+        callOnMainThread([this, weakThis = makeWeakPtr(this)] {
+            if (weakThis)
+                m_stateUpdateCallback(SpeechRecognitionUpdate::createError(m_clientIdentifier, SpeechRecognitionError { SpeechRecognitionErrorType::AudioCapture, "Unable to set output format" }));
+        });
+        return false;
+    }
+
+    m_dataSource = WTFMove(dataSource);
+    return true;
+}
+
+void SpeechRecognitionCaptureSourceImpl::pullSamplesAndCallDataCallback(const MediaTime& time, const CAAudioStreamDescription& audioDescription, size_t sampleCount)
+{
+    ASSERT(isMainThread());
+
+    auto data = "" { audioDescription, static_cast<uint32_t>(sampleCount) };
+    {
+        Locker locker { m_dataSourceLock };
+        m_dataSource->pullSamples(*data.list(), sampleCount, time.timeValue(), 0, AudioSampleDataSource::Copy);
+    }
+
+    m_dataCallback(time, data, audioDescription, sampleCount);
+}
+#endif
+
+// FIXME: It is unclear why it is safe to use m_dataSource without locking in this function.
+void SpeechRecognitionCaptureSourceImpl::audioSamplesAvailable(const MediaTime& time, const PlatformAudioData& data, const AudioStreamDescription& description, size_t sampleCount) WTF_IGNORES_THREAD_SAFETY_ANALYSIS
+{
+    ASSERT(!isMainThread());
 #if PLATFORM(COCOA)
     DisableMallocRestrictionsForCurrentThreadScope scope;
 
@@ -88,41 +135,15 @@
     ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
     auto audioDescription = toCAAudioStreamDescription(description);
     if (!m_dataSource || !m_dataSource->inputDescription() || *m_dataSource->inputDescription() != description) {
-        auto dataSource = AudioSampleDataSource::create(description.sampleRate() * 1, m_source.get());
-        if (dataSource->setInputFormat(audioDescription)) {
-            callOnMainThread([this, weakThis = makeWeakPtr(this)] {
-                if (weakThis)
-                    m_stateUpdateCallback(SpeechRecognitionUpdate::createError(m_clientIdentifier, SpeechRecognitionError { SpeechRecognitionErrorType::AudioCapture, "Unable to set input format" }));
-            });
+        if (!updateDataSource(audioDescription))
             return;
-        }
-
-        if (dataSource->setOutputFormat(audioDescription)) {
-            callOnMainThread([this, weakThis = makeWeakPtr(this)] {
-                if (weakThis)
-                    m_stateUpdateCallback(SpeechRecognitionUpdate::createError(m_clientIdentifier, SpeechRecognitionError { SpeechRecognitionErrorType::AudioCapture, "Unable to set output format" }));
-            });
-            return;
-        }
-        
-        if (auto locker = tryHoldLock(m_dataSourceLock))
-            m_dataSource = WTFMove(dataSource);
-        else
-            return;
     }
 
     m_dataSource->pushSamples(time, data, sampleCount);
-    callOnMainThread([this, weakThis = makeWeakPtr(this), time, audioDescription, sampleCount] {
-        if (!weakThis)
-            return;
 
-        auto data = "" { audioDescription, static_cast<uint32_t>(sampleCount) };
-        {
-            Locker locker { m_dataSourceLock };
-            m_dataSource->pullSamples(*data.list(), sampleCount, time.timeValue(), 0, AudioSampleDataSource::Copy);
-        }
-
-        m_dataCallback(time, data, audioDescription, sampleCount);
+    callOnMainThread([weakThis = makeWeakPtr(this), time, audioDescription, sampleCount] {
+        if (weakThis)
+            weakThis->pullSamplesAndCallDataCallback(time, audioDescription, sampleCount);
     });
 #else
     m_dataCallback(time, data, description, sampleCount);

Modified: trunk/Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.h (277920 => 277921)


--- trunk/Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.h	2021-05-22 16:49:42 UTC (rev 277920)
+++ trunk/Source/WebCore/Modules/speech/SpeechRecognitionCaptureSourceImpl.h	2021-05-22 20:10:40 UTC (rev 277921)
@@ -29,6 +29,7 @@
 
 #include "RealtimeMediaSource.h"
 #include "SpeechRecognitionConnectionClientIdentifier.h"
+#include <wtf/CheckedLock.h>
 
 #if PLATFORM(COCOA)
 #include "AudioSampleDataSource.h"
@@ -60,6 +61,11 @@
     // RealtimeMediaSource::AudioSampleObserver
     void audioSamplesAvailable(const MediaTime&, const PlatformAudioData&, const AudioStreamDescription&, size_t) final;
 
+#if PLATFORM(COCOA)
+    bool updateDataSource(const CAAudioStreamDescription&);
+    void pullSamplesAndCallDataCallback(const MediaTime&, const CAAudioStreamDescription&, size_t sampleCount);
+#endif
+
     // RealtimeMediaSource::Observer
     void sourceStarted() final;
     void sourceStopped() final;
@@ -71,8 +77,8 @@
     Ref<RealtimeMediaSource> m_source;
 
 #if PLATFORM(COCOA)
-    RefPtr<AudioSampleDataSource> m_dataSource;
-    Lock m_dataSourceLock;
+    RefPtr<AudioSampleDataSource> m_dataSource WTF_GUARDED_BY_LOCK(m_dataSourceLock); // Only modified on the audio thread but may get used on the main thread.
+    CheckedLock m_dataSourceLock;
 #endif
 };
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to