Bug 1966238 - (fix-bf85c38fb3) Make SckPickerProxy thread-safe but each SckPickerHandle single-threaded. r=mjf,webrtc-reviewers

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.

Differential Revision: https://phabricator.services.mozilla.com/D249742

third_party/libwebrtc/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;
   }
 
@@ -42,8 +40,8 @@ class API_AVAILABLE(macos(14.0)) SckPickerProxy {
 
   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 +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_;
 };

third_party/libwebrtc/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,8 +778,7 @@ std::unique_ptr<DesktopCapturer> CreateGenericCapturerSck(
   if (@available(macOS 14.0, *)) {
     if (options.allow_sck_system_picker()) {
       return std::make_unique<ScreenCapturerSck>(
-          options,
-          SCContentSharingPickerModeSingleDisplay |
+          options, SCContentSharingPickerModeSingleDisplay |
                        SCContentSharingPickerModeMultipleWindows);
     }
   }