[WebRTC] A real ScreenCapturer test

webrtc/modules/desktop_capture/screen_capturer_unittest.cc

Index: webrtc/modules/desktop_capture/screen_capturer_unittest.cc
diff --git a/webrtc/modules/desktop_capture/screen_capturer_unittest.cc b/webrtc/modules/desktop_capture/screen_capturer_unittest.cc
index 6d2c0eb0658221f98ebb4f34d18a22076de8521a..99ccf1c5759e80afe6b89bdb76dcd35659f8f350 100644
--- a/webrtc/modules/desktop_capture/screen_capturer_unittest.cc
+++ b/webrtc/modules/desktop_capture/screen_capturer_unittest.cc
@@ -8,8 +8,12 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <string.h>
+
+#include <algorithm>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "webrtc/modules/desktop_capture/screen_capturer.h"
 
@@ -17,10 +21,13 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/modules/desktop_capture/color.h"
 #include "webrtc/modules/desktop_capture/desktop_capture_options.h"
 #include "webrtc/modules/desktop_capture/desktop_frame.h"
 #include "webrtc/modules/desktop_capture/desktop_region.h"
 #include "webrtc/modules/desktop_capture/screen_capturer_mock_objects.h"
+#include "webrtc/modules/desktop_capture/screen_drawer.h"
+#include "webrtc/system_wrappers/include/sleep.h"
 
 #if defined(WEBRTC_WIN)
 #include "webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h"
