From 16b9b8615d5950edf08658dbb9647c3ee12bedf7 Mon Sep 17 00:00:00 2001 From: Peter Varga Date: Tue, 22 Jan 2019 16:15:07 +0100 Subject: [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 Reviewed-by: Tommi Cr-Commit-Position: refs/heads/master@{#25437} Reviewed-by: Allan Sandfeld Jensen --- .../mac/desktop_configuration_monitor.cc | 51 +++++++--------------- .../mac/desktop_configuration_monitor.h | 19 +++----- .../desktop_capture/mac/screen_capturer_mac.mm | 10 ----- .../desktop_capture/mouse_cursor_monitor_mac.mm | 2 - .../modules/desktop_capture/window_capturer_mac.mm | 2 - 5 files changed, 21 insertions(+), 63 deletions(-) (limited to 'chromium') 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 reconfiguring_displays_; - MacDesktopConfiguration desktop_configuration_; - std::unique_ptr 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_; -- cgit v1.2.1