[Backport][WebRTC] Remove event wait logic from DesktopConfigurationMonitor

This class exposes Wait()-Set() logic to synchronize events.
- There is a bug in checking EventWrapper::Wait() as it returns [1,2]. Negating
these values cause us to always pass timeout checks.
- There is a general problem in this class with waiter. There are 2 scenarios:
1) Lock()-Unlock()-DisplaysReconfigured()
In this scenario, Wait() in DisplaysReconfigured() immediately passes as event
is already signaled. Next Lock() call won't continue until Set() is called in
DisplaysReconfigured(). This blocks capture thread from accessing display until
reconfiguration completes.
2) Lock()-DisplaysReconfigured()-Unlock()
In this scenario, Wait() in DisplaysReconfigured() passes when Unlock() called.
Capture thread accesses display while reconfiguration happens. Note that we are
only delaying the OS delegate thread here. As an experiment, adding Sleep() in
DisplaysReconfigured() results in no change, because it looks like OS uses this
thread for only delegates but not for the actual display switch.

Overall, (1) doesnt seem necessary as (2) already accesses display while
reconfiguration happens. (2) doesn't seem necessary as blocking system delegate
thread doesn't help. Therefore, I changed the class to only protect from race
condition on |desktop_configuration_|.

Bug: chromium:796889
Change-Id: I5a70c4efff999204eab8b1cfd66cbfe953b26467
Reviewed-on: https://webrtc-review.googlesource.com/c/108560
Commit-Queue: Emircan Uysaler <emircan@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25437}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

5 files changed, 21 insertions, 63 deletions

