From bd616ecd647ebc1b94dd08927a1b85a912a04d37 Mon Sep 17 00:00:00 2001 From: Joe Downing Date: Thu, 18 Aug 2022 13:01:47 -0700 Subject: [PATCH] Allow concurrent desktop_capture instances on X11 I would like to run a separate capturer for each desktop on Linux and I ran into the DCHECK in XErrorTrap when I was prototyping that solution. I addressed it by using a Mutex and then experienced and occasional hang when capturing which I traced down to SharedXDisplay::ProcessPendingXEvents(), this is a shared display instance used by each unique capturer instance so I added a mutex there as well. I ran 2 capturer instances concurrently for well over an hour and did not experience any hangs or capture artifacts. Bug: webrtc:2022 Change-Id: Ia6778cae4bbae48886fe45f2991f02e0ea08fef6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/271920 Reviewed-by: Alexander Cooper Commit-Queue: Joe Downing Cr-Commit-Position: refs/heads/main@{#37892} --- .../linux/x11/mouse_cursor_monitor_x11.cc | 25 +++++++++------- .../linux/x11/shared_x_display.cc | 6 ++++ .../linux/x11/shared_x_display.h | 6 +++- .../desktop_capture/linux/x11/x_error_trap.cc | 30 +++++++++++-------- .../desktop_capture/linux/x11/x_error_trap.h | 14 +++++---- 5 files changed, 52 insertions(+), 29 deletions(-) diff --git a/modules/desktop_capture/linux/x11/mouse_cursor_monitor_x11.cc b/modules/desktop_capture/linux/x11/mouse_cursor_monitor_x11.cc index 0f4be547e9..d9c7635c1d 100644 --- a/modules/desktop_capture/linux/x11/mouse_cursor_monitor_x11.cc +++ b/modules/desktop_capture/linux/x11/mouse_cursor_monitor_x11.cc @@ -145,18 +145,21 @@ void MouseCursorMonitorX11::Capture() { Window child_window; unsigned int mask; - XErrorTrap error_trap(display()); - Bool result = XQueryPointer(display(), window_, &root_window, &child_window, - &root_x, &root_y, &win_x, &win_y, &mask); CursorState state; - if (!result || error_trap.GetLastErrorAndDisable() != 0) { - state = OUTSIDE; - } else { - // In screen mode (window_ == root_window) the mouse is always inside. - // XQueryPointer() sets `child_window` to None if the cursor is outside - // `window_`. - state = - (window_ == root_window || child_window != None) ? INSIDE : OUTSIDE; + { + XErrorTrap error_trap(display()); + Bool result = + XQueryPointer(display(), window_, &root_window, &child_window, + &root_x, &root_y, &win_x, &win_y, &mask); + if (!result || error_trap.GetLastErrorAndDisable() != 0) { + state = OUTSIDE; + } else { + // In screen mode (window_ == root_window) the mouse is always inside. + // XQueryPointer() sets `child_window` to None if the cursor is outside + // `window_`. + state = + (window_ == root_window || child_window != None) ? INSIDE : OUTSIDE; + } } // As the comments to GetTopLevelWindow() above indicate, in window capture, diff --git a/modules/desktop_capture/linux/x11/shared_x_display.cc b/modules/desktop_capture/linux/x11/shared_x_display.cc index b7849508b0..d690b0e2ba 100644 --- a/modules/desktop_capture/linux/x11/shared_x_display.cc +++ b/modules/desktop_capture/linux/x11/shared_x_display.cc @@ -48,10 +48,12 @@ rtc::scoped_refptr SharedXDisplay::CreateDefault() { } void SharedXDisplay::AddEventHandler(int type, XEventHandler* handler) { + MutexLock lock(&mutex_); event_handlers_[type].push_back(handler); } void SharedXDisplay::RemoveEventHandler(int type, XEventHandler* handler) { + MutexLock lock(&mutex_); EventHandlersMap::iterator handlers = event_handlers_.find(type); if (handlers == event_handlers_.end()) return; @@ -70,6 +72,10 @@ void SharedXDisplay::ProcessPendingXEvents() { // processing events. rtc::scoped_refptr self(this); + // Protect access to `event_handlers_` after incrementing the refcount for + // `this` to ensure the instance is still valid when the lock is acquired. + MutexLock lock(&mutex_); + // Find the number of events that are outstanding "now." We don't just loop // on XPending because we want to guarantee this terminates. int events_to_process = XPending(display()); diff --git a/modules/desktop_capture/linux/x11/shared_x_display.h b/modules/desktop_capture/linux/x11/shared_x_display.h index 084da80167..c05fc46546 100644 --- a/modules/desktop_capture/linux/x11/shared_x_display.h +++ b/modules/desktop_capture/linux/x11/shared_x_display.h @@ -17,7 +17,9 @@ #include "absl/strings/string_view.h" #include "api/ref_counted_base.h" #include "api/scoped_refptr.h" +#include "rtc_base/synchronization/mutex.h" #include "rtc_base/system/rtc_export.h" +#include "rtc_base/thread_annotations.h" // Including Xlib.h will involve evil defines (Bool, Status, True, False), which // easily conflict with other headers. @@ -76,7 +78,9 @@ class RTC_EXPORT SharedXDisplay Display* display_; - EventHandlersMap event_handlers_; + Mutex mutex_; + + EventHandlersMap event_handlers_ RTC_GUARDED_BY(mutex_); }; } // namespace webrtc diff --git a/modules/desktop_capture/linux/x11/x_error_trap.cc b/modules/desktop_capture/linux/x11/x_error_trap.cc index f31565e24b..94e3a03c73 100644 --- a/modules/desktop_capture/linux/x11/x_error_trap.cc +++ b/modules/desktop_capture/linux/x11/x_error_trap.cc @@ -10,6 +10,8 @@ #include "modules/desktop_capture/linux/x11/x_error_trap.h" +#include + #include #include "rtc_base/checks.h" @@ -18,36 +20,40 @@ namespace webrtc { namespace { -// TODO(sergeyu): This code is not thread safe. Fix it. Bug 2202. -static bool g_xserver_error_trap_enabled = false; static int g_last_xserver_error_code = 0; +static std::atomic g_display_for_error_handler = nullptr; + +Mutex* AcquireMutex() { + static Mutex* mutex = new Mutex(); + return mutex; +} int XServerErrorHandler(Display* display, XErrorEvent* error_event) { - RTC_DCHECK(g_xserver_error_trap_enabled); + RTC_DCHECK_EQ(display, g_display_for_error_handler.load()); g_last_xserver_error_code = error_event->error_code; return 0; } } // namespace -XErrorTrap::XErrorTrap(Display* display) - : original_error_handler_(NULL), enabled_(true) { - RTC_DCHECK(!g_xserver_error_trap_enabled); - original_error_handler_ = XSetErrorHandler(&XServerErrorHandler); - g_xserver_error_trap_enabled = true; +XErrorTrap::XErrorTrap(Display* display) : mutex_lock_(AcquireMutex()) { + // We don't expect this class to be used in a nested fashion so therefore + // g_display_for_error_handler should never be valid here. + RTC_DCHECK(!g_display_for_error_handler.load()); + RTC_DCHECK(display); + g_display_for_error_handler.store(display); g_last_xserver_error_code = 0; + original_error_handler_ = XSetErrorHandler(&XServerErrorHandler); } int XErrorTrap::GetLastErrorAndDisable() { - enabled_ = false; - RTC_DCHECK(g_xserver_error_trap_enabled); + g_display_for_error_handler.store(nullptr); XSetErrorHandler(original_error_handler_); - g_xserver_error_trap_enabled = false; return g_last_xserver_error_code; } XErrorTrap::~XErrorTrap() { - if (enabled_) + if (g_display_for_error_handler.load() != nullptr) GetLastErrorAndDisable(); } diff --git a/modules/desktop_capture/linux/x11/x_error_trap.h b/modules/desktop_capture/linux/x11/x_error_trap.h index 882d690922..1f21ab969c 100644 --- a/modules/desktop_capture/linux/x11/x_error_trap.h +++ b/modules/desktop_capture/linux/x11/x_error_trap.h @@ -13,24 +13,28 @@ #include +#include "rtc_base/synchronization/mutex.h" + namespace webrtc { -// Helper class that registers X Window error handler. Caller can use +// Helper class that registers an X Window error handler. Caller can use // GetLastErrorAndDisable() to get the last error that was caught, if any. class XErrorTrap { public: explicit XErrorTrap(Display* display); - ~XErrorTrap(); XErrorTrap(const XErrorTrap&) = delete; XErrorTrap& operator=(const XErrorTrap&) = delete; - // Returns last error and removes unregisters the error handler. + ~XErrorTrap(); + + // Returns the last error if one was caught, otherwise 0. Also unregisters the + // error handler and replaces it with `original_error_handler_`. int GetLastErrorAndDisable(); private: - XErrorHandler original_error_handler_; - bool enabled_; + MutexLock mutex_lock_; + XErrorHandler original_error_handler_ = nullptr; }; } // namespace webrtc