[WebRTC] Add TwoCapturers test and TwoMagnifierCapturers 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 a9297584d93def2cd9ffaf7979b81fbc3c26b241..8db838dac9a86d40ec8771dfbc4c80746803d5ea 100644
--- a/webrtc/modules/desktop_capture/screen_capturer_unittest.cc
+++ b/webrtc/modules/desktop_capture/screen_capturer_unittest.cc
@@ -130,37 +130,34 @@ class ScreenCapturerTest : public testing::Test {
         drawer->Clear();
         drawer->DrawRectangle(rect, color);
 
-        const int wait_first_capture_round = 20;
-        for (int j = 0; j < wait_first_capture_round; j++) {
+        std::vector<ScreenCapturer*> test_capturers(capturers);
+        size_t succeeded_capturers = 0;
+        const int wait_capture_round = 20;
+        for (int j = 0; j < wait_capture_round; j++) {

[Sergey Ulanov, 2016/09/09 17:23:15]

Maybe move this code to a separate function to make it easier to parse? There
are 4 nested for loops here, which makes it hard to understand what's going on.

[Hzj_jie, 2016/09/12 23:13:41]

Sure.

           drawer->WaitForPendingDraws();
-          std::unique_ptr<DesktopFrame> frame =
-              CaptureFrame(*capturers.begin(), &callback_);
-          if (!frame) {
-            return;
+          for (size_t k = 0; k < test_capturers.size(); k++) {
+            if (test_capturers[k] == nullptr) {
+              // TODO(zijiehe): ScreenCapturerX11 and ScreenCapturerWinGdi
+              // cannot capture a correct frame again if screen does not update.

[Sergey Ulanov, 2016/09/09 17:23:14]

I don't understand this comment. All capturers should be capture a new frame
even if nothing has changed. They will just keep updated_region() empty.
Also TODO() normally should say what needs to be done. 

[Hzj_jie, 2016/09/12 23:13:40]

Emmm, updated.

+              continue;
+            }
+            std::unique_ptr<DesktopFrame> frame =
+                CaptureFrame(test_capturers[k], &callback_);
+            if (!frame) {
+              return;

[Sergey Ulanov, 2016/09/09 17:23:15]

Is it appropriate to just return if one of the capturer fails? It means that the
test will pass, and this doesn't look right to me.

[Hzj_jie, 2016/09/12 23:13:41]

Not really. CaptureFrame triggers an assertion failure. But I should add comment
to explain, I remember Jamie asked same question before.

+            }
+
+            if (ArePixelsColoredBy(*frame, rect, color)) {
+              test_capturers[k] = nullptr;
+              succeeded_capturers++;
+            }
           }
 
-          if (ArePixelsColoredBy(*frame, rect, color)) {
-            // The first capturer successfully captured the frame we expected.
-            // So the others should also be able to capture it.
+          if (succeeded_capturers == test_capturers.size()) {
             break;
-          } else {
-            ASSERT_LT(j, wait_first_capture_round);
-          }
-        }
-
-        for (ScreenCapturer* capturer : capturers) {
-          if (capturer == *capturers.begin()) {
-            // TODO(zijiehe): ScreenCapturerX11 and ScreenCapturerWinGdi cannot
-            // capture a correct frame again if screen does not update.
-            continue;
-          }
-          std::unique_ptr<DesktopFrame> frame =
-              CaptureFrame(capturer, &callback_);
-          if (!frame) {
-            return;
           }
 
-          ASSERT_TRUE(ArePixelsColoredBy(*frame, rect, color));
+          ASSERT_LT(j, wait_capture_round);
         }
       }
     }
@@ -182,6 +179,12 @@ class ScreenCapturerTest : public testing::Test {
     capturer_.reset(ScreenCapturer::Create(options));
     return true;
   }
+
+  void SetMagnifierMode() {
+    DesktopCaptureOptions options(DesktopCaptureOptions::CreateDefault());
+    options.set_allow_use_magnification_api(true);
+    capturer_.reset(ScreenCapturer::Create(options));

[Sergey Ulanov, 2016/09/09 17:23:15]

It doesn't look right that a method called Set*() create a screen capturer.
Rename it to CreateMagnifierCapturer()?

[Hzj_jie, 2016/09/12 23:13:40]

Done.

+  }
 #endif  // defined(WEBRTC_WIN)
 
   std::unique_ptr<ScreenCapturer> capturer_;
@@ -259,6 +262,12 @@ TEST_F(ScreenCapturerTest, CaptureUpdatedRegion) {
   TestCaptureUpdatedRegion();
 }
 
+TEST_F(ScreenCapturerTest, TwoCapturers) {
+  std::unique_ptr<ScreenCapturer> capturer2(capturer_.release());
+  SetUp();
+  TestCaptureUpdatedRegion({capturer_.get(), capturer2.get()});
+}
+
 #if defined(WEBRTC_WIN)
 
 TEST_F(ScreenCapturerTest, UseSharedBuffers) {
@@ -278,9 +287,7 @@ TEST_F(ScreenCapturerTest, UseSharedBuffers) {
 }
 
 TEST_F(ScreenCapturerTest, UseMagnifier) {
-  DesktopCaptureOptions options(DesktopCaptureOptions::CreateDefault());
-  options.set_allow_use_magnification_api(true);
-  capturer_.reset(ScreenCapturer::Create(options));
+  SetMagnifierMode();
 
   std::unique_ptr<DesktopFrame> frame;
   EXPECT_CALL(callback_,
@@ -344,6 +351,13 @@ TEST_F(ScreenCapturerTest, TwoDirectxCapturers) {
   TestCaptureUpdatedRegion({capturer_.get(), capturer2.get()});
 }
 
+TEST_F(ScreenCapturerTest, TwoMagnifierCapturers) {
+  SetMagnifierMode();
+  std::unique_ptr<ScreenCapturer> capturer2(capturer_.release());

[Sergey Ulanov, 2016/09/09 17:23:15]

capturer2 = std::move(capturer_);

[Hzj_jie, 2016/09/12 23:13:41]

Done.

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