Title: [285024] trunk/Source/WebCore
- Revision
- 285024
- Author
- [email protected]
- Date
- 2021-10-29 03:59:34 -0700 (Fri, 29 Oct 2021)
Log Message
Fix potential race in AudioMediaStreamTrackRendererCocoa::pushSamples
https://bugs.webkit.org/show_bug.cgi?id=232430
Reviewed by Eric Carlson.
In case we start a AudioMediaStreamTrackRendererCocoa, and we quickly stop it, there may be a time
where we create an AudioSampleDataSource from a background thread, then hop to main thread.
We would then register it to the rendering unit even if the AudioMediaStreamTrackRendererCocoa is cleared.
The AudioSampleDataSource would then be kept registered with no data which would be wasteful in terms of CPU.
To fix this, we keep m_dataSource but introduce a m_registeredDataSource.
m_registeredDataSource is only manipulated in main thread and is the one that is registered/unregistered to the rendering unit.
m_dataSource is only accessed from background thread.
Manually tested and covered by added assertion in AudioMediaStreamTrackRendererCocoa destructor.
* platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:
(WebCore::AudioMediaStreamTrackRendererCocoa::~AudioMediaStreamTrackRendererCocoa):
(WebCore::AudioMediaStreamTrackRendererCocoa::clear):
(WebCore::AudioMediaStreamTrackRendererCocoa::setRegisteredDataSource):
(WebCore::AudioMediaStreamTrackRendererCocoa::pushSamples):
(WebCore::pollSamplesCount): Deleted.
* platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (285023 => 285024)
--- trunk/Source/WebCore/ChangeLog 2021-10-29 09:50:10 UTC (rev 285023)
+++ trunk/Source/WebCore/ChangeLog 2021-10-29 10:59:34 UTC (rev 285024)
@@ -1,5 +1,30 @@
2021-10-29 Youenn Fablet <[email protected]>
+ Fix potential race in AudioMediaStreamTrackRendererCocoa::pushSamples
+ https://bugs.webkit.org/show_bug.cgi?id=232430
+
+ Reviewed by Eric Carlson.
+
+ In case we start a AudioMediaStreamTrackRendererCocoa, and we quickly stop it, there may be a time
+ where we create an AudioSampleDataSource from a background thread, then hop to main thread.
+ We would then register it to the rendering unit even if the AudioMediaStreamTrackRendererCocoa is cleared.
+ The AudioSampleDataSource would then be kept registered with no data which would be wasteful in terms of CPU.
+ To fix this, we keep m_dataSource but introduce a m_registeredDataSource.
+ m_registeredDataSource is only manipulated in main thread and is the one that is registered/unregistered to the rendering unit.
+ m_dataSource is only accessed from background thread.
+
+ Manually tested and covered by added assertion in AudioMediaStreamTrackRendererCocoa destructor.
+
+ * platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:
+ (WebCore::AudioMediaStreamTrackRendererCocoa::~AudioMediaStreamTrackRendererCocoa):
+ (WebCore::AudioMediaStreamTrackRendererCocoa::clear):
+ (WebCore::AudioMediaStreamTrackRendererCocoa::setRegisteredDataSource):
+ (WebCore::AudioMediaStreamTrackRendererCocoa::pushSamples):
+ (WebCore::pollSamplesCount): Deleted.
+ * platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h:
+
+2021-10-29 Youenn Fablet <[email protected]>
+
LocalAudioMediaStreamTrackRendererInternalUnit should clip audio data
https://bugs.webkit.org/show_bug.cgi?id=232428
Modified: trunk/Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp (285023 => 285024)
--- trunk/Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp 2021-10-29 09:50:10 UTC (rev 285023)
+++ trunk/Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp 2021-10-29 10:59:34 UTC (rev 285024)
@@ -41,7 +41,10 @@
{
}
-AudioMediaStreamTrackRendererCocoa::~AudioMediaStreamTrackRendererCocoa() = default;
+AudioMediaStreamTrackRendererCocoa::~AudioMediaStreamTrackRendererCocoa()
+{
+ ASSERT(!m_registeredDataSource);
+}
void AudioMediaStreamTrackRendererCocoa::start(CompletionHandler<void()>&& callback)
{
@@ -48,8 +51,10 @@
clear();
AudioMediaStreamTrackRendererUnit::singleton().retrieveFormatDescription([weakThis = WeakPtr { *this }, callback = WTFMove(callback)](auto* formatDescription) mutable {
- if (weakThis && formatDescription)
+ if (weakThis && formatDescription) {
weakThis->m_outputDescription = makeUnique<CAAudioStreamDescription>(*formatDescription);
+ weakThis->m_shouldRecreateDataSource = true;
+ }
callback();
});
}
@@ -56,8 +61,10 @@
void AudioMediaStreamTrackRendererCocoa::stop()
{
- if (m_dataSource)
- AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_dataSource);
+ ASSERT(isMainThread());
+
+ if (m_registeredDataSource)
+ AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_registeredDataSource);
}
void AudioMediaStreamTrackRendererCocoa::clear()
@@ -64,21 +71,25 @@
{
stop();
- m_dataSource = nullptr;
+ setRegisteredDataSource(nullptr);
m_outputDescription = { };
}
void AudioMediaStreamTrackRendererCocoa::setVolume(float volume)
{
+ ASSERT(isMainThread());
+
AudioMediaStreamTrackRenderer::setVolume(volume);
- if (m_dataSource)
- m_dataSource->setVolume(volume);
+ if (m_registeredDataSource)
+ m_registeredDataSource->setVolume(volume);
}
void AudioMediaStreamTrackRendererCocoa::reset()
{
- if (m_dataSource)
- m_dataSource->recomputeSampleOffset();
+ ASSERT(isMainThread());
+
+ if (m_registeredDataSource)
+ m_registeredDataSource->recomputeSampleOffset();
}
void AudioMediaStreamTrackRendererCocoa::setAudioOutputDevice(const String& deviceId)
@@ -88,6 +99,26 @@
m_shouldRecreateDataSource = true;
}
+void AudioMediaStreamTrackRendererCocoa::setRegisteredDataSource(RefPtr<AudioSampleDataSource>&& source)
+{
+ ASSERT(isMainThread());
+
+ if (m_registeredDataSource)
+ AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_registeredDataSource);
+
+ if (!m_outputDescription)
+ return;
+
+ m_registeredDataSource = WTFMove(source);
+ if (!m_registeredDataSource)
+ return;
+
+ m_registeredDataSource->setLogger(logger(), logIdentifier());
+ m_registeredDataSource->setVolume(volume());
+ AudioMediaStreamTrackRendererUnit::singleton().addResetObserver(m_resetObserver);
+ AudioMediaStreamTrackRendererUnit::singleton().addSource(*m_registeredDataSource);
+}
+
static unsigned pollSamplesCount()
{
#if USE(LIBWEBRTC)
@@ -118,19 +149,9 @@
return;
}
- callOnMainThread([this, weakThis = WeakPtr { *this }, oldSource = m_dataSource, newSource = dataSource]() mutable {
- if (!weakThis)
- return;
-
-#if !RELEASE_LOG_DISABLED
- newSource->setLogger(logger(), logIdentifier());
-#endif
- if (oldSource)
- AudioMediaStreamTrackRendererUnit::singleton().removeSource(*oldSource);
-
- newSource->setVolume(volume());
- AudioMediaStreamTrackRendererUnit::singleton().addResetObserver(m_resetObserver);
- AudioMediaStreamTrackRendererUnit::singleton().addSource(WTFMove(newSource));
+ callOnMainThread([weakThis = WeakPtr { *this }, newSource = dataSource]() mutable {
+ if (weakThis)
+ weakThis->setRegisteredDataSource(WTFMove(newSource));
});
m_dataSource = WTFMove(dataSource);
m_shouldRecreateDataSource = false;
Modified: trunk/Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h (285023 => 285024)
--- trunk/Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h 2021-10-29 09:50:10 UTC (rev 285023)
+++ trunk/Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h 2021-10-29 10:59:34 UTC (rev 285024)
@@ -57,9 +57,11 @@
void setAudioOutputDevice(const String&) final;
void reset();
+ void setRegisteredDataSource(RefPtr<AudioSampleDataSource>&&);
std::unique_ptr<CAAudioStreamDescription> m_outputDescription;
- RefPtr<AudioSampleDataSource> m_dataSource;
+ RefPtr<AudioSampleDataSource> m_dataSource; // Used in background thread.
+ RefPtr<AudioSampleDataSource> m_registeredDataSource; // Used in main thread.
bool m_shouldRecreateDataSource { false };
WebCore::AudioMediaStreamTrackRendererUnit::ResetObserver m_resetObserver;
};
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
