Reland: Image Capture Linux/CrOs: wire some capabilities set/get

media/capture/video/video_capture_device_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/capture/video/video_capture_device.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
@@ skipping 31 matching lines @@
 #include "media/capture/video/android/video_capture_device_android.h"
 #include "media/capture/video/android/video_capture_device_factory_android.h"
 #endif
 
 #if defined(OS_MACOSX)
 // Mac will always give you the size you ask for and this case will fail.
 #define MAYBE_AllocateBadSize DISABLED_AllocateBadSize
 // We will always get YUYV from the Mac AVFoundation implementations.
 #define MAYBE_CaptureMjpeg DISABLED_CaptureMjpeg
 #define MAYBE_TakePhoto TakePhoto
+#define MAYBE_GetPhotoCapabilities DISABLED_GetPhotoCapabilities
 #elif defined(OS_WIN)
 #define MAYBE_AllocateBadSize AllocateBadSize
 #define MAYBE_CaptureMjpeg CaptureMjpeg
 #define MAYBE_TakePhoto TakePhoto
+#define MAYBE_GetPhotoCapabilities DISABLED_GetPhotoCapabilities
 #elif defined(OS_ANDROID)
 #define MAYBE_AllocateBadSize AllocateBadSize
 #define MAYBE_CaptureMjpeg CaptureMjpeg
 #define MAYBE_TakePhoto TakePhoto
+#define MAYBE_GetPhotoCapabilities GetPhotoCapabilities
 #elif defined(OS_LINUX)
 // AllocateBadSize will hang when a real camera is attached and if more than one
 // test is trying to use the camera (even across processes). Do NOT renable
 // this test without fixing the many bugs associated with it:
 // http://crbug.com/94134 http://crbug.com/137260 http://crbug.com/417824
 #define MAYBE_AllocateBadSize DISABLED_AllocateBadSize
 #define MAYBE_CaptureMjpeg CaptureMjpeg
 #define MAYBE_TakePhoto TakePhoto
+#define MAYBE_GetPhotoCapabilities GetPhotoCapabilities
 #else
 #define MAYBE_AllocateBadSize AllocateBadSize
 #define MAYBE_CaptureMjpeg CaptureMjpeg
 #define MAYBE_TakePhoto DISABLED_TakePhoto
