Implement all resolution change policies for desktop and tab capture.

content/browser/media/capture/desktop_capture_device_unittest.cc

Index: content/browser/media/capture/desktop_capture_device_unittest.cc
diff --git a/content/browser/media/capture/desktop_capture_device_unittest.cc b/content/browser/media/capture/desktop_capture_device_unittest.cc
index c86ddd3857ec569b9c6a11a54a3d2845b621ecdd..67d37e16335953be97da889386f0b81050bc08df 100644
--- a/content/browser/media/capture/desktop_capture_device_unittest.cc
+++ b/content/browser/media/capture/desktop_capture_device_unittest.cc
@@ -4,7 +4,9 @@
 
 #include "content/browser/media/capture/desktop_capture_device.h"
 
+#include <algorithm>
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/synchronization/waitable_event.h"
@@ -22,8 +24,10 @@ using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::DoAll;
 using ::testing::Expectation;
+using ::testing::Invoke;
 using ::testing::InvokeWithoutArgs;
 using ::testing::SaveArg;
+using ::testing::WithArg;
 
 namespace content {
 
@@ -33,10 +37,10 @@ MATCHER_P2(EqualsCaptureCapability, width, height, "") {
   return arg.width == width && arg.height == height;
 }
 
-const int kTestFrameWidth1 = 100;
-const int kTestFrameHeight1 = 100;
-const int kTestFrameWidth2 = 200;
-const int kTestFrameHeight2 = 150;
+const int kTestFrameWidth1 = 500;
+const int kTestFrameHeight1 = 500;
+const int kTestFrameWidth2 = 400;
+const int kTestFrameHeight2 = 300;
 
 const int kFrameRate = 30;
 
@@ -293,6 +297,8 @@ TEST_F(DesktopCaptureDeviceTest, ScreenResolutionChangeConstantResolution) {
                                                      kTestFrameHeight1);
   capture_params.requested_format.frame_rate = kFrameRate;
   capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
+  capture_params.resolution_change_policy =
+      media::RESOLUTION_POLICY_FIXED_RESOLUTION;
 
   capture_device_->AllocateAndStart(capture_params, client.Pass());
 
@@ -312,6 +318,67 @@ TEST_F(DesktopCaptureDeviceTest, ScreenResolutionChangeConstantResolution) {
   EXPECT_EQ(format.frame_size.GetArea() * 4, frame_size);
 }
 
+// Test that screen capturer behaves correctly if the source frame size changes,
+// where the video frames sent the the client vary in resolution but maintain
+// the same aspect ratio.
+TEST_F(DesktopCaptureDeviceTest, ScreenResolutionChangeFixedAspectRatio) {
+  FakeScreenCapturer* mock_capturer = new FakeScreenCapturer();
+
+  CreateScreenCaptureDevice(scoped_ptr<webrtc::DesktopCapturer>(mock_capturer));
+
+  media::VideoCaptureFormats formats;
+  base::WaitableEvent done_event(false, false);
+
+  scoped_ptr<MockDeviceClient> client(new MockDeviceClient());
+  EXPECT_CALL(*client, OnError(_)).Times(0);
+  void (media::VideoCaptureFormats::*push_back_method)(
+      const media::VideoCaptureFormat&) =
+      &media::VideoCaptureFormats::push_back;
+  EXPECT_CALL(*client, OnIncomingCapturedData(_, _, _, _, _)).WillRepeatedly(
+      DoAll(WithArg<2>(Invoke(&formats, push_back_method)),
+            InvokeWithoutArgs(&done_event, &base::WaitableEvent::Signal)));
+
+  media::VideoCaptureParams capture_params;
+  const gfx::Size high_def_16_by_9(1920, 1080);
+  ASSERT_GE(high_def_16_by_9.width(),
+            std::max(kTestFrameWidth1, kTestFrameWidth2));
+  ASSERT_GE(high_def_16_by_9.height(),
+            std::max(kTestFrameHeight1, kTestFrameHeight2));
+  capture_params.requested_format.frame_size = high_def_16_by_9;
+  capture_params.requested_format.frame_rate = kFrameRate;
+  capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
+  capture_params.resolution_change_policy =
+      media::RESOLUTION_POLICY_FIXED_ASPECT_RATIO;
+
+  capture_device_->AllocateAndStart(
+      capture_params, client.Pass());
+
+  // Capture at least three frames, to ensure that the source frame size has
+  // changed at least twice while capturing.

[Wez, 2015/05/14 01:44:55]

nit: Why is this not a loop?

Why do we care that the source frame size has changed at least twice, versus
just once?

If the requirement is that it has changed at least twice, could we simply
compare received frame sizes between OnIncomingCapturedData() invocations?

[miu, 2015/05/14 21:12:26]

I originally wrote this test by forking the existing one
(ScreenResolutionChangeVariableResolution).  I've fixed all the tests to use a
loop.
 
Elaborated in code comments.
Yes.  Thinking about this a bit, I've ripped out the existing "vector of sizes"
approach, and replaced it with a simple FrameChecker helper class/method.  I've
done this for the three tests that correspond to each of the three possible
resolution change policies.

+  EXPECT_TRUE(done_event.TimedWait(TestTimeouts::action_max_timeout()));
+  done_event.Reset();
+  EXPECT_TRUE(done_event.TimedWait(TestTimeouts::action_max_timeout()));
+  done_event.Reset();
+  EXPECT_TRUE(done_event.TimedWait(TestTimeouts::action_max_timeout()));
+
+  capture_device_->StopAndDeAllocate();
+
+  std::vector<gfx::Size> distinct_sizes_seen;
+  for (const media::VideoCaptureFormat& f : formats) {

[Wez, 2015/05/14 01:44:55]

nit: Could this be:

for (auto format : formats)

?

[miu, 2015/05/14 21:12:26]

Reply 1: It could, but the style guide suggests only to use it where it would
help readability (e.g., huge-string template class types, like iterators).  IMO,
it's more readable to provide the exact, known type here.

Reply 2: No longer applicable since I've replaced this code.

+    ASSERT_FALSE(f.frame_size.IsEmpty());
+    if (std::find(distinct_sizes_seen.begin(),
+                  distinct_sizes_seen.end(),
+                  f.frame_size) == distinct_sizes_seen.end()) {
+      distinct_sizes_seen.push_back(f.frame_size);
+    }
+    EXPECT_EQ(kFrameRate, f.frame_rate);
+    EXPECT_EQ(media::PIXEL_FORMAT_ARGB, f.pixel_format);
+  }
+  EXPECT_EQ(2u, distinct_sizes_seen.size());

[Wez, 2015/05/14 01:44:55]

Is there something about the mock capturer that causes it to output two specific
sets of dimensions?

[miu, 2015/05/14 21:12:26]

Done.  Elaborated on this in code comments.

+  for (const gfx::Size& s : distinct_sizes_seen)

[Wez, 2015/05/14 01:44:55]

nit: auto here?

[miu, 2015/05/14 21:12:26]

No longer applicable since I've replaced this code.

+    EXPECT_NEAR(16.0 / 9.0, static_cast<double>(s.width()) / s.height(), 0.01);
+}
+
 // Test that screen capturer behaves correctly if the source frame size changes
 // and the caller can cope with variable resolution output.
 TEST_F(DesktopCaptureDeviceTest, ScreenResolutionChangeVariableResolution) {
@@ -319,20 +386,29 @@ TEST_F(DesktopCaptureDeviceTest, ScreenResolutionChangeVariableResolution) {
 
   CreateScreenCaptureDevice(scoped_ptr<webrtc::DesktopCapturer>(mock_capturer));
 
-  media::VideoCaptureFormat format;
+  media::VideoCaptureFormats formats;
   base::WaitableEvent done_event(false, false);
 
   scoped_ptr<MockDeviceClient> client(new MockDeviceClient());
   EXPECT_CALL(*client, OnError(_)).Times(0);
+  void (media::VideoCaptureFormats::*push_back_method)(
+      const media::VideoCaptureFormat&) =
+      &media::VideoCaptureFormats::push_back;
   EXPECT_CALL(*client, OnIncomingCapturedData(_, _, _, _, _)).WillRepeatedly(
-      DoAll(SaveArg<2>(&format),
+      DoAll(WithArg<2>(Invoke(&formats, push_back_method)),
             InvokeWithoutArgs(&done_event, &base::WaitableEvent::Signal)));
 
   media::VideoCaptureParams capture_params;
-  capture_params.requested_format.frame_size.SetSize(kTestFrameWidth2,
-                                                     kTestFrameHeight2);
+  const gfx::Size high_def_16_by_9(1920, 1080);
+  ASSERT_GE(high_def_16_by_9.width(),
+            std::max(kTestFrameWidth1, kTestFrameWidth2));
+  ASSERT_GE(high_def_16_by_9.height(),
+            std::max(kTestFrameHeight1, kTestFrameHeight2));
+  capture_params.requested_format.frame_size = high_def_16_by_9;
   capture_params.requested_format.frame_rate = kFrameRate;
   capture_params.requested_format.pixel_format = media::PIXEL_FORMAT_I420;
+  capture_params.resolution_change_policy =
+      media::RESOLUTION_POLICY_ANY_WITHIN_LIMIT;
 
   capture_device_->AllocateAndStart(
       capture_params, client.Pass());
@@ -347,10 +423,22 @@ TEST_F(DesktopCaptureDeviceTest, ScreenResolutionChangeVariableResolution) {
 
   capture_device_->StopAndDeAllocate();
 
-  EXPECT_EQ(kTestFrameWidth1, format.frame_size.width());
-  EXPECT_EQ(kTestFrameHeight1, format.frame_size.height());
-  EXPECT_EQ(kFrameRate, format.frame_rate);
-  EXPECT_EQ(media::PIXEL_FORMAT_ARGB, format.pixel_format);
+  bool have_seen_frame_size_1 = false;
+  bool have_seen_frame_size_2 = false;
+  bool have_seen_other_frame_size = false;
+  for (const media::VideoCaptureFormat& f : formats) {
+    if (f.frame_size == gfx::Size(kTestFrameWidth1, kTestFrameHeight1))
+      have_seen_frame_size_1 = true;
+    else if (f.frame_size == gfx::Size(kTestFrameWidth2, kTestFrameHeight2))
+      have_seen_frame_size_2 = true;
+    else
+      have_seen_other_frame_size = true;
+    EXPECT_EQ(kFrameRate, f.frame_rate);
+    EXPECT_EQ(media::PIXEL_FORMAT_ARGB, f.pixel_format);
+  }
+  EXPECT_TRUE(have_seen_frame_size_1);
+  EXPECT_TRUE(have_seen_frame_size_2);
+  EXPECT_FALSE(have_seen_other_frame_size);

[Wez, 2015/05/14 01:44:55]

nit: The fixed aspect-ratio test collates a list of the sizes seen and checks
e.g. that there are exactly two of them - you could do the same here, plus test
that the two test frame sizes are contained in the list, and the resulting code
looks like it'd be more concise?

Either way, it seems that using the same basic approach between the two tests
would be valid; the fixed-aspect test should still have predictable output
values, so could use a similar approach to this test.

[miu, 2015/05/14 21:12:26]

Done.

 }
 
 // This test verifies that an unpacked frame is converted to a packed frame.