Make SckPickerProxy thread-safe with a Mutex
Single-threaded access doesn't work because SckPickerProxy is a
singleton and clients typically use different capturers for enumeration
and capture, and runs them for those use cases on different threads.
Bug: webrtc:367915807
Change-Id: I8695ea38a21bf3a307dfd21d169ef5ca2fe43809
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/392780
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Andreas Pehrson <apehrson@mozilla.com>
Cr-Commit-Position: refs/heads/main@{#44722}
diff --git a/modules/desktop_capture/mac/sck_picker_handle.mm b/modules/desktop_capture/mac/sck_picker_handle.mm
index c1b4db1..dd5391f 100644
--- a/modules/desktop_capture/mac/sck_picker_handle.mm
+++ b/modules/desktop_capture/mac/sck_picker_handle.mm
@@ -29,10 +29,7 @@
return g_picker;
}
- SckPickerProxy() : thread_checker_(SequenceChecker::kDetached) {}
-
- bool AtCapacity() const {
- RTC_DCHECK_RUN_ON(&thread_checker_);
+ bool AtCapacityLocked() const RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_) {
return handle_count_ == kMaximumStreamCount;
}
@@ -42,8 +39,8 @@
ABSL_MUST_USE_RESULT std::optional<DesktopCapturer::SourceId>
AcquireSourceId() {
- RTC_DCHECK_RUN_ON(&thread_checker_);
- if (AtCapacity()) {
+ MutexLock lock(&mutex_);
+ if (AtCapacityLocked()) {
return std::nullopt;
}
if (handle_count_ == 0) {
@@ -58,25 +55,26 @@
}
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 +88,10 @@
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 +101,11 @@
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 d6e3c2e..49c923e 100644
--- a/modules/desktop_capture/mac/screen_capturer_sck.mm
+++ b/modules/desktop_capture/mac/screen_capturer_sck.mm
@@ -72,6 +72,8 @@
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 @@
: 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::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 @@
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(), std::string()});
}
return true;
}
bool ScreenCapturerSck::SelectSource(SourceId id) {
- RTC_DCHECK_RUN_ON(&api_checker_);
-
if (capture_options_.allow_sck_system_picker()) {
return true;
}