+#define MAYBE_GetPhotoCapabilities DISABLED_GetPhotoCapabilities
 #endif
 
 using ::testing::_;
 using ::testing::Invoke;
 using ::testing::SaveArg;
 
 namespace media {
 namespace {
 
+ACTION_P(RunClosure, closure) {
+  closure.Run();
+}
+
 void DumpError(const tracked_objects::Location& location,
                const std::string& message) {
   DPLOG(ERROR) << location.ToString() << " " << message;
 }
 
 class MockVideoCaptureClient : public VideoCaptureDevice::Client {
  public:
   MOCK_METHOD0(DoReserveOutputBuffer, void(void));
   MOCK_METHOD0(DoOnIncomingCapturedBuffer, void(void));
   MOCK_METHOD0(DoOnIncomingCapturedVideoFrame, void(void));
@@ skipping 65 matching lines @@
       EXPECT_EQ(0xFF, blob->data[0]);         // First SOI byte
       EXPECT_EQ(0xD8, blob->data[1]);         // Second SOI byte
       EXPECT_EQ(0xFF, blob->data[2]);         // First JFIF-APP0 byte
       EXPECT_EQ(0xE0, blob->data[3] & 0xF0);  // Second JFIF-APP0 byte
       OnCorrectPhotoTaken();
     } else if (strcmp("image/png", blob->mime_type.c_str()) == 0) {
       ASSERT_GT(blob->data.size(), 4u);
       EXPECT_EQ('P', blob->data[1]);
       EXPECT_EQ('N', blob->data[2]);
       EXPECT_EQ('G', blob->data[3]);
       OnCorrectPhotoTaken();

[chfremer, 2016/09/23 21:18:30]

The expectation on the arriving data is expressed here, while the expectation on
the call count is expressed in the actual test cases.
This scattering of expectation/verification code between here and the actual
test cases makes it hard to understand what is actually verified when reading
the code for the test case.

As an example, when reading the code for test case MAYBE_TakePhoto it is
non-obvious that there is verification happening beyond the expectations set on
the call count of OnCorrectPhotoTaken.

Would it be possible to express all expectations in one place, i.e. the test
case?

[mcasas, 2016/09/23 22:10:57]

Yeah, but then we'd have to duplicate this header
checking-code, which will in turn force us to put
it in a static method, which will bring us back
to the beginning.

I like to see it as two different verifications:
One when we received a photo Blob, make sure it has
the appropriate type, and two, expect a certain 
amount of photo blob callbacks.  I can envision 
cases in which photo blobs are expected, but have
the wrong format, and also a case in which a
received blob has the right format, but is unexpected.
(Note that these are EXPECT, not asserts, so they
would "accumulate" as failures, without stopping
the test case on the spot).

[chfremer, 2016/09/23 22:48:56]

Okay to keep it the way it is, especially since it is pretty commonly done this
way. 

I was thinking about having a static method named
"VerifyDataHasExpectedFormat()" and then attaching that to the EXPECT_CALL in
the test case using Invoke(). This would make it obvious to the reader that this
verification is happening.

     } else {
       ADD_FAILURE() << "Photo format should be jpeg or png";
     }
   }
   MOCK_METHOD0(OnCorrectPhotoTaken, void(void));
   MOCK_METHOD1(OnTakePhotoFailure,
                void(const base::Callback<void(mojom::BlobPtr)>&));
 
+  // GMock doesn't support move-only arguments, so we use this forward method.
+  void DoOnGetPhotoCapabilities(mojom::PhotoCapabilitiesPtr capabilities) {
+    capabilities_ = std::move(capabilities);
+    OnCorrectGetPhotoCapabilities();
+  }
+  MOCK_METHOD0(OnCorrectGetPhotoCapabilities, void(void));
+  MOCK_METHOD1(OnGetPhotoCapabilitiesFailure,
+               void(const base::Callback<void(mojom::PhotoCapabilitiesPtr)>&));
+
+  const mojom::PhotoCapabilities* capabilities() { return capabilities_.get(); }
+
  private:
   friend class base::RefCounted<MockImageCaptureClient>;
   virtual ~MockImageCaptureClient() {}
+
+  mojom::PhotoCapabilitiesPtr capabilities_;
 };
 
 class DeviceEnumerationListener
     : public base::RefCounted<DeviceEnumerationListener> {
  public:
   MOCK_METHOD1(DoOnEnumerateDeviceDescriptors,
                void(VideoCaptureDeviceDescriptors* device_descriptors));
   // GMock doesn't support move-only arguments, so we use this forward method.
   void OnEnumerateDeviceDescriptors(
       std::unique_ptr<VideoCaptureDeviceDescriptors> device_descriptors) {
@@ skipping 326 matching lines @@
 TEST_F(VideoCaptureDeviceTest, NoCameraSupportsPixelFormatMax) {
   // Use PIXEL_FORMAT_MAX to iterate all device names for testing
   // GetDeviceSupportedFormats().
   std::unique_ptr<VideoCaptureDeviceDescriptor> device_descriptor =
       GetFirstDeviceDescriptorSupportingPixelFormat(PIXEL_FORMAT_MAX);
   // Verify no camera returned for PIXEL_FORMAT_MAX. Nothing else to test here
   // since we cannot forecast the hardware capabilities.
   ASSERT_FALSE(device_descriptor);
 }
 
-// Starts the camera and take a photo.
+// Starts the camera and takes a photo.

[chfremer, 2016/09/23 21:18:30]

This description is not really an answer to the question "What does this test?".
It is a description of the exercise, but does not say what is verified.

I would think something along the lines of "Tests that TakePhotoCallback is
invoked with valid data when calling TakePhoto() on an active capture device."

[mcasas, 2016/09/23 22:10:57]

Done.

 TEST_F(VideoCaptureDeviceTest, MAYBE_TakePhoto) {
   if (!EnumerateAndFindUsableDevices())
     return;
 
 #if defined(OS_ANDROID)
   // TODO(mcasas): fails on Lollipop devices, reconnect https://crbug.com/646840
   if (base::android::BuildInfo::GetInstance()->sdk_int() <
       base::android::SDK_VERSION_MARSHMALLOW) {
     return;
   }
 #endif
 
   std::unique_ptr<VideoCaptureDevice> device(
       video_capture_device_factory_->CreateDevice(
           device_descriptors_->front()));
   ASSERT_TRUE(device);
 
   EXPECT_CALL(*video_capture_client_, OnError(_, _)).Times(0);
 
   VideoCaptureParams capture_params;
   capture_params.requested_format.frame_size.SetSize(320, 240);
   capture_params.requested_format.frame_rate = 30;
   capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
   device->AllocateAndStart(capture_params, std::move(video_capture_client_));
   WaitForCapturedFrame();

[chfremer, 2016/09/23 21:18:29]

The description of the interface media::VideoCaptureDevice, does not state that
it is required to wait for the first captured frame before invoking TakePhoto().

If it is required, we need to update the description.
If it is not required, the test should not do it.

[mcasas, 2016/09/23 22:10:57]

Hmm well, it kinda says so:
"...and/or interrupting the capture flow", plus the
JS side prevents creating an ImageCapturer if the
MediaStream(Video)Track is not active, or muted, so
the device has to be capturing.

[chfremer, 2016/09/23 22:48:56]

As per discussion on IM, I think we should remove the WaitForCaptureFrame(),
since actual clients do not have to wait, and we want to cover that scenario
with our test.

Let us add a TODO for this, so we can move forward with this CL.

[mcasas, 2016/09/23 23:16:26]

Oh hark. Let me explain: from the ImageCapture 
POV, there's no need to wait for the first incoming
capture frame; however 
VideoCaptureDeviceTest::OnFrameCaptured() does try to
run_loop_->QuitClosure() and |run_loop_| is
allocated... in WaitForCaptureFrame().

So: I'll protect OnFrameCaptured() access to |run_loop_|,
and here just do a RunUntilIdle() in a local RunLoop,
with a comment on what we want by this.

 
   VideoCaptureDevice::TakePhotoCallback scoped_callback(
       base::Bind(&MockImageCaptureClient::DoOnPhotoTaken,
                  image_capture_client_),
       media::BindToCurrentLoop(base::Bind(
           &MockImageCaptureClient::OnTakePhotoFailure, image_capture_client_)));
 
-  EXPECT_CALL(*image_capture_client_.get(), OnCorrectPhotoTaken()).Times(1);
+  base::RunLoop run_loop;
+  base::Closure quit_closure = media::BindToCurrentLoop(run_loop.QuitClosure());
+  EXPECT_CALL(*image_capture_client_.get(), OnCorrectPhotoTaken())
+      .Times(1)
+      .WillOnce(RunClosure(quit_closure));
+
   device->TakePhoto(std::move(scoped_callback));
-  WaitForCapturedFrame();
+  run_loop.Run();
 
   device->StopAndDeAllocate();
 }
 
+// Starts the camera and tries to get the Photo capabilities.
+TEST_F(VideoCaptureDeviceTest, MAYBE_GetPhotoCapabilities) {
+  if (!EnumerateAndFindUsableDevices())
+    return;

[chfremer, 2016/09/23 21:18:29]

I assume we do this to not fail on machines that do not have devices attached.
But this way it will report the test as passed even though it really was
skipped. Is there a way in gtest to bail out of a test and have it marked as
skipped?

[mcasas, 2016/09/23 22:10:57]

Hmm that's a good suggestion. This behaviour is
pervasive among WebRtc tests, because many bots
don't have any camera. I would love to drop off
in the SetUp() version of the test, in that case
not logging the test's name on the console, but
I'm not sure how to do it.  I'll investigate.

Update: investigated and found out that there
is no runtime test-skipping features [1]. The
only way to skip it is to mark the test case 
name with a DISABLED_ prefix.


[1] https://github.com/google/googletest/issues/490

+
+#if defined(OS_ANDROID)
+  // TODO(mcasas): fails on Lollipop devices, reconnect https://crbug.com/646840
+  if (base::android::BuildInfo::GetInstance()->sdk_int() <
+      base::android::SDK_VERSION_MARSHMALLOW) {
+    return;
+  }
+#endif
+
+  std::unique_ptr<VideoCaptureDevice> device(
+      video_capture_device_factory_->CreateDevice(
+          device_descriptors_->front()));
+  ASSERT_TRUE(device);
+
+  EXPECT_CALL(*video_capture_client_, OnError(_, _)).Times(0);
+
+  VideoCaptureParams capture_params;
+  capture_params.requested_format.frame_size.SetSize(320, 240);
+  capture_params.requested_format.frame_rate = 30;
+  capture_params.requested_format.pixel_format = PIXEL_FORMAT_I420;
+  device->AllocateAndStart(capture_params, std::move(video_capture_client_));
+  WaitForCapturedFrame();
+
+  VideoCaptureDevice::GetPhotoCapabilitiesCallback scoped_get_callback(
+      base::Bind(&MockImageCaptureClient::DoOnGetPhotoCapabilities,
+                 image_capture_client_),
+      media::BindToCurrentLoop(
+          base::Bind(&MockImageCaptureClient::OnGetPhotoCapabilitiesFailure,
+                     image_capture_client_)));
+
+  base::RunLoop run_loop;
+  base::Closure quit_closure = media::BindToCurrentLoop(run_loop.QuitClosure());
+  EXPECT_CALL(*image_capture_client_.get(), OnCorrectGetPhotoCapabilities())
+      .Times(1)
+      .WillOnce(RunClosure(quit_closure));
+
+  device->GetPhotoCapabilities(std::move(scoped_get_callback));
+  run_loop.Run();
+
+  ASSERT_TRUE(image_capture_client_->capabilities());
+
+  device->StopAndDeAllocate();
+}
+
 };  // namespace media