diff --git a/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.cc b/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.cc
index d4364bedc2b..8029ffda1c7 100644
--- a/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.cc
+++ b/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.cc
@@ -17,19 +17,13 @@
 
 namespace webrtc {
 
-// The amount of time allowed for displays to reconfigure.
-static const int64_t kDisplayConfigurationEventTimeoutMs = 10 * 1000;
-
-DesktopConfigurationMonitor::DesktopConfigurationMonitor()
-    : display_configuration_capture_event_(EventWrapper::Create()) {
+DesktopConfigurationMonitor::DesktopConfigurationMonitor() {
   CGError err = CGDisplayRegisterReconfigurationCallback(
       DesktopConfigurationMonitor::DisplaysReconfiguredCallback, this);
-  if (err != kCGErrorSuccess) {
+  if (err != kCGErrorSuccess)
     RTC_LOG(LS_ERROR) << "CGDisplayRegisterReconfigurationCallback " << err;
-    abort();
-  }
-  display_configuration_capture_event_->Set();
 
+  rtc::CritScope cs(&desktop_configuration_lock_);
   desktop_configuration_ = MacDesktopConfiguration::GetCurrent(
       MacDesktopConfiguration::TopLeftOrigin);
 }
@@ -41,19 +35,13 @@ DesktopConfigurationMonitor::~DesktopConfigurationMonitor() {
     RTC_LOG(LS_ERROR) << "CGDisplayRemoveReconfigurationCallback " << err;
 }
 
-void DesktopConfigurationMonitor::Lock() {
-  if (!display_configuration_capture_event_->Wait(
-          kDisplayConfigurationEventTimeoutMs)) {
-    RTC_LOG_F(LS_ERROR) << "Event wait timed out.";
-    abort();
-  }
-}
-
-void DesktopConfigurationMonitor::Unlock() {
-  display_configuration_capture_event_->Set();
+MacDesktopConfiguration DesktopConfigurationMonitor::desktop_configuration() {
+    rtc::CritScope crit(&desktop_configuration_lock_);
+    return desktop_configuration_;
 }
 
 // static
+// This method may be called on any system thread.
 void DesktopConfigurationMonitor::DisplaysReconfiguredCallback(
     CGDirectDisplayID display,
     CGDisplayChangeSummaryFlags flags,
@@ -72,24 +60,15 @@ void DesktopConfigurationMonitor::DisplaysReconfigured(
                    << flags;
 
   if (flags & kCGDisplayBeginConfigurationFlag) {
-    if (reconfiguring_displays_.empty()) {
-      // If this is the first display to start reconfiguring then wait on
-      // |display_configuration_capture_event_| to block the capture thread
-      // from accessing display memory until the reconfiguration completes.
-      if (!display_configuration_capture_event_->Wait(
-              kDisplayConfigurationEventTimeoutMs)) {
-        RTC_LOG_F(LS_ERROR) << "Event wait timed out.";
-        abort();
-      }
-    }
     reconfiguring_displays_.insert(display);
-  } else {
-    reconfiguring_displays_.erase(display);
-    if (reconfiguring_displays_.empty()) {
-      desktop_configuration_ = MacDesktopConfiguration::GetCurrent(
-          MacDesktopConfiguration::TopLeftOrigin);
-      display_configuration_capture_event_->Set();
-    }
+    return;
+  }
+
+  reconfiguring_displays_.erase(display);
+  if (reconfiguring_displays_.empty()) {
+    rtc::CritScope cs(&desktop_configuration_lock_);
+    desktop_configuration_ = MacDesktopConfiguration::GetCurrent(
+        MacDesktopConfiguration::TopLeftOrigin);
   }
 }
 
diff --git a/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h b/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h
index 5215e5496ad..cbf580b2353 100644
--- a/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h
+++ b/chromium/third_party/webrtc/modules/desktop_capture/mac/desktop_configuration_monitor.h
@@ -19,25 +19,17 @@
 #include "api/refcountedbase.h"
 #include "modules/desktop_capture/mac/desktop_configuration.h"
 #include "rtc_base/constructormagic.h"
+#include "rtc_base/criticalsection.h"
 
 namespace webrtc {
 
-class EventWrapper;
-
 // The class provides functions to synchronize capturing and display
 // reconfiguring across threads, and the up-to-date MacDesktopConfiguration.
 class DesktopConfigurationMonitor : public rtc::RefCountedBase {
  public:
   DesktopConfigurationMonitor();
-  // Acquires a lock on the current configuration.
-  void Lock();
-  // Releases the lock previously acquired.
-  void Unlock();
-  // Returns the current desktop configuration. Should only be called when the
-  // lock has been acquired.
-  const MacDesktopConfiguration& desktop_configuration() {
-    return desktop_configuration_;
-  }
+  // Returns the current desktop configuration.
+  MacDesktopConfiguration desktop_configuration();
 
  protected:
   ~DesktopConfigurationMonitor() override;
@@ -49,9 +41,10 @@ class DesktopConfigurationMonitor : public rtc::RefCountedBase {
   void DisplaysReconfigured(CGDirectDisplayID display,
                             CGDisplayChangeSummaryFlags flags);
 
+  rtc::CriticalSection desktop_configuration_lock_;
+  MacDesktopConfiguration desktop_configuration_
+      RTC_GUARDED_BY(&desktop_configuration_lock_);
   std::set<CGDirectDisplayID> reconfiguring_displays_;
-  MacDesktopConfiguration desktop_configuration_;
-  std::unique_ptr<EventWrapper> display_configuration_capture_event_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(DesktopConfigurationMonitor);
 };
diff --git a/chromium/third_party/webrtc/modules/desktop_capture/mac/screen_capturer_mac.mm b/chromium/third_party/webrtc/modules/desktop_capture/mac/screen_capturer_mac.mm
index 0d4fa072b5f..542d1c5f43f 100644
--- a/chromium/third_party/webrtc/modules/desktop_capture/mac/screen_capturer_mac.mm
+++ b/chromium/third_party/webrtc/modules/desktop_capture/mac/screen_capturer_mac.mm
@@ -167,11 +167,7 @@ ScreenCapturerMac::~ScreenCapturerMac() {
 
 bool ScreenCapturerMac::Init() {
   TRACE_EVENT0("webrtc", "ScreenCapturerMac::Init");
-
-  desktop_config_monitor_->Lock();
   desktop_config_ = desktop_config_monitor_->desktop_configuration();
-  desktop_config_monitor_->Unlock();
-
   return true;
 }
 
@@ -207,7 +203,6 @@ void ScreenCapturerMac::CaptureFrame() {
   queue_.MoveToNextFrame();
   RTC_DCHECK(!queue_.current_frame() || !queue_.current_frame()->IsShared());
 
-  desktop_config_monitor_->Lock();
   MacDesktopConfiguration new_config = desktop_config_monitor_->desktop_configuration();
   if (!desktop_config_.Equals(new_config)) {
     desktop_config_ = new_config;
@@ -234,7 +229,6 @@ void ScreenCapturerMac::CaptureFrame() {
   DesktopFrame* current_frame = queue_.current_frame();
 
   if (!CgBlit(*current_frame, region)) {
-    desktop_config_monitor_->Unlock();
     callback_->OnCaptureResult(Result::ERROR_PERMANENT, nullptr);
     return;
   }
@@ -256,10 +250,6 @@ void ScreenCapturerMac::CaptureFrame() {
 
   helper_.set_size_most_recent(new_frame->size());
 
-  // Signal that we are done capturing data from the display framebuffer,
-  // and accessing display structures.
-  desktop_config_monitor_->Unlock();
-
   new_frame->set_capture_time_ms((rtc::TimeNanos() - capture_start_time_nanos) /
                                  rtc::kNumNanosecsPerMillisec);
   callback_->OnCaptureResult(Result::SUCCESS, std::move(new_frame));
diff --git a/chromium/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm b/chromium/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm
index de8876438f8..3402a5ffbbc 100644
--- a/chromium/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm
+++ b/chromium/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_mac.mm
@@ -117,10 +117,8 @@ void MouseCursorMonitorMac::Capture() {
 
   DesktopVector position(gc_position.x, gc_position.y);
 
-  configuration_monitor_->Lock();
   MacDesktopConfiguration configuration =
       configuration_monitor_->desktop_configuration();
-  configuration_monitor_->Unlock();
   float scale = GetScaleFactorAtPosition(configuration, position);
 
   CaptureImage(scale);
diff --git a/chromium/third_party/webrtc/modules/desktop_capture/window_capturer_mac.mm b/chromium/third_party/webrtc/modules/desktop_capture/window_capturer_mac.mm
index 95e622a77a1..b0647f041b6 100644
--- a/chromium/third_party/webrtc/modules/desktop_capture/window_capturer_mac.mm
+++ b/chromium/third_party/webrtc/modules/desktop_capture/window_capturer_mac.mm
@@ -140,9 +140,7 @@ bool WindowCapturerMac::FocusOnSelectedSource() {
 bool WindowCapturerMac::IsOccluded(const DesktopVector& pos) {
   DesktopVector sys_pos = pos;
   if (configuration_monitor_) {
-    configuration_monitor_->Lock();
     auto configuration = configuration_monitor_->desktop_configuration();
-    configuration_monitor_->Unlock();
     sys_pos = pos.add(configuration.bounds.top_left());
   }
   return window_finder_.GetWindowUnderPoint(sys_pos) != window_id_;