Source code

Revision control

Copy as Markdown

Other Tools

From: Andreas Pehrson <apehrson@mozilla.com>
Date: Fri, 16 May 2025 08:36:00 +0000
Subject: Bug 1966238 - (fix-bf85c38fb3) Make SckPickerProxy thread-safe but
each SckPickerHandle single-threaded. r?mjf
In gecko, enumeration via GetSourceList happens on the VideoCapture thread while
frame capture happens on the DesktopCapture thread. All these instances share
the same SckPickerProxy singleton, which then has to be thread-safe.
---
.../desktop_capture/mac/sck_picker_handle.mm | 39 +++++++++--------
.../mac/screen_capturer_sck.mm | 43 +++++++++++--------
2 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/modules/desktop_capture/mac/sck_picker_handle.mm b/modules/desktop_capture/mac/sck_picker_handle.mm
index c1b4db19f6..442648676c 100644
--- a/modules/desktop_capture/mac/sck_picker_handle.mm
+++ b/modules/desktop_capture/mac/sck_picker_handle.mm
@@ -29,10 +29,8 @@ class API_AVAILABLE(macos(14.0)) SckPickerProxy {
return g_picker;
}
- SckPickerProxy() : thread_checker_(SequenceChecker::kDetached) {}
-
- bool AtCapacity() const {
- RTC_DCHECK_RUN_ON(&thread_checker_);
+ bool AtCapacityLocked() const {
+ mutex_.AssertHeld();
return handle_count_ == kMaximumStreamCount;
}
@@ -41,9 +39,9 @@ class API_AVAILABLE(macos(14.0)) SckPickerProxy {
}
ABSL_MUST_USE_RESULT std::optional<DesktopCapturer::SourceId>
- AcquireSourceId() {
- RTC_DCHECK_RUN_ON(&thread_checker_);
- if (AtCapacity()) {
+ AcquireSourceId() {
+ MutexLock lock(&mutex_);
+ if (AtCapacityLocked()) {
return std::nullopt;
}
if (handle_count_ == 0) {
@@ -58,25 +56,26 @@ class API_AVAILABLE(macos(14.0)) SckPickerProxy {
}
void RelinquishSourceId(DesktopCapturer::SourceId source) {
- RTC_DCHECK_RUN_ON(&thread_checker_);
+ MutexLock lock(&mutex_);
handle_count_ -= 1;
if (handle_count_ > 0) {
return;
}
- // Detach now in case the next user (possibly after a long time) uses a
- // different thread.
- thread_checker_.Detach();
GetPicker().active = NO;
}
private:
- webrtc::SequenceChecker thread_checker_;
+ // SckPickerProxy is a process-wide singleton. ScreenCapturerSck and
+ // SckPickerHandle are largely single-threaded but may be used on different
+ // threads. For instance some clients use a capturer on one thread for
+ // enumeration and on another for frame capture. Since all those capturers
+ // share the same SckPickerProxy instance, it must be thread-safe.
+ Mutex mutex_;
// 100 is an arbitrary number that seems high enough to never get reached,
// while still providing a reasonably low upper bound.
static constexpr size_t kMaximumStreamCount = 100;
- size_t handle_count_ RTC_GUARDED_BY(thread_checker_) = 0;
- DesktopCapturer::SourceId unique_source_id_ RTC_GUARDED_BY(thread_checker_) =
- 0;
+ size_t handle_count_ RTC_GUARDED_BY(mutex_) = 0;
+ DesktopCapturer::SourceId unique_source_id_ RTC_GUARDED_BY(mutex_) = 0;
};
class API_AVAILABLE(macos(14.0)) SckPickerHandle
@@ -90,7 +89,10 @@ class API_AVAILABLE(macos(14.0)) SckPickerHandle
return std::unique_ptr<SckPickerHandle>(new SckPickerHandle(proxy, *id));
}
- ~SckPickerHandle() { proxy_->RelinquishSourceId(source_); }
+ ~SckPickerHandle() {
+ RTC_DCHECK_RUN_ON(&thread_checker_);
+ proxy_->RelinquishSourceId(source_);
+ }
SCContentSharingPicker* GetPicker() const override {
return proxy_->GetPicker();
@@ -100,8 +102,11 @@ class API_AVAILABLE(macos(14.0)) SckPickerHandle
private:
SckPickerHandle(SckPickerProxy* proxy, DesktopCapturer::SourceId source)
- : proxy_(proxy), source_(source) {}
+ : proxy_(proxy), source_(source) {
+ RTC_DCHECK_RUN_ON(&thread_checker_);
+ }
+ webrtc::SequenceChecker thread_checker_;
SckPickerProxy* const proxy_;
const DesktopCapturer::SourceId source_;
};
diff --git a/modules/desktop_capture/mac/screen_capturer_sck.mm b/modules/desktop_capture/mac/screen_capturer_sck.mm
index a664c013bc..7b3814645e 100644
--- a/modules/desktop_capture/mac/screen_capturer_sck.mm
+++ b/modules/desktop_capture/mac/screen_capturer_sck.mm
@@ -72,6 +72,8 @@ class API_AVAILABLE(macos(14.0)) ScreenCapturerSck final
void CaptureFrame() override;
bool GetSourceList(SourceList* sources) override;
bool SelectSource(SourceId id) override;
+ // Creates the SckPickerHandle if needed and not already done.
+ void EnsurePickerHandle();
// Prep for implementing DelegatedSourceListController interface, for now used
// by Start(). Triggers SCContentSharingPicker. Runs on the caller's thread.
void EnsureVisible();
@@ -218,15 +220,6 @@ ScreenCapturerSck::ScreenCapturerSck(const DesktopCaptureOptions& options,
: api_checker_(SequenceChecker::kDetached),
capture_options_(options),
picker_modes_(modes) {
- if (capture_options_.allow_sck_system_picker()) {
- picker_handle_ = CreateSckPickerHandle();
- }
- RTC_LOG(LS_INFO) << "ScreenCapturerSck " << this
- << " created. allow_sck_system_picker="
- << capture_options_.allow_sck_system_picker() << ", source="
- << (picker_handle_ ? picker_handle_->Source() : -1)
- << ", modes="
- << StringifiableSCContentSharingPickerMode{.modes_ = modes};
helper_ = [[SckHelper alloc] initWithCapturer:this];
}
@@ -322,9 +315,25 @@ void ScreenCapturerSck::CaptureFrame() {
}
}
+void ScreenCapturerSck::EnsurePickerHandle() {
+ RTC_DCHECK_RUN_ON(&api_checker_);
+ if (!picker_handle_ && capture_options_.allow_sck_system_picker()) {
+ picker_handle_ = CreateSckPickerHandle();
+ RTC_LOG(LS_INFO) << "ScreenCapturerSck " << this
+ << " Created picker handle. allow_sck_system_picker="
+ << capture_options_.allow_sck_system_picker()
+ << ", source="
+ << (picker_handle_ ? picker_handle_->Source() : -1)
+ << ", modes="
+ << StringifiableSCContentSharingPickerMode{
+ .modes_ = picker_modes_};
+ }
+}
+
void ScreenCapturerSck::EnsureVisible() {
RTC_DCHECK_RUN_ON(&api_checker_);
RTC_LOG(LS_INFO) << "ScreenCapturerSck " << this << " " << __func__ << ".";
+ EnsurePickerHandle();
if (picker_handle_) {
if (!picker_handle_registered_) {
picker_handle_registered_ = true;
@@ -433,15 +442,14 @@ void ScreenCapturerSck::NotifyCaptureStopped(SCStream* stream) {
bool ScreenCapturerSck::GetSourceList(SourceList* sources) {
RTC_DCHECK_RUN_ON(&api_checker_);
sources->clear();
- if (capture_options_.allow_sck_system_picker() && picker_handle_) {
+ EnsurePickerHandle();
+ if (picker_handle_) {
sources->push_back({picker_handle_->Source(), 0, std::string()});
}
return true;
}
bool ScreenCapturerSck::SelectSource(SourceId id) {
- RTC_DCHECK_RUN_ON(&api_checker_);
-
if (capture_options_.allow_sck_system_picker()) {
return true;
}
@@ -526,9 +534,9 @@ void ScreenCapturerSck::StartWithFilter(SCContentFilter* __strong filter) {
config.colorSpaceName = kCGColorSpaceSRGB;
config.showsCursor = capture_options_.prefer_cursor_embedded();
config.captureResolution = SCCaptureResolutionNominal;
- config.minimumFrameInterval = max_frame_rate_ > 0 ?
- CMTimeMake(1, static_cast<int32_t>(max_frame_rate_)) :
- kCMTimeZero;
+ config.minimumFrameInterval =
+ max_frame_rate_ > 0 ? CMTimeMake(1, static_cast<int32_t>(max_frame_rate_))
+ : kCMTimeZero;
{
MutexLock lock(&latest_frame_lock_);
@@ -770,9 +778,8 @@ std::unique_ptr<DesktopCapturer> CreateGenericCapturerSck(
if (@available(macOS 14.0, *)) {
if (options.allow_sck_system_picker()) {
return std::make_unique<ScreenCapturerSck>(
- options,
- SCContentSharingPickerModeSingleDisplay |
- SCContentSharingPickerModeMultipleWindows);
+ options, SCContentSharingPickerModeSingleDisplay |
+ SCContentSharingPickerModeMultipleWindows);
}
}
return nullptr;