From 6a4607e1006560ce598f1e1d79fd120308b9e67f Mon Sep 17 00:00:00 2001 From: zijiehe Date: Tue, 18 Oct 2016 18:22:18 -0700 Subject: [PATCH] Deflaky ScreenCapturerTest ScreenCapturer tests may fail on trybot, so this change is to fix the issue. Changes include, 1. Sometimes, a capturer may capture part of the change, i.e. usually the draw actions are not atomic. So the updated_region may be inaccurate. So I have added a MayDrawIncompleteShapes() function in ScreenDrawer. If it returns false, the updated_region check will be ignored. 2. Several test cases may run concurrently, which makes one ScreenDrawer won't really work. Its window may be covered by another ScreenDrawer. So I have added a system wide lock to ensure only one ScreenDrawer is working at a certain time. 3. On unity (Linux), the top several pixels of a window may be covered by a shadow effect if the window is not focused. So I have added a BringToFront() function, and call it in WaitForPendingDraws(). 4. On Windows, the drawn shapes are 'temporary drawing', which will be erased once the window is covered by another one. So I repeat DrawRectangle() function call in the test case. TODO(zijiehe): The DISABLED_ prefixes will be added back after the code review. And I will move these test cases into modules_test in a coming change. BUG=647067 Review-Url: https://codereview.webrtc.org/2337073007 Cr-Commit-Position: refs/heads/master@{#14674} --- webrtc/modules/BUILD.gn | 1 + .../screen_capturer_unittest.cc | 42 ++++++++---- .../modules/desktop_capture/screen_drawer.cc | 30 +++++++++ .../modules/desktop_capture/screen_drawer.h | 25 ++++++- .../desktop_capture/screen_drawer_linux.cc | 66 +++++++++++++++++++ .../desktop_capture/screen_drawer_mac.cc | 5 ++ .../desktop_capture/screen_drawer_win.cc | 60 +++++++++++++++++ 7 files changed, 214 insertions(+), 15 deletions(-) create mode 100644 webrtc/modules/desktop_capture/screen_drawer.cc diff --git a/webrtc/modules/BUILD.gn b/webrtc/modules/BUILD.gn index 17da35dff4..d983360d9b 100644 --- a/webrtc/modules/BUILD.gn +++ b/webrtc/modules/BUILD.gn @@ -551,6 +551,7 @@ if (rtc_include_tests) { "desktop_capture/screen_capturer_mac_unittest.cc", "desktop_capture/screen_capturer_mock_objects.h", "desktop_capture/screen_capturer_unittest.cc", + "desktop_capture/screen_drawer.cc", "desktop_capture/screen_drawer.h", "desktop_capture/screen_drawer_linux.cc", "desktop_capture/screen_drawer_mac.cc", diff --git a/webrtc/modules/desktop_capture/screen_capturer_unittest.cc b/webrtc/modules/desktop_capture/screen_capturer_unittest.cc index ce0cab1fd3..ce18812fd5 100644 --- a/webrtc/modules/desktop_capture/screen_capturer_unittest.cc +++ b/webrtc/modules/desktop_capture/screen_capturer_unittest.cc @@ -51,12 +51,15 @@ ACTION_P(SaveUniquePtrArg, dest) { // Returns true if color in |rect| of |frame| is |color|. bool ArePixelsColoredBy(const DesktopFrame& frame, DesktopRect rect, - RgbaColor color) { - // updated_region() should cover the painted area. - DesktopRegion updated_region(frame.updated_region()); - updated_region.IntersectWith(rect); - if (!updated_region.Equals(DesktopRegion(rect))) { - return false; + RgbaColor color, + bool may_partially_draw) { + if (!may_partially_draw) { + // updated_region() should cover the painted area. + DesktopRegion updated_region(frame.updated_region()); + updated_region.IntersectWith(rect); + if (!updated_region.Equals(DesktopRegion(rect))) { + return false; + } } // Color in the |rect| should be |color|. @@ -107,18 +110,28 @@ class ScreenCapturerTest : public testing::Test { capturer->Start(&callback_); } - // Draw a set of |kRectSize| by |kRectSize| rectangles at (|i|, |i|). One of - // (controlled by |c|) its primary colors is |i|, and the other two are - // 0xff. So we won't draw a white rectangle. + // Draw a set of |kRectSize| by |kRectSize| rectangles at (|i|, |i|), or + // |i| by |i| rectangles at (|kRectSize|, |kRectSize|). One of (controlled + // by |c|) its primary colors is |i|, and the other two are 0x7f. So we + // won't draw a black or white rectangle. for (int c = 0; c < 3; c++) { + // A fixed size rectangle. for (int i = 0; i < kTestArea - kRectSize; i += 16) { DesktopRect rect = DesktopRect::MakeXYWH(i, i, kRectSize, kRectSize); rect.Translate(drawer->DrawableRegion().top_left()); RgbaColor color((c == 0 ? (i & 0xff) : 0x7f), (c == 1 ? (i & 0xff) : 0x7f), (c == 2 ? (i & 0xff) : 0x7f)); - drawer->Clear(); - drawer->DrawRectangle(rect, color); + TestCaptureOneFrame(capturers, drawer.get(), rect, color); + } + + // A variable-size rectangle. + for (int i = 0; i < kTestArea - kRectSize; i += 16) { + DesktopRect rect = DesktopRect::MakeXYWH(kRectSize, kRectSize, i, i); + rect.Translate(drawer->DrawableRegion().top_left()); + RgbaColor color((c == 0 ? (i & 0xff) : 0x7f), + (c == 1 ? (i & 0xff) : 0x7f), + (c == 2 ? (i & 0xff) : 0x7f)); TestCaptureOneFrame(capturers, drawer.get(), rect, color); } } @@ -166,9 +179,11 @@ class ScreenCapturerTest : public testing::Test { ScreenDrawer* drawer, DesktopRect rect, RgbaColor color) { - size_t succeeded_capturers = 0; const int wait_capture_round = 600; + drawer->Clear(); + size_t succeeded_capturers = 0; for (int i = 0; i < wait_capture_round; i++) { + drawer->DrawRectangle(rect, color); drawer->WaitForPendingDraws(); for (size_t j = 0; j < capturers.size(); j++) { if (capturers[j] == nullptr) { @@ -184,7 +199,8 @@ class ScreenCapturerTest : public testing::Test { return; } - if (ArePixelsColoredBy(*frame, rect, color)) { + if (ArePixelsColoredBy( + *frame, rect, color, drawer->MayDrawIncompleteShapes())) { capturers[j] = nullptr; succeeded_capturers++; } diff --git a/webrtc/modules/desktop_capture/screen_drawer.cc b/webrtc/modules/desktop_capture/screen_drawer.cc new file mode 100644 index 0000000000..f1209edc54 --- /dev/null +++ b/webrtc/modules/desktop_capture/screen_drawer.cc @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2016 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.h" + +namespace webrtc { + +namespace { +std::unique_ptr g_screen_drawer_lock; +} // namespace + +ScreenDrawerLock::ScreenDrawerLock() = default; +ScreenDrawerLock::~ScreenDrawerLock() = default; + +ScreenDrawer::ScreenDrawer() { + g_screen_drawer_lock = ScreenDrawerLock::Create(); +} + +ScreenDrawer::~ScreenDrawer() { + g_screen_drawer_lock.reset(); +} + +} // namespace webrtc diff --git a/webrtc/modules/desktop_capture/screen_drawer.h b/webrtc/modules/desktop_capture/screen_drawer.h index d7ec5d186c..cde9a2e50e 100644 --- a/webrtc/modules/desktop_capture/screen_drawer.h +++ b/webrtc/modules/desktop_capture/screen_drawer.h @@ -20,16 +20,31 @@ namespace webrtc { +// A cross-process lock to ensure only one ScreenDrawer can be used at a certain +// time. +class ScreenDrawerLock { + public: + virtual ~ScreenDrawerLock(); + + static std::unique_ptr Create(); + + protected: + ScreenDrawerLock(); +}; + // A set of basic platform dependent functions to draw various shapes on the // screen. class ScreenDrawer { public: // Creates a ScreenDrawer for the current platform, returns nullptr if no // ScreenDrawer implementation available. + // If the implementation cannot guarantee two ScreenDrawer instances won't + // impact each other, this function may block current thread until another + // ScreenDrawer has been destroyed. static std::unique_ptr Create(); - ScreenDrawer() {} - virtual ~ScreenDrawer() {} + ScreenDrawer(); + virtual ~ScreenDrawer(); // Returns the region inside which DrawRectangle() function are expected to // work, in capturer coordinates (assuming ScreenCapturer::SelectScreen has @@ -49,6 +64,12 @@ class ScreenDrawer { // ScreenCapturer should be able to capture the changes after this function // finish. virtual void WaitForPendingDraws() = 0; + + // Returns true if incomplete shapes previous actions required may be drawn on + // the screen after a WaitForPendingDraws() call. i.e. Though the complete + // shapes will eventually be drawn on the screen, due to some OS limitations, + // these shapes may be partially appeared sometimes. + virtual bool MayDrawIncompleteShapes() = 0; }; } // namespace webrtc diff --git a/webrtc/modules/desktop_capture/screen_drawer_linux.cc b/webrtc/modules/desktop_capture/screen_drawer_linux.cc index 08d8195b42..c745205df0 100644 --- a/webrtc/modules/desktop_capture/screen_drawer_linux.cc +++ b/webrtc/modules/desktop_capture/screen_drawer_linux.cc @@ -8,6 +8,11 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include +#include +#include +#include + #include #include "webrtc/base/checks.h" @@ -19,6 +24,30 @@ namespace webrtc { namespace { +static constexpr char kSemaphoreName[] = + "/global-screen-drawer-linux-54fe5552-8047-11e6-a725-3f429a5b4fb4"; + +class ScreenDrawerLockLinux : public ScreenDrawerLock { + public: + ScreenDrawerLockLinux(); + ~ScreenDrawerLockLinux(); + + 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: @@ -30,8 +59,13 @@ class ScreenDrawerLinux : public ScreenDrawer { void DrawRectangle(DesktopRect rect, RgbaColor color) override; void Clear() override; void WaitForPendingDraws() override; + bool MayDrawIncompleteShapes() override; private: + // Bring the window to the front, this can help to avoid the impact from other + // windows or shadow effect. + void BringToFront(); + rtc::scoped_refptr display_; int screen_num_; DesktopRect rect_; @@ -80,6 +114,7 @@ ScreenDrawerLinux::ScreenDrawerLinux() { root_attributes.height); context_ = DefaultGC(display_->display(), screen_num_); colormap_ = DefaultColormap(display_->display(), screen_num_); + BringToFront(); // Wait for window animations. SleepMs(200); } @@ -121,8 +156,39 @@ void ScreenDrawerLinux::WaitForPendingDraws() { SleepMs(50); } +bool ScreenDrawerLinux::MayDrawIncompleteShapes() { + return true; +} + +void ScreenDrawerLinux::BringToFront() { + Atom state_above = XInternAtom(display_->display(), "_NET_WM_STATE_ABOVE", 1); + Atom window_state = XInternAtom(display_->display(), "_NET_WM_STATE", 1); + if (state_above == None || window_state == None) { + // Fallback to use XRaiseWindow, it's not reliable if two windows are both + // raise itself to the top. + XRaiseWindow(display_->display(), window_); + return; + } + + XEvent event; + memset(&event, 0, sizeof(event)); + event.type = ClientMessage; + event.xclient.window = window_; + event.xclient.message_type = window_state; + event.xclient.format = 32; + event.xclient.data.l[0] = 1; // _NET_WM_STATE_ADD + event.xclient.data.l[1] = state_above; + XSendEvent(display_->display(), RootWindow(display_->display(), screen_num_), + False, SubstructureRedirectMask | SubstructureNotifyMask, &event); +} + } // namespace +// static +std::unique_ptr ScreenDrawerLock::Create() { + return std::unique_ptr(new ScreenDrawerLockLinux()); +} + // static std::unique_ptr ScreenDrawer::Create() { if (SharedXDisplay::CreateDefault().get()) { diff --git a/webrtc/modules/desktop_capture/screen_drawer_mac.cc b/webrtc/modules/desktop_capture/screen_drawer_mac.cc index 1d0437b5a1..c84592923f 100644 --- a/webrtc/modules/desktop_capture/screen_drawer_mac.cc +++ b/webrtc/modules/desktop_capture/screen_drawer_mac.cc @@ -14,6 +14,11 @@ namespace webrtc { +// static +std::unique_ptr ScreenDrawerLock::Create() { + return nullptr; +} + // static std::unique_ptr ScreenDrawer::Create() { return nullptr; diff --git a/webrtc/modules/desktop_capture/screen_drawer_win.cc b/webrtc/modules/desktop_capture/screen_drawer_win.cc index 48b770d14b..0b66991647 100644 --- a/webrtc/modules/desktop_capture/screen_drawer_win.cc +++ b/webrtc/modules/desktop_capture/screen_drawer_win.cc @@ -19,6 +19,36 @@ namespace webrtc { namespace { +static constexpr TCHAR kMutexName[] = + TEXT("Local\\ScreenDrawerWin-da834f82-8044-11e6-ac81-73dcdd1c1869"); + +class ScreenDrawerLockWin : public ScreenDrawerLock { + public: + ScreenDrawerLockWin(); + ~ScreenDrawerLockWin(); + + private: + HANDLE mutex_; +}; + +ScreenDrawerLockWin::ScreenDrawerLockWin() { + while (true) { + mutex_ = CreateMutex(NULL, FALSE, kMutexName); + if (GetLastError() != ERROR_ALREADY_EXISTS && mutex_ != NULL) { + break; + } else { + if (mutex_) { + CloseHandle(mutex_); + } + SleepMs(1000); + } + } +} + +ScreenDrawerLockWin::~ScreenDrawerLockWin() { + CloseHandle(mutex_); +} + DesktopRect GetScreenRect() { HDC hdc = GetDC(NULL); DesktopRect rect = DesktopRect::MakeWH(GetDeviceCaps(hdc, HORZRES), @@ -51,8 +81,13 @@ class ScreenDrawerWin : public ScreenDrawer { void DrawRectangle(DesktopRect rect, RgbaColor color) override; void Clear() override; void WaitForPendingDraws() override; + bool MayDrawIncompleteShapes() override; private: + // Bring the window to the front, this can help to avoid the impact from other + // windows or shadow effects. + void BringToFront(); + // Draw a line with |color|. void DrawLine(DesktopVector start, DesktopVector end, RgbaColor color); @@ -76,6 +111,7 @@ ScreenDrawerWin::ScreenDrawerWin() // Always use stock pen (DC_PEN) and brush (DC_BRUSH). SelectObject(hdc_, GetStockObject(DC_PEN)); SelectObject(hdc_, GetStockObject(DC_BRUSH)); + BringToFront(); } ScreenDrawerWin::~ScreenDrawerWin() { @@ -117,6 +153,10 @@ void ScreenDrawerWin::WaitForPendingDraws() { SleepMs(50); } +bool ScreenDrawerWin::MayDrawIncompleteShapes() { + return true; +} + void ScreenDrawerWin::DrawLine(DesktopVector start, DesktopVector end, RgbaColor color) { @@ -133,8 +173,28 @@ void ScreenDrawerWin::DrawDot(DesktopVector vect, RgbaColor color) { SetPixel(hdc_, vect.x(), vect.y(), ColorToRef(color)); } +void ScreenDrawerWin::BringToFront() { + if (SUCCEEDED(SetWindowPos( + window_, HWND_TOPMOST, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE))) { + return; + } + + long ex_style = GetWindowLong(window_, GWL_EXSTYLE); + ex_style |= WS_EX_TOPMOST; + if (SetWindowLong(window_, GWL_EXSTYLE, ex_style) != 0) { + return; + } + + BringWindowToTop(window_); +} + } // namespace +// static +std::unique_ptr ScreenDrawerLock::Create() { + return std::unique_ptr(new ScreenDrawerLockWin()); +} + // static std::unique_ptr ScreenDrawer::Create() { return std::unique_ptr(new ScreenDrawerWin());