From 06e88bbb5aabdbc2614e51739d79a756c9e4d34b Mon Sep 17 00:00:00 2001
From: Jan Grulich <grulja@gmail.com>
Date: Tue, 4 Jun 2024 19:38:21 +0200
Subject: [PATCH] PipeWire capturer: fix some possible threading issues

- avoid holding a lock across OnCaptureResult() callback to avoid a risk
  of a possible deadlock
- annotate damage region as guarded by the same lock as latest frame as
  both belong together
- document the acqusition order between locks

Bug: chromium:333945842
Change-Id: I9c65beed720ba54e40b85fb243a07d40524695f4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/353600
Commit-Queue: Jan Grulich <grulja@gmail.com>
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Andreas Pehrson <apehrson@mozilla.com>
Cr-Commit-Position: refs/heads/main@{#42432}
---
 .../linux/wayland/shared_screencast_stream.cc | 53 +++++++++----------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc b/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc
index 15f9b1e55f..90273aba3e 100644
--- a/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc
+++ b/modules/desktop_capture/linux/wayland/shared_screencast_stream.cc
@@ -73,7 +73,7 @@ class SharedScreenCastStreamPrivate {
 
   // Track damage region updates that were reported since the last time
   // frame was captured
-  DesktopRegion damage_region_;
+  DesktopRegion damage_region_ RTC_GUARDED_BY(&latest_frame_lock_);
 
   uint32_t pw_stream_node_id_ = 0;
 
@@ -83,7 +83,7 @@ class SharedScreenCastStreamPrivate {
   webrtc::Mutex queue_lock_;
   ScreenCaptureFrameQueue<SharedDesktopFrame> queue_
       RTC_GUARDED_BY(&queue_lock_);
-  webrtc::Mutex latest_frame_lock_;
+  webrtc::Mutex latest_frame_lock_ RTC_ACQUIRED_AFTER(queue_lock_);
   SharedDesktopFrame* latest_available_frame_
       RTC_GUARDED_BY(&latest_frame_lock_) = nullptr;
   std::unique_ptr<MouseCursor> mouse_cursor_;
@@ -140,7 +140,6 @@ class SharedScreenCastStreamPrivate {
   void ConvertRGBxToBGRx(uint8_t* frame, uint32_t size);
   void UpdateFrameUpdatedRegions(const spa_buffer* spa_buffer,
                                  DesktopFrame& frame);
-  void NotifyCallbackOfNewFrame(std::unique_ptr<SharedDesktopFrame> frame);
 
   // PipeWire callbacks
   static void OnCoreError(void* data,
@@ -636,6 +635,8 @@ DesktopVector SharedScreenCastStreamPrivate::CaptureCursorPosition() {
 void SharedScreenCastStreamPrivate::UpdateFrameUpdatedRegions(
     const spa_buffer* spa_buffer,
     DesktopFrame& frame) {
+  latest_frame_lock_.AssertHeld();
+
   if (!use_damage_region_) {
     frame.mutable_updated_region()->SetRect(
         DesktopRect::MakeSize(frame.size()));
@@ -664,22 +665,6 @@ void SharedScreenCastStreamPrivate::UpdateFrameUpdatedRegions(
   }
 }
 
-void SharedScreenCastStreamPrivate::NotifyCallbackOfNewFrame(
-    std::unique_ptr<SharedDesktopFrame> frame) {
-  if (!pw_stream_ || !frame->data()) {
-    callback_->OnCaptureResult(DesktopCapturer::Result::ERROR_TEMPORARY,
-                               nullptr);
-    return;
-  }
-
-  if (use_damage_region_) {
-    frame->mutable_updated_region()->Swap(&damage_region_);
-    damage_region_.Clear();
-  }
-  callback_->OnCaptureResult(DesktopCapturer::Result::SUCCESS,
-                             std::move(frame));
-}
-
 RTC_NO_SANITIZE("cfi-icall")
 void SharedScreenCastStreamPrivate::ProcessBuffer(pw_buffer* buffer) {
   int64_t capture_start_time_nanos = rtc::TimeNanos();
@@ -865,22 +850,32 @@ void SharedScreenCastStreamPrivate::ProcessBuffer(pw_buffer* buffer) {
     observer_->OnDesktopFrameChanged();
   }
 
-  // We have to hold the lock over the latest_frame here already, since
-  // we are going to update damage region, which corresponds to the latest
-  // frame and is accessed in CaptureFrame()
-  webrtc::MutexLock latest_frame_lock(&latest_frame_lock_);
+  std::unique_ptr<SharedDesktopFrame> frame;
+  {
+    webrtc::MutexLock latest_frame_lock(&latest_frame_lock_);
 
-  UpdateFrameUpdatedRegions(spa_buffer, *queue_.current_frame());
-  queue_.current_frame()->set_may_contain_cursor(is_cursor_embedded_);
+    UpdateFrameUpdatedRegions(spa_buffer, *queue_.current_frame());
+    queue_.current_frame()->set_may_contain_cursor(is_cursor_embedded_);
 
-  latest_available_frame_ = queue_.current_frame();
+    latest_available_frame_ = queue_.current_frame();
 
-  if (callback_) {
-    std::unique_ptr<SharedDesktopFrame> frame = queue_.current_frame()->Share();
+    if (!callback_) {
+      return;
+    }
+
+    frame = latest_available_frame_->Share();
     frame->set_capturer_id(DesktopCapturerId::kWaylandCapturerLinux);
     frame->set_capture_time_ms((rtc::TimeNanos() - capture_start_time_nanos) /
                                rtc::kNumNanosecsPerMillisec);
-    NotifyCallbackOfNewFrame(std::move(frame));
+    if (use_damage_region_) {
+      frame->mutable_updated_region()->Swap(&damage_region_);
+      damage_region_.Clear();
+    }
+  }
+
+  if (callback_) {
+    callback_->OnCaptureResult(DesktopCapturer::Result::SUCCESS,
+                               std::move(frame));
   }
 }