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

media/capture/video/chromeos/camera_hal_delegate.h

+// 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.
+
+#ifndef MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_HAL_DELEGATE_H_
+#define MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_HAL_DELEGATE_H_
+
+#include <memory>
+#include <string>
+#include <unordered_map>
+
+#include "base/macros.h"
+#include "base/sequence_checker.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/thread.h"
+#include "media/capture/video/chromeos/mojo/arc_camera3.mojom.h"
+#include "media/capture/video/video_capture_device_factory.h"
+#include "media/capture/video_capture_types.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+namespace media {
+
+// VideoCaptureDeviceFactoryChromeOS delegates all its VCD factory interface
+// function calls to CameraHalDelegate.

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

As a reader, here, I would expect to find a description of what this class does,
which is missing here and is not obvious from the class name. The information
who is using the class is useful, but dangerous to include in the class-level
description, because it is easy to become inaccurate. IMHO, it is better to rely
on the code search tool to find usages.

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

Done.

+//
+// CameraHalDelegate is refcounted because VideoCaptureDeviceFactoryChromeOS and
+// CameraDeviceDelegate both need to reference CameraHalDelegate, and
+// VideoCaptureDeviceFactoryChromeOS may be destroyed while CameraDeviceDelegate
+// is still alive.
+class CAPTURE_EXPORT CameraHalDelegate final
+    : public base::RefCountedThreadSafe<CameraHalDelegate>,
+      public arc::mojom::CameraModuleCallbacks {
+ public:
+  explicit CameraHalDelegate(
+      const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner);

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

Either here or in the class-level description it would be helpful to have an
explanation of what |module_task_runner| is needed for.

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

Done.

+
+  // Establishes the Mojo IPC channel to the camera HAL adapter.  This method
+  // should be called before any other methods of CameraHalDelegate is called.
+  // Called by VideoCaptureDeviceFactoryChromeOS::Init on the browser UI thread.

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

I recommend removing the information about by who and on which thread this gets
called (it is already inaccurate).

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

Done.

+  bool StartCameraModuleIpc();
+
+  // Delegation methods for the VideoCaptureDeviceFactory interface.  These
+  // methods are called by VideoCaptureDeviceFactoryChromeOS directly.  They
+  // operate on the same thread that the VideoCaptureDeviceFactoryChromeOS runs
+  // on.

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

This raises the following question (in me, as a reader): If the interface
exposed here is so similar to VideoCaptureDeviceFactory, what is actually the
purpose of VideoCaptureDeviceFactoryChromeOS? Why do we need it? What does it do
that is worth having it separate from this class?

A good place for this information would be the class-level description.

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

The reason that VideoCaptureDeviceFactoryChromeOS and CameraHalDelegate need to
be separate is because the static function
VideoCaptureDeviceFactory::CreateVideoCaptureDeviceFactory must return a raw
pointer while we need CameraHalDelegate to be ref-counted.

I also have a follow-up CL that will add a ArcCamera3Service Mojo proxy which
will be started by VideoCaptureDeviceFactoryChromeOS:
https://codereview.chromium.org/2878233002/. It's not a big change to
VideoCaptureDeviceFactoryChromeOS though; the VideoCaptureDeviceFactoryChromeOS
simply serves as an entry point to start the ArcCamera3Service Mojo proxy. The
raw pointer v.s. ref-counted pointer is still the blocker for me to merge these
two classes.

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

I see and agree that this is reasonable.

+  std::unique_ptr<VideoCaptureDevice> CreateDevice(
+      const scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
+      const VideoCaptureDeviceDescriptor& device_descriptor);
+  void GetSupportedFormats(
+      const VideoCaptureDeviceDescriptor& device_descriptor,
+      VideoCaptureFormats* supported_formats);
+  void GetDeviceDescriptors(VideoCaptureDeviceDescriptors* device_descriptors);
+
+  // Synchronous method to get the camera info of |camera_id|.  This method may
+  // be called on any thread.

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

Hmm, the method signature does look asynchronous, though. Does the comment above
mean that this implementation guarantees that the callback is invoked
synchronously? If so, why not use a return value instead of a callback?

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

Fixed the comment. I forgot to fix the comment after I changed the function from
synchronous to asynchronous.

+  using GetCameraInfoCallback =
+      base::Callback<void(int32_t, arc::mojom::CameraInfoPtr)>;
+  void GetCameraInfo(int32_t camera_id, const GetCameraInfoCallback& callback);
+
+  // Asynchronous method to open the camera device designated by |camera_id|.
+  // This method may be called on any thread; |callback| will run on
+  // |module_task_runner_|.
+  using OpenDeviceCallback =
+      base::Callback<void(int32_t, arc::mojom::Camera3DeviceOpsPtr)>;
+  void OpenDevice(int32_t camera_id, const OpenDeviceCallback& callback);
+
+ private:
+  friend class base::RefCountedThreadSafe<CameraHalDelegate>;
+
+  ~CameraHalDelegate();
+
+  friend class CameraHalDelegateTest;
+  friend class CameraDeviceDelegateTest;
+  void StartForTesting(arc::mojom::CameraModulePtrInfo info);

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

The need for a test-only API like this is often an indicator of a design issue.
In the case of this class, this is caused by the class wanting to do both
establishing the Mojo connection and using it, which are almost unrelated
responsibilities. I recommend resolving this by splitting out the responsibility
of establishing the Mojo connection to a factory class, something like:

class ServiceCameraHalDelegateFactory {
 public:
  scoped_refptr<CameraHalDelegate>
CreateCameraHalDelegate(scoped_refptr<base::SingleThreadTaskRunner>
module_task_runner);
  // ...
};

This class would contain the logic from StartCameraModuleIpc(). The class
CameraHalDelegate, would get its |camera_module_| via dependency injection from
the constructor, i.e.:

class CameraHalDelegate {
 public:
  CameraHalDelegate(
      const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner,
      arc::mojom::CameraModulePtr camera_module);
};

Unit tests can then instantiate CameraHalDelegate through its public constructor
using a mock for |camera_module|.

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

This test-only API is only to make this CL self-contained to be able to run the
unit tests. The test-only API is transient and will be removed with the CL that
adds the ArcCamera3Service Mojo proxy CL:
https://codereview.chromium.org/2878233002/. The ArcCamera3Service Mojo proxy
acts like a factory class like you suggested above. I separate the
ArcCamera3Service out to another CL to make this CL easier to review.

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

I don't see any test for CameraHalDelegate in
https://codereview.chromium.org/2878233002/. It does not currently seem to be a
follow-up based on the code of this CL. If you have a newer version, could you
please update it so I can see the diff from this CL?

If your follow-up work already removes the need for this testing-only API, then
I don't want to ask for any unnecessary extra work to remove them from this CL.
Otherwise, I don't see why the cleaner solution using dependency injection would
not lead to a self-contained CL.

[jcliang, 2017/06/01 17:11:16]

Sorry, by self-contained I mean I need to make this CL work with the current
camera HAL process on Chrome OS, hence I need the StartCameraModuleIpc() method
above to establish the Mojo connection.

The follow-up CL will change the Mojo process model to register-observer mode,
which will remove StartCameraModuleIpc() and add another SetCameraModule()
method (https://codereview.chromium.org/2878233002/patch/1/10005). I will be
using the SetCameraModule() method to inject MockCameraModule, which is similar
to the dependency-injection approach you suggested.

I haven't started on the new version of unit tests in the follow-up CL as it
largely depend on this CL. I need to rebase every time I make any changes in
this CL. I'm waiting for this CL to stabilize first.

+
+  // Resets the Mojo interface and bindings.
+  void ResetMojoInterfaceOnModuleThread(base::WaitableEvent* interface_closed);
+
+  // Internal method to update the camera info for all built-in cameras. Runs on
+  // the same thread as CreateDevice, GetSupportedFormats, and
+  // GetDeviceDescriptors.
+  bool UpdateBuiltInCameraInfo();
+  void UpdateBuiltInCameraInfoOnModuleThread();
+  // Callback for GetNumberOfCameras Mojo IPC function.  GetNumberOfCameras
+  // returns the number of built-in cameras on the device.
+  void OnGotNumberOfCamerasOnModuleThread(int32_t num_cameras);
+  // Callback for SetCallbacks Mojo IPC function. SetCallbacks is called after
+  // GetNumberOfCameras is called for the first time, and before any other calls
+  // to |camera_module_|.
+  void OnSetCallbacksOnModuleThread(int32_t result);
+  void GetCameraInfoOnModuleThread(int32_t camera_id,
+                                   const GetCameraInfoCallback& callback);
+  void OnGotCameraInfoOnModuleThread(int32_t camera_id,
+                                     int32_t result,
+                                     arc::mojom::CameraInfoPtr camera_info);
+
+  // Called by OpenDevice to actually open the device specified by |camera_id|.
+  // This method runs on |module_task_runner_|.
+  void OpenDeviceOnModuleThread(int32_t camera_id,
+                                const OpenDeviceCallback& callback);
+
+  // CameraModuleCallbacks implementation. Operates on |module_task_runner_|.
+  void CameraDeviceStatusChange(
+      int32_t camera_id,
+      arc::mojom::CameraDeviceStatus new_status) final;
+
+  // Signaled when |num_builtin_cameras_| and |camera_info_| are updated.
+  // Queried and waited by UpdateBuiltInCameraInfo, signaled by
+  // OnGotCameraInfoOnModuleThread.
+  base::WaitableEvent builtin_camera_info_updated_;
+
+  // |num_builtin_cameras_| stores the number of built-in camera devices
+  // reported by the camera HAL, and |camera_info_| stores the camera info of
+  // each camera device. They are modified only on |module_task_runner_|. They
+  // are also read in GetSupportedFormats and GetDeviceDescriptors, in which the
+  // access is sequenced through UpdateBuiltInCameraInfo and
+  // |builtin_camera_info_updated_| to avoid race conditions.
+  uint32_t num_builtin_cameras_;
+  std::unordered_map<std::string, arc::mojom::CameraInfoPtr> camera_info_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  // The task runner where all the camera module Mojo communication takes place.
+  const scoped_refptr<base::SingleThreadTaskRunner> module_task_runner_;
+
+  // The Mojo proxy to access the camera module at the remote camera HAL.  Bound
+  // to |module_task_runner_|.
+  arc::mojom::CameraModulePtr camera_module_;
+
+  // The Mojo binding serving the camera module callbacks.  Bound to
+  // |module_task_runner_|.
+  mojo::Binding<arc::mojom::CameraModuleCallbacks> camera_module_callbacks_;
+
+  DISALLOW_COPY_AND_ASSIGN(CameraHalDelegate);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_HAL_DELEGATE_H_