Title: [283611] trunk/Source/WebCore
- Revision
- 283611
- Author
- [email protected]
- Date
- 2021-10-06 03:14:38 -0700 (Wed, 06 Oct 2021)
Log Message
Simplify BaseAudioSharedUnit client management
https://bugs.webkit.org/show_bug.cgi?id=231230
<rdar://83884131>
Reviewed by Eric Carlson.
Before the patch, m_clientsLock was used to handle BaseAudioSharedUnit concurrent access from main thread and audio thread.
The potential issue is that if the audio unit is running and m_clients is locked in main thread due to a call of forEach,
the audio thread might have to wait for the lock to be removed.
This is not great as this might cause glitches and potential deadlocks if trying to stop the audio unit in main thread inside a forEach call.
Instead of doing that, we now exclusively access m_clients from main thread, it does not require a lock anymore.
The audio thread accesses a copy of m_clients, called m_audioThreadClients, which is only accessed in addClient/removeClient and from the audio thread.
This reduces the possibility for stopping the audio thread and removes any potential deadlock.
Covered by existing tests.
* platform/mediastream/mac/BaseAudioSharedUnit.cpp:
* platform/mediastream/mac/BaseAudioSharedUnit.h:
* platform/mediastream/mac/CoreAudioCaptureSource.cpp:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (283610 => 283611)
--- trunk/Source/WebCore/ChangeLog 2021-10-06 10:13:15 UTC (rev 283610)
+++ trunk/Source/WebCore/ChangeLog 2021-10-06 10:14:38 UTC (rev 283611)
@@ -1,3 +1,26 @@
+2021-10-06 Youenn Fablet <[email protected]>
+
+ Simplify BaseAudioSharedUnit client management
+ https://bugs.webkit.org/show_bug.cgi?id=231230
+ <rdar://83884131>
+
+ Reviewed by Eric Carlson.
+
+ Before the patch, m_clientsLock was used to handle BaseAudioSharedUnit concurrent access from main thread and audio thread.
+ The potential issue is that if the audio unit is running and m_clients is locked in main thread due to a call of forEach,
+ the audio thread might have to wait for the lock to be removed.
+ This is not great as this might cause glitches and potential deadlocks if trying to stop the audio unit in main thread inside a forEach call.
+
+ Instead of doing that, we now exclusively access m_clients from main thread, it does not require a lock anymore.
+ The audio thread accesses a copy of m_clients, called m_audioThreadClients, which is only accessed in addClient/removeClient and from the audio thread.
+ This reduces the possibility for stopping the audio thread and removes any potential deadlock.
+
+ Covered by existing tests.
+
+ * platform/mediastream/mac/BaseAudioSharedUnit.cpp:
+ * platform/mediastream/mac/BaseAudioSharedUnit.h:
+ * platform/mediastream/mac/CoreAudioCaptureSource.cpp:
+
2021-10-06 Alicia Boya García <[email protected]>
[MSE][GStreamer] Honor MP4 edit lists
Modified: trunk/Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.cpp (283610 => 283611)
--- trunk/Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.cpp 2021-10-06 10:13:15 UTC (rev 283610)
+++ trunk/Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.cpp 2021-10-06 10:14:38 UTC (rev 283611)
@@ -45,26 +45,31 @@
void BaseAudioSharedUnit::addClient(CoreAudioCaptureSource& client)
{
ASSERT(isMainThread());
- Locker locker { m_clientsLock };
m_clients.add(&client);
+ Locker locker { m_audioThreadClientsLock };
+ m_audioThreadClients = copyToVector(m_clients);
}
void BaseAudioSharedUnit::removeClient(CoreAudioCaptureSource& client)
{
ASSERT(isMainThread());
- Locker locker { m_clientsLock };
m_clients.remove(&client);
+ Locker locker { m_audioThreadClientsLock };
+ m_audioThreadClients = copyToVector(m_clients);
}
+void BaseAudioSharedUnit::clearClients()
+{
+ ASSERT(isMainThread());
+ m_clients.clear();
+ Locker locker { m_audioThreadClientsLock };
+ m_audioThreadClients.clear();
+}
+
void BaseAudioSharedUnit::forEachClient(const Function<void(CoreAudioCaptureSource&)>& apply) const
{
- Vector<CoreAudioCaptureSource*> clientsCopy;
- {
- Locker locker { m_clientsLock };
- clientsCopy = copyToVector(m_clients);
- }
- for (auto* client : clientsCopy) {
- Locker locker { m_clientsLock };
+ ASSERT(isMainThread());
+ for (auto* client : copyToVector(m_clients)) {
// Make sure the client has not been destroyed.
if (!m_clients.contains(client))
continue;
@@ -72,12 +77,6 @@
}
}
-void BaseAudioSharedUnit::clearClients()
-{
- Locker locker { m_clientsLock };
- m_clients.clear();
-}
-
void BaseAudioSharedUnit::startProducingData()
{
ASSERT(isMainThread());
@@ -217,12 +216,12 @@
void BaseAudioSharedUnit::audioSamplesAvailable(const MediaTime& time, const PlatformAudioData& data, const AudioStreamDescription& description, size_t numberOfFrames)
{
// We hold the lock here since adding/removing clients can only happen in main thread.
- Locker locker { m_clientsLock };
+ Locker locker { m_audioThreadClientsLock };
// For performance reasons, we forbid heap allocations while doing rendering on the capture audio thread.
ForbidMallocUseForCurrentThreadScope forbidMallocUse;
- for (auto* client : m_clients) {
+ for (auto* client : m_audioThreadClients) {
if (client->isProducingData())
client->audioSamplesAvailable(time, data, description, numberOfFrames);
}
Modified: trunk/Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.h (283610 => 283611)
--- trunk/Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.h 2021-10-06 10:13:15 UTC (rev 283610)
+++ trunk/Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.h 2021-10-06 10:14:38 UTC (rev 283611)
@@ -30,8 +30,8 @@
#include "RealtimeMediaSourceCapabilities.h"
#include <wtf/Function.h>
#include <wtf/HashSet.h>
+#include <wtf/Lock.h>
#include <wtf/MediaTime.h>
-#include <wtf/RecursiveLockAdapter.h>
#include <wtf/text/WTFString.h>
namespace WebCore {
@@ -103,7 +103,8 @@
int32_t m_producingCount { 0 };
HashSet<CoreAudioCaptureSource*> m_clients;
- mutable RecursiveLock m_clientsLock;
+ Vector<CoreAudioCaptureSource*> m_audioThreadClients WTF_GUARDED_BY_LOCK(m_audioThreadClientsLock);
+ Lock m_audioThreadClientsLock;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp (283610 => 283611)
--- trunk/Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp 2021-10-06 10:13:15 UTC (rev 283610)
+++ trunk/Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp 2021-10-06 10:14:38 UTC (rev 283611)
@@ -188,6 +188,7 @@
if (WTF::anyOf(devices, [this] (auto& device) { return m_persistentID == device.persistentId(); }))
return;
+ RELEASE_LOG_ERROR(WebRTC, "CoreAudioSharedUnit::devicesChanged - failing capture");
captureFailed();
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