@@ -34,6 +41,48 @@ const int kTestSharedMemoryId = 123;
 
 namespace webrtc {
 
+namespace {
+
+ACTION_P(SaveUniquePtrArg, dest) {
+  *dest = std::move(*arg1);
+}
+
+// Expects |capturer| to successfully capture a frame, and returns it.
+std::unique_ptr<DesktopFrame> CaptureFrame(
+    ScreenCapturer* capturer,
+    MockScreenCapturerCallback* callback) {
+  std::unique_ptr<DesktopFrame> frame;
+  EXPECT_CALL(*callback,
+              OnCaptureResultPtr(DesktopCapturer::Result::SUCCESS, _))
+      .WillOnce(SaveUniquePtrArg(&frame));
+  capturer->Capture(DesktopRegion());
+  EXPECT_TRUE(frame);
+  return frame;
+}
+
+// Expects color in |rect| of |frame| is |color|.
+void ExpectPixelsAreColoredBy(const DesktopFrame& frame,
+                              DesktopRect rect,
+                              Color color) {
+  // updated_region() should cover the painted area.
+  DesktopRegion updated_region(frame.updated_region());
+  updated_region.IntersectWith(rect);
+  ASSERT_TRUE(updated_region.Equals(DesktopRegion(rect)));

[Jamie, 2016/08/26 22:29:09]

It looks like this test will pass if updated_region is larger than expected. If
that's deliberate, it should be called out explicitly, though I think a better
goal long-term would to have capturers return only the region that has changed
and control the test environment tightly enough that other on-screen changes
such as a clock are not possible.

[Hzj_jie, 2016/08/29 21:57:28]

Yes, currently it's expected. Before change 2202443002 has been submitted, we
cannot guarantee all ScreenCapturer implementations return only the updated
region. (At least DirectX capturer won't.)

After all ScreenCapturer implementations are wrapped by
ScreenCapturerDifferWrapper (in 2202443002), we can expect the changes in
ScreenDrawer::DrawableRegion are all controllable. Then we won't need to take
care about clock or other areas, they should cover only a very small part of the
screen. From the point of view of probability, it's not likely the bug happens
only in these small areas.

[Jamie, 2016/08/31 17:39:39]

So will you update this test to be more exact once CL 2202443002 has landed, or
will it still be too difficult to ensure that no genuine changes occur outside
the expected area?

[Hzj_jie, 2016/08/31 21:22:39]

Yes, all the area covered by ScreenDrawer::DrawableRegion() should be fully
controllable.

+
+  // Color in the |rect| should be |color|.
+  uint8_t* row = frame.GetFrameDataAtPos(rect.top_left());
+  for (int i = 0; i < rect.height(); i++) {
+    uint8_t* column = row;
+    for (int j = 0; j < rect.width(); j++) {
+      ASSERT_EQ(color, column);
+      column += DesktopFrame::kBytesPerPixel;
+    }
+    row += frame.stride();
+  }
+}
+
+}  // namespace
+
 class ScreenCapturerTest : public testing::Test {
  public:
   void SetUp() override {
@@ -42,6 +91,80 @@ class ScreenCapturerTest : public testing::Test {
   }
 
  protected:
+  void TestCaptureUpdatedRegion(
+      std::vector<ScreenCapturer*> capturers = std::vector<ScreenCapturer*>()) {
+    // Always test capturer_.
+    capturers.push_back(capturer_.get());

[Jamie, 2016/08/26 22:29:10]

I think the code would be cleaner if you explicitly passed in the capturers to
be tested. If nothing else, you wouldn't need a default value for the parameter,
which I think is discouraged by the style guide.

[Hzj_jie, 2016/08/29 21:57:28]

Done.

+    // A large enough area for the tests, which should be able to fulfill by
+    // most of systems.
+    const int kTestArea = 512;
+    const int kRectSize = 32;
+    std::unique_ptr<ScreenDrawer> drawer = ScreenDrawer::Create();
+    if (!drawer || drawer->DrawableRegion().is_empty()) {
+      LOG(LS_WARNING) << "No ScreenDrawer implementation for current platform.";
+      return;
+    }
+    if (drawer->DrawableRegion().width() < kTestArea ||
+        drawer->DrawableRegion().height() < kTestArea) {
+      LOG(LS_WARNING) << "ScreenDrawer::DrawableRegion() is too small for the "
+                         "CaptureUpdatedRegion tests.";
+      return;
+    }
+    for (ScreenCapturer* capturer : capturers) {
+      capturer->Start(&callback_);
+    }
+#if defined(WEBRTC_LINUX)
+    // TODO(zijiehe): ScreenCapturerX11 won't be able to capture correct images
+    // in the first several capture attempts.
+    for (int i = 0; i < 10; i++) {
+      for (ScreenCapturer* capturer : capturers) {
+        std::unique_ptr<DesktopFrame> frame =
+            CaptureFrame(capturer, &callback_);
+        if (!frame) {
+          return;
+        }
+      }
+    }
+#endif
+
+    for (int c = 0; c < 3; c++) {
+      for (int i = 0; i < kTestArea - kRectSize; i += 16) {
+        DesktopRect rect = DesktopRect::MakeXYWH(i, i, kRectSize, kRectSize);
+        Color color = Color::FromBGR((c == 0 ? (i & 0xff) : 0x7f),
+                                     (c == 1 ? (i & 0xff) : 0x7f),
+                                     (c == 2 ? (i & 0xff) : 0x7f));
+        drawer->Clear();
+        drawer->DrawRectangle(rect, color);
+        drawer->WaitForPendingPaintings();
+        rect.Translate(drawer->DrawableRegion().top_left());

[Jamie, 2016/08/26 22:29:10]

This line goes away if you use the same frame-of-reference.

[Hzj_jie, 2016/08/29 21:57:28]

Done.

+
+        for (ScreenCapturer* capturer : capturers) {
+          std::unique_ptr<DesktopFrame> frame =
+              CaptureFrame(capturer, &callback_);
+          if (!frame) {
+            return;

[Jamie, 2016/08/26 22:29:10]

Shouldn't this be a test failure?

[Hzj_jie, 2016/08/29 21:57:28]

The assertion has been added in CaptureFrame already.

+          }
+
+          ExpectPixelsAreColoredBy(*frame, rect, color);
+        }
+      }
+    }
+  }
+
+#if defined(WEBRTC_WIN)
+  bool UseDirectxCapturer() {

[Jamie, 2016/08/26 22:29:10]

Since this returns a bool, it's not clear that it's a setter rather than a
getter. I suggest calling it something like SetDirectxCaptureMode().

[Hzj_jie, 2016/08/29 21:57:28]

Done.

+    if (!ScreenCapturerWinDirectx::IsSupported()) {
+      LOG(LS_WARNING) << "Directx capturer is not supported";
+      return false;
+    }
+
+    DesktopCaptureOptions options(DesktopCaptureOptions::CreateDefault());
+    options.set_allow_directx_capturer(true);
+    capturer_.reset(ScreenCapturer::Create(options));
+    return true;
+  }
+#endif  // defined(WEBRTC_WIN)
+
   std::unique_ptr<ScreenCapturer> capturer_;
   MockScreenCapturerCallback callback_;
 };
@@ -74,10 +197,6 @@ class FakeSharedMemoryFactory : public SharedMemoryFactory {
   RTC_DISALLOW_COPY_AND_ASSIGN(FakeSharedMemoryFactory);
 };
 
-ACTION_P(SaveUniquePtrArg, dest) {
-  *dest = std::move(*arg1);
-}
-
 TEST_F(ScreenCapturerTest, GetScreenListAndSelectScreen) {
   webrtc::ScreenCapturer::ScreenList screens;
   EXPECT_TRUE(capturer_->GetScreenList(&screens));
@@ -117,6 +236,10 @@ TEST_F(ScreenCapturerTest, Capture) {
   EXPECT_TRUE(it.IsAtEnd());
 }
 
+TEST_F(ScreenCapturerTest, CaptureUpdatedRegion) {
+  TestCaptureUpdatedRegion();
+}
+
 #if defined(WEBRTC_WIN)
 
 TEST_F(ScreenCapturerTest, UseSharedBuffers) {
@@ -151,15 +274,10 @@ TEST_F(ScreenCapturerTest, UseMagnifier) {
 }
 
 TEST_F(ScreenCapturerTest, UseDirectxCapturer) {
-  if (!ScreenCapturerWinDirectx::IsSupported()) {
-    LOG(LS_WARNING) << "Directx capturer is not supported";
+  if (!UseDirectxCapturer()) {
     return;
   }
 
-  DesktopCaptureOptions options(DesktopCaptureOptions::CreateDefault());
-  options.set_allow_directx_capturer(true);
-  capturer_.reset(ScreenCapturer::Create(options));
-
   std::unique_ptr<DesktopFrame> frame;
   EXPECT_CALL(callback_,
               OnCaptureResultPtr(DesktopCapturer::Result::SUCCESS, _))
@@ -171,15 +289,10 @@ TEST_F(ScreenCapturerTest, UseDirectxCapturer) {
 }
 
 TEST_F(ScreenCapturerTest, UseDirectxCapturerWithSharedBuffers) {
-  if (!ScreenCapturerWinDirectx::IsSupported()) {
-    LOG(LS_WARNING) << "Directx capturer is not supported";
+  if (!UseDirectxCapturer()) {
     return;
   }
 
-  DesktopCaptureOptions options(DesktopCaptureOptions::CreateDefault());
-  options.set_allow_directx_capturer(true);
-  capturer_.reset(ScreenCapturer::Create(options));
-
   std::unique_ptr<DesktopFrame> frame;
   EXPECT_CALL(callback_,
               OnCaptureResultPtr(DesktopCapturer::Result::SUCCESS, _))
@@ -194,6 +307,26 @@ TEST_F(ScreenCapturerTest, UseDirectxCapturerWithSharedBuffers) {
   EXPECT_EQ(frame->shared_memory()->id(), kTestSharedMemoryId);
 }
 
+TEST_F(ScreenCapturerTest, CaptureUpdatedRegionWithDirectxCapturer) {
+  if (!UseDirectxCapturer()) {
+    return;
+  }
+
+  TestCaptureUpdatedRegion();
+}
+
+TEST_F(ScreenCapturerTest, TwoDirectxCapturers) {
+  if (!UseDirectxCapturer()) {
+    return;
+  }
+
+  DesktopCaptureOptions options(DesktopCaptureOptions::CreateDefault());
+  options.set_allow_directx_capturer(true);
+  std::unique_ptr<ScreenCapturer> capturer2(ScreenCapturer::Create(options));

[Jamie, 2016/08/26 22:29:10]

You could instead take ownership of capturer_ and call SetDirectxCaptureMode to
create a second one.

[Hzj_jie, 2016/08/29 21:57:28]

Done.

+
+  TestCaptureUpdatedRegion({capturer2.get()});
+}
+
 #endif  // defined(WEBRTC_WIN)
 
 }  // namespace webrtc