media: add video capture device for ARC++ camera HAL v3

media/capture/video/chromeos/camera_hal_delegate_unittest.cc

+// Copyright 2017 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/chromeos/camera_hal_delegate_unittest.h"
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <memory>
+#include <utility>
+
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "media/capture/video/chromeos/mojo/arc_camera3.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::A;
+using testing::Invoke;
+
+namespace media {
+
+class CameraHalDelegateTest : public ::testing::Test {
+ public:
+  CameraHalDelegateTest()
+      : message_loop_(new base::MessageLoop),
+        hal_delegate_thread_("HalDelegateThread") {}
+
+  void SetUp() override {
+    hal_delegate_thread_.Start();
+    camera_hal_delegate_ =
+        new CameraHalDelegate(hal_delegate_thread_.task_runner());
+    camera_hal_delegate_->StartForTesting(
+        mock_camera_module_.GetInterfacePtrInfo());
+  }
+
+  void TearDown() override {
+    while (!camera_hal_delegate_->HasOneRef()) {
+      base::PlatformThread::YieldCurrentThread();
+    }

[chfremer, 2017/05/26 17:40:57]

Same as in the production code, this while loop looks suspicious and can
probably be replaced with a more official way of other reference holders
signaling us when they have finished whatever we need to wait for here.

[jcliang, 2017/05/31 14:58:06]

We can remove the while loop here by explicitly calling the
camera_hal_delegate_.Reset() to queue the task to reset all Mojo
interface/binding on the task runner, and then count on
hal_delegate_thread_.Stop() to wait until the task is done.

The scenario in the production code in VideoCaptureDeviceArcChromeOS is more
complex though, as it involves a Close() Mojo IPC call which in theory could
block indefinitely (say, the camera HAL has a bug).

[chfremer, 2017/06/01 00:16:27]

It appears to me that a lot of the code in this CL is designed with synchronous
shutdown methods (or destructors) that block until shutdown is complete. This
TearDown() method is one example. An alternative design approach would be to
make the shutdown methods asynchronous and non-blocking, and use callbacks to
notify the caller when the shutdown is complete. I guess both approaches can
work, and I leave it up to you to decide what is most suited for this component.
In general I would usually go for the asynchronous approach, because it gives
the caller the flexibility to decide if and when to block.

 

+    camera_hal_delegate_ = nullptr;
+    hal_delegate_thread_.Stop();
+  }
+
+  void Wait() {

[chfremer, 2017/05/26 17:40:57]

Couldn't find where this is used. Can it be removed?

[jcliang, 2017/05/31 14:58:07]

Done.

+    run_loop_.reset(new base::RunLoop());
+    run_loop_->Run();
+  }
+
+ protected:
+  scoped_refptr<CameraHalDelegate> camera_hal_delegate_;
+  testing::StrictMock<unittest_internal::MockCameraModule> mock_camera_module_;
+
+ private:
+  std::unique_ptr<base::MessageLoop> message_loop_;

[chfremer, 2017/05/26 17:40:58]

IIUC, it is now encouraged to use a ScopedTaskEnvironment instead, see
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

[jcliang, 2017/05/31 14:58:07]

Thanks for the pointer. I'm removing this for now. I'll use
ScopedTaskEnvironment when I need to use Wait() for other test cases later.

+  base::Thread hal_delegate_thread_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+  DISALLOW_COPY_AND_ASSIGN(CameraHalDelegateTest);
+};
+
+TEST_F(CameraHalDelegateTest, GetBuiltinCameraInfo) {
+  auto get_number_of_cameras_cb =
+      [](arc::mojom::CameraModule::GetNumberOfCamerasCallback& cb) {
+        std::move(cb).Run(2);
+      };

[chfremer, 2017/05/26 17:40:58]

I would be okay with this, but not sure if this may violate the Chromium C++11
allowed feature list. Assuming the lambda expression leads to the auto type
expanding to std::function<...>, which is on the banned feature list.

If so, I guess we could just define the lambdas directly where they are used
instead of storing them in a temporary variable.

[jcliang, 2017/05/31 14:58:06]

I see that binding captureless lambda is documented in
https://cs.chromium.org/chromium/src/docs/callback.md?type=cs&l=97. I guess this
means it is allowed.

+
+  auto get_camera_info_cb =
+      [](uint32_t camera_id,
+         arc::mojom::CameraModule::GetCameraInfoCallback& cb) {
+        arc::mojom::CameraInfoPtr camera_info = arc::mojom::CameraInfo::New();
+        arc::mojom::CameraMetadataPtr static_metadata =
+            arc::mojom::CameraMetadata::New();
+        switch (camera_id) {
+          case 0:
+            camera_info->facing = arc::mojom::CameraFacing::CAMERA_FACING_BACK;
+            camera_info->orientation = 0;
+            camera_info->static_camera_characteristics =
+                std::move(static_metadata);
+            break;
+          case 1:
+            camera_info->facing = arc::mojom::CameraFacing::CAMERA_FACING_FRONT;
+            camera_info->orientation = 0;
+            camera_info->static_camera_characteristics =
+                std::move(static_metadata);
+            break;
+          default:
+            FAIL() << "Invalid camera id";
+        }
+        std::move(cb).Run(0, std::move(camera_info));
+      };
+
+  EXPECT_CALL(mock_camera_module_, DoGetNumberOfCameras(_))
+      .Times(1)
+      .WillOnce(Invoke(get_number_of_cameras_cb));
+  EXPECT_CALL(
+      mock_camera_module_,
+      DoSetCallbacks(A<arc::mojom::CameraModuleCallbacksPtr&>(),
+                     A<arc::mojom::CameraModule::SetCallbacksCallback&>()))
+      .Times(1);
+  EXPECT_CALL(
+      mock_camera_module_,
+      DoGetCameraInfo(0, A<arc::mojom::CameraModule::GetCameraInfoCallback&>()))
+      .Times(1)
+      .WillOnce(Invoke(get_camera_info_cb));
+  EXPECT_CALL(
+      mock_camera_module_,
+      DoGetCameraInfo(1, A<arc::mojom::CameraModule::GetCameraInfoCallback&>()))
+      .Times(1)
+      .WillOnce(Invoke(get_camera_info_cb));
+
+  VideoCaptureDeviceDescriptors descriptors;
+  camera_hal_delegate_->GetDeviceDescriptors(&descriptors);
+
+  ASSERT_EQ(2U, descriptors.size());
+  ASSERT_EQ(std::to_string(0), descriptors[0].device_id);

[chfremer, 2017/05/26 17:40:58]

Where does this expectation come from? I do not see the deivce_id being set up
in the mock.

[jcliang, 2017/05/31 14:58:07]

The device_id is filled in camera_hal_delegate_->GetDeviceDescriptors(); it is
simply the camera_id as a string.

[chfremer, 2017/05/31 18:30:41]

I see. So this basically just verifies that CameraHalDelegate uses a string
representation of the device index as |device_id|.

+  ASSERT_EQ(VideoFacingMode::MEDIA_VIDEO_FACING_ENVIRONMENT,
+            descriptors[0].facing);
+  ASSERT_EQ(std::to_string(1), descriptors[1].device_id);
+  ASSERT_EQ(VideoFacingMode::MEDIA_VIDEO_FACING_USER, descriptors[1].facing);
+
+  VideoCaptureFormats supported_formats;
+  camera_hal_delegate_->GetSupportedFormats(descriptors[0], &supported_formats);
+
+  ASSERT_EQ(1U, supported_formats.size());
+  ASSERT_EQ(gfx::Size(1280, 720), supported_formats[0].frame_size);
+  ASSERT_EQ(60.0, supported_formats[0].frame_rate);
+  ASSERT_EQ(PIXEL_FORMAT_NV12, supported_formats[0].pixel_format);

[chfremer, 2017/05/26 17:40:58]

Where do these expected values come from?

[jcliang, 2017/05/31 14:58:06]

This was from a workaround I had in CameraHalDelegate::GetSupportedFormats. I've
updated the unit test to use fake metadata instead.

+}
+
+}  // namespace media