From 825f65e9d2285e69467acd6449549f10f9e1df6b Mon Sep 17 00:00:00 2001 From: Zijie He Date: Wed, 16 Aug 2017 14:56:42 -0700 Subject: [PATCH] Share ScreenDrawerLockPosix implementation This change renames ScreenDrawerLockLinux into ScreenDrawerLockPosix and shares it with Mac OSX. Bug: webrtc:7950 Change-Id: Ib141781d2c35bfda0d6f9458fff235adbb643280 Reviewed-on: https://chromium-review.googlesource.com/607688 Commit-Queue: Zijie He Reviewed-by: Jamie Walch Cr-Commit-Position: refs/heads/master@{#19393} --- webrtc/modules/desktop_capture/BUILD.gn | 2 + .../desktop_capture/screen_drawer_linux.cc | 33 +------ .../screen_drawer_lock_posix.cc | 58 +++++++++++ .../screen_drawer_lock_posix.h | 38 +++++++ .../desktop_capture/screen_drawer_mac.cc | 4 +- .../desktop_capture/screen_drawer_unittest.cc | 99 +++++++++++++++++++ 6 files changed, 204 insertions(+), 30 deletions(-) create mode 100644 webrtc/modules/desktop_capture/screen_drawer_lock_posix.cc create mode 100644 webrtc/modules/desktop_capture/screen_drawer_lock_posix.h diff --git a/webrtc/modules/desktop_capture/BUILD.gn b/webrtc/modules/desktop_capture/BUILD.gn index caf27a364b..92d670c340 100644 --- a/webrtc/modules/desktop_capture/BUILD.gn +++ b/webrtc/modules/desktop_capture/BUILD.gn @@ -127,6 +127,8 @@ if (rtc_include_tests) { "screen_drawer.cc", "screen_drawer.h", "screen_drawer_linux.cc", + "screen_drawer_lock_posix.cc", + "screen_drawer_lock_posix.h", "screen_drawer_mac.cc", "screen_drawer_win.cc", ] diff --git a/webrtc/modules/desktop_capture/screen_drawer_linux.cc b/webrtc/modules/desktop_capture/screen_drawer_linux.cc index 9a9c20db47..fa2cfa4849 100644 --- a/webrtc/modules/desktop_capture/screen_drawer_linux.cc +++ b/webrtc/modules/desktop_capture/screen_drawer_linux.cc @@ -8,47 +8,22 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include -#include -#include #include #include #include #include "webrtc/modules/desktop_capture/screen_drawer.h" +#include "webrtc/modules/desktop_capture/screen_drawer_lock_posix.h" #include "webrtc/modules/desktop_capture/x11/shared_x_display.h" #include "webrtc/rtc_base/checks.h" +#include "webrtc/rtc_base/ptr_util.h" #include "webrtc/system_wrappers/include/sleep.h" namespace webrtc { namespace { -static constexpr char kSemaphoreName[] = - "/global-screen-drawer-linux-54fe5552-8047-11e6-a725-3f429a5b4fb4"; - -class ScreenDrawerLockLinux : public ScreenDrawerLock { - public: - ScreenDrawerLockLinux(); - ~ScreenDrawerLockLinux() override; - - private: - sem_t* semaphore_; -}; - -ScreenDrawerLockLinux::ScreenDrawerLockLinux() { - semaphore_ = - sem_open(kSemaphoreName, O_CREAT, S_IRWXU | S_IRWXG | S_IRWXO, 1); - sem_wait(semaphore_); -} - -ScreenDrawerLockLinux::~ScreenDrawerLockLinux() { - sem_post(semaphore_); - sem_close(semaphore_); - // sem_unlink(kSemaphoreName); -} - // A ScreenDrawer implementation for X11. class ScreenDrawerLinux : public ScreenDrawer { public: @@ -187,13 +162,13 @@ void ScreenDrawerLinux::BringToFront() { // static std::unique_ptr ScreenDrawerLock::Create() { - return std::unique_ptr(new ScreenDrawerLockLinux()); + return rtc::MakeUnique(); } // static std::unique_ptr ScreenDrawer::Create() { if (SharedXDisplay::CreateDefault().get()) { - return std::unique_ptr(new ScreenDrawerLinux()); + return rtc::MakeUnique(); } return nullptr; } diff --git a/webrtc/modules/desktop_capture/screen_drawer_lock_posix.cc b/webrtc/modules/desktop_capture/screen_drawer_lock_posix.cc new file mode 100644 index 0000000000..5b6bc9c0c1 --- /dev/null +++ b/webrtc/modules/desktop_capture/screen_drawer_lock_posix.cc @@ -0,0 +1,58 @@ +/* + * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "webrtc/modules/desktop_capture/screen_drawer_lock_posix.h" + +#include +#include + +#include "webrtc/rtc_base/checks.h" +#include "webrtc/rtc_base/logging.h" + +namespace webrtc { + +namespace { + +// A uuid as the name of semaphore. +static constexpr char kSemaphoreName[] = "GSDL54fe5552804711e6a7253f429a"; + +} // namespace + +ScreenDrawerLockPosix::ScreenDrawerLockPosix() + : ScreenDrawerLockPosix(kSemaphoreName) {} + +ScreenDrawerLockPosix::ScreenDrawerLockPosix(const char* name) { + semaphore_ = sem_open(name, O_CREAT, S_IRWXU | S_IRWXG | S_IRWXO, 1); + if (semaphore_ == SEM_FAILED) { + LOG_ERRNO(LS_ERROR) << "Failed to create named semaphore with " << name; + RTC_NOTREACHED(); + } + + sem_wait(semaphore_); +} + +ScreenDrawerLockPosix::~ScreenDrawerLockPosix() { + if (semaphore_ == SEM_FAILED) { + return; + } + + sem_post(semaphore_); + sem_close(semaphore_); + // sem_unlink a named semaphore won't wait until other clients to release the + // sem_t. So if a new process starts, it will sem_open a different kernel + // object with the same name and eventually breaks the cross-process lock. +} + +// static +void ScreenDrawerLockPosix::Unlink(const char* name) { + sem_unlink(name); +} + +} // namespace webrtc diff --git a/webrtc/modules/desktop_capture/screen_drawer_lock_posix.h b/webrtc/modules/desktop_capture/screen_drawer_lock_posix.h new file mode 100644 index 0000000000..a768066533 --- /dev/null +++ b/webrtc/modules/desktop_capture/screen_drawer_lock_posix.h @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LOCK_POSIX_H_ +#define WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LOCK_POSIX_H_ + +#include + +#include "webrtc/modules/desktop_capture/screen_drawer.h" + +namespace webrtc { + +class ScreenDrawerLockPosix final : public ScreenDrawerLock { + public: + ScreenDrawerLockPosix(); + // Provides a name other than the default one for test only. + explicit ScreenDrawerLockPosix(const char* name); + ~ScreenDrawerLockPosix() override; + + // Unlinks the named semaphore actively. This will remove the sem_t object in + // the system and allow others to create a different sem_t object with the + // same/ name. + static void Unlink(const char* name); + + private: + sem_t* semaphore_; +}; + +} // namespace webrtc + +#endif // WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_DRAWER_LOCK_POSIX_H_ diff --git a/webrtc/modules/desktop_capture/screen_drawer_mac.cc b/webrtc/modules/desktop_capture/screen_drawer_mac.cc index c84592923f..90f727124b 100644 --- a/webrtc/modules/desktop_capture/screen_drawer_mac.cc +++ b/webrtc/modules/desktop_capture/screen_drawer_mac.cc @@ -11,12 +11,14 @@ // TODO(zijiehe): Implement ScreenDrawerMac #include "webrtc/modules/desktop_capture/screen_drawer.h" +#include "webrtc/modules/desktop_capture/screen_drawer_lock_posix.h" +#include "webrtc/rtc_base/ptr_util.h" namespace webrtc { // static std::unique_ptr ScreenDrawerLock::Create() { - return nullptr; + return rtc::MakeUnique(); } // static diff --git a/webrtc/modules/desktop_capture/screen_drawer_unittest.cc b/webrtc/modules/desktop_capture/screen_drawer_unittest.cc index a99395b4df..94404ee6a1 100644 --- a/webrtc/modules/desktop_capture/screen_drawer_unittest.cc +++ b/webrtc/modules/desktop_capture/screen_drawer_unittest.cc @@ -10,16 +10,98 @@ #include "webrtc/modules/desktop_capture/screen_drawer.h" +#include #include +#include "webrtc/rtc_base/checks.h" +#include "webrtc/rtc_base/function_view.h" #include "webrtc/rtc_base/logging.h" #include "webrtc/rtc_base/random.h" +#include "webrtc/rtc_base/platform_thread.h" +#include "webrtc/rtc_base/ptr_util.h" #include "webrtc/rtc_base/timeutils.h" #include "webrtc/system_wrappers/include/sleep.h" #include "webrtc/test/gtest.h" +#if defined(WEBRTC_POSIX) +#include "webrtc/modules/desktop_capture/screen_drawer_lock_posix.h" +#endif + namespace webrtc { +namespace { + +void TestScreenDrawerLock( + rtc::FunctionView()> ctor) { + constexpr int kLockDurationMs = 100; + + RTC_DCHECK(ctor); + + std::atomic created(false); + std::atomic ready(false); + + class Task { + public: + Task(std::atomic* created, + const std::atomic& ready, + rtc::FunctionView()> ctor) + : created_(created), + ready_(ready), + ctor_(ctor) {} + + ~Task() = default; + + static void RunTask(void* me) { + Task* task = static_cast(me); + std::unique_ptr lock = task->ctor_(); + ASSERT_TRUE(!!lock); + task->created_->store(true); + // Wait for the main thread to get the signal of created_. + while (!task->ready_.load()) { + SleepMs(1); + } + // At this point, main thread should begin to create a second lock. Though + // it's still possible the second lock won't be created before the + // following sleep has been finished, the possibility will be + // significantly reduced. + const int64_t current_ms = rtc::TimeMillis(); + // SleepMs() may return early. See + // https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/include/sleep.h?rcl=4a604c80cecce18aff6fc5e16296d04675312d83&l=20 + // But we need to ensure at least 100 ms has been passed before unlocking + // |lock|. + while (rtc::TimeMillis() - current_ms < kLockDurationMs) { + SleepMs(kLockDurationMs - (rtc::TimeMillis() - current_ms)); + } + } + + private: + std::atomic* const created_; + const std::atomic& ready_; + const rtc::FunctionView()> ctor_; + } task(&created, ready, ctor); + + rtc::PlatformThread lock_thread(&Task::RunTask, &task, "lock_thread"); + lock_thread.Start(); + + // Wait for the first lock in Task::RunTask() to be created. + // TODO(zijiehe): Find a better solution to wait for the creation of the first + // lock. See https://chromium-review.googlesource.com/c/607688/13/webrtc/modules/desktop_capture/screen_drawer_unittest.cc + while (!created.load()) { + SleepMs(1); + } + + const int64_t start_ms = rtc::TimeMillis(); + ready.store(true); + // This is unlikely to fail, but just in case current thread is too laggy and + // cause the SleepMs() in RunTask() to finish before we creating another lock. + ASSERT_GT(kLockDurationMs, rtc::TimeMillis() - start_ms); + ctor(); + ASSERT_LE(kLockDurationMs, rtc::TimeMillis() - start_ms); + lock_thread.Stop(); +} + +} // namespace + // These are a set of manual test cases, as we do not have an automatical way to // detect whether a ScreenDrawer on a certain platform works well without // ScreenCapturer(s). So you may execute these test cases with @@ -57,4 +139,21 @@ TEST(ScreenDrawerTest, DISABLED_DrawRectangles) { SleepMs(10000); } +TEST(ScreenDrawerTest, TwoScreenDrawerLocks) { +#if defined(WEBRTC_POSIX) + // ScreenDrawerLockPosix won't be able to unlink the named semaphore. So use a + // different semaphore name here to avoid deadlock. + const char* semaphore_name = "GSDL8784541a812011e788ff67427b"; + ScreenDrawerLockPosix::Unlink(semaphore_name); + + TestScreenDrawerLock([semaphore_name]() { + return rtc::MakeUnique(semaphore_name); + }); +#elif defined(WEBRTC_WIN) + TestScreenDrawerLock([]() { + return ScreenDrawerLock::Create(); + }); +#endif +} + } // namespace webrtc