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

media/capture/video/chromeos/camera_device_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_DEVICE_DELEGATE_H_
+#define MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_DEVICE_DELEGATE_H_
+
+#include <map>
+#include <memory>
+#include <set>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/synchronization/waitable_event.h"
+#include "media/capture/video/chromeos/mojo/arc_camera3.mojom.h"
+#include "media/capture/video/video_capture_device.h"
+#include "media/capture/video_capture_types.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+namespace media {
+
+class CameraHalDelegate;
+
+// CameraDeviceDelegate is instantiated on the capture thread where
+// AllocateAndStart of VideoCaptureDeviceArcChromeOS runs on.  All the methods
+// in CameraDeviceDelegate run on |ipc_task_runner_| and hence all the
+// access to member variables is sequenced.
+//
+// CameraDeviceDelegate is refcounted because it is bound to closures that run
+// on the module thread of |hal_delegate_| and the device thread which is
+// referenced by |ipc_task_runner_|.
+class CAPTURE_EXPORT CameraDeviceDelegate final
+    : public base::RefCountedThreadSafe<CameraDeviceDelegate>,
+      public arc::mojom::Camera3CallbackOps {
+ public:
+  enum State {

[chfremer, 2017/06/01 17:16:48]

I don't see any public API of this class that involves the State enum, so why
does State need to be public?

[jcliang, 2017/06/06 16:04:29]

With the new test cases I added in camera_device_delegate_unittest.cc I now need
to access CameraDeviceDelegate::State in the unit test.

+    // The camera device is completely stopped. This is the initial state, and
+    // is also set in OnClosed().
+    kStopped,
+
+    // The camera device is starting and waiting to be initialized.
+    //
+    // The kStarting state is set in AllocateAndStart().
+    kStarting,
+
+    // The camera device is initialized and can accept stream configuration
+    // requests.
+    //
+    // The state is transitioned to kInitialized through:
+    //
+    //   |hal_delegate_|->GetCameraInfo() -> OnGotCameraInfo() ->
+    //   |hal_delegate_|->OpenDevice() -> OnOpenedDevice() ->
+    //   Initialize() -> OnInitialized()
+    kInitialized,
+
+    // The various capture streams are configured and the camera device is ready
+    // to process capture requests.

[chfremer, 2017/06/01 17:16:48]

For a reader unfamiliar with the larger context (including myself), this raises
the following questions:
* What is a capture stream?
* Can there be more than one capture stream?
* When I use an instance of this class, do I need to configure these streams
through its public API or is that something that happens internally?

If knowing the answers to all these is relevant to the clients of the public API
of this class, ideally the class-level description would include a high-level
overview of these concepts.

[jcliang, 2017/06/06 16:04:29]

A capture stream is a set of parameters (width, height, buffer format, etc..)
which define a input to / output from the camera HAL. There can be multiple
streams. In the Chrome use case here we only use one preview stream. The stream
configuration happens internally through some HAL-specific API calls. The users
of the public APIs (i.e. the VideoCaptureDevice interface) do not need to worry
about it.

I've moved the State enum to private to make it clear that the states and stream
configurations are internal to the implementation.

[jcliang, 2017/06/06 16:09:23]

Oops, I should have deleted the last paragraph. The State enum is still in the
public section because I need to access it in the unit tests.

[chfremer, 2017/06/06 17:09:25]

If I understand correctly, the situation is as follows. The enum State is not
relevant for readers or clients of the public API of CameraDeviceDelegate. But
it is relevant for unit tests of the implementation details. IMHO, the clean
approach for this situation is to move the implementation details out to a class
of its own. This way, the things that are private or irrelevant to clients of
CameraDeviceDelegate do not have to appear here, but can still be public to
clients to who they are relevant, e.g. the unit tests that verify their behavior
or readers who are interested in the implementation details at that level.

[jcliang, 2017/06/07 08:18:39]

This is a great suggestion! I've extracted the state and some other member
variable into a CameraDeviceContext class.

+    //
+    // The state is transitioned to kStreamConfigured through:
+    //
+    //   ConfigureStreams() -> OnConfiguredStreams() ->
+    //   ConstructDefaultRequestSettings() ->
+    //   OnConstructedDefaultRequestSettings()
+    kStreamConfigured,
+
+    // The camera device is capturing video streams.
+    //
+    // The kCapturing state is set in StartCapture().
+    kCapturing,
+
+    // When the camera device is in the kCapturing state, a capture loop is
+    // constantly running:
+    //
+    // On the CameraDeviceDelegate side, we register and submit a capture
+    // request whenever a free buffer is available:
+    //
+    //   RegisterBuffer() -> OnRegisteredBuffer() ->
+    //   ProcessCaptureRequest() -> OnProcessedCaptureRequest()
+    //
+    // We get various capture metadata and error notifications from the camera
+    // HAL through the following callbacks:
+    //
+    //   ProcessCaptureResult()
+    //   Notify()
+
+    // The camera device is going through the shut down process; in order to
+    // avoid race conditions, no new Mojo messages may be sent to camera HAL in
+    // the kStopping state.
+    //
+    // The kStopping state is set in StopAndDeAllocate().
+    kStopping,
+
+    // The camera device encountered an unrecoverable error and needs to be
+    // StopAndDeAllocate()'d.
+    //
+    // The kError state is set in SetErrorState().
+    kError,
+  };
+
+  CameraDeviceDelegate(
+      VideoCaptureDeviceDescriptor device_descriptor,
+      scoped_refptr<CameraHalDelegate> camera_hal_delegate,
+      scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner);
+
+  // Delegation methods for the VideoCaptureDevice interface.
+  void AllocateAndStart(const VideoCaptureParams& params,
+                        std::unique_ptr<VideoCaptureDevice::Client> client);
+  void StopAndDeAllocate();
+  void TakePhoto(VideoCaptureDevice::TakePhotoCallback callback);
+  void GetPhotoCapabilities(
+      VideoCaptureDevice::GetPhotoCapabilitiesCallback callback);
+  void SetPhotoOptions(mojom::PhotoSettingsPtr settings,
+                       VideoCaptureDevice::SetPhotoOptionsCallback callback);
+
+  // Sets the frame rotation angle in |rotation_|.  |rotation_| is clockwise
+  // rotation in degrees, and is passed to |client_| along with the captured
+  // frames.
+  void SetRotation(int rotation);
+
+ private:
+  friend class base::RefCountedThreadSafe<CameraDeviceDelegate>;
+
+  ~CameraDeviceDelegate() = default;
+
+  // Sets the state.

[chfremer, 2017/06/01 17:16:49]

This comment is redundant. I recommend only using comments where the method
signature is not sufficient to convey what it does.

[jcliang, 2017/06/06 16:04:29]

Done.

+  void SetState(State state);
+
+  // Sets state to kError and call |client_->OnError| to tear down the
+  // VideoCaptureDevice.
+  void SetErrorState(const tracked_objects::Location& from_here,
+                     const std::string& reason);
+
+  // Resets the Mojo interface and bindings.
+  void ResetMojoInterface();
+
+  // Mojo connection error handler.
+  void OnMojoConnectionError();
+
+  // Callback method for the Close Mojo IPC call.  This method resets the Mojo
+  // connection and closes the camera device.
+  void OnClosed(int32_t result);
+
+  // This method run on |module_task_runner_| of |camera_hal_delegate_|.
+  void OnGotCameraInfoOnModuleDelegate(int32_t result,
+                                       arc::mojom::CameraInfoPtr camera_info);
+  // Sets |static_metadata_| from |camera_info|.
+  void OnGotCameraInfo(int32_t result, arc::mojom::CameraInfoPtr camera_info);
+
+  // This method run on |module_task_runner_| of |camera_hal_delegate_|.
+  void OnOpenedDeviceOnModuleDelegate(int32_t result);
+  // Creates the Mojo connection to the camera device.
+  void OnOpenedDevice(int32_t result);
+
+  // Initializes the camera HAL.  Initialize sets up the Camera3CallbackOps with
+  // the camera HAL.  OnInitialized continues to ConfigureStreams if the
+  // Initialize call succeeds.
+  void Initialize();
+  void OnInitialized(int32_t result);
+
+  // ConfigureStreams sets up stream context in |streams_| and configure the
+  // streams with the camera HAL.  OnConfiguredStreams updates |streams_| with
+  // the stream config returned, and allocates buffers as per |updated_config|
+  // indicates.  If there's no error OnConfiguredStreams notifies
+  // |client_| the capture has started by calling OnStarted, and proceeds to
+  // ConstructDefaultRequestSettings.
+  void ConfigureStreams();
+  void OnConfiguredStreams(
+      int32_t result,
+      arc::mojom::Camera3StreamConfigurationPtr updated_config);
+
+  // ConstructDefaultRequestSettings asks the camera HAL for the default request
+  // settings of the stream in |stream_context_|.
+  // OnConstructedDefaultRequestSettings sets the request settings in
+  // |streams_context_|.  If there's no error
+  // OnConstructedDefaultRequestSettings calls StartCapture to start the video
+  // capture loop.
+  void ConstructDefaultRequestSettings();
+  void OnConstructedDefaultRequestSettings(
+      arc::mojom::CameraMetadataPtr settings);
+
+  // StartCapture is the entry point to starting the video capture.  The way the
+  // video capture loop works is:
+  //
+  //  (1) If there is a free buffer, RegisterBuffer registers the buffer with
+  //      the camera HAL.
+  //  (2) Once the free buffer is registered, ProcessCaptureRequest is called to
+  //      issue a capture request which will eventually fill the registered
+  //      buffer.  Goto (1) to register the remaining free buffers.
+  //  (3) The camera HAL returns shutter time of a capture request through
+  //      Notify, and the filled buffer through ProcessCaptureResult.
+  //  (4) Once all the result metadata are collected, SubmitCaptureResult is
+  //      called to deliver the filled buffer to Chrome.  After the buffer is
+  //      consumed by Chrome it is enqueued back to the free buffer queue.
+  //      Goto (1) to start another capture loop.
+  void StartCapture();
+
+  // Registers a free buffer, if any, to the camera HAL.
+  void RegisterBuffer();
+
+  // Calls ProcessCaptureRequest if the buffer indicated by |buffer_id| is
+  // successfully registered.
+  void OnRegisteredBuffer(size_t buffer_id, int32_t result);
+
+  // The capture request contains the buffer handle indicated by |buffer_id|.
+  void ProcessCaptureRequest(size_t buffer_id);
+  // Calls RegisterBuffer to attempt to register any remaining free buffers.
+  void OnProcessedCaptureRequest(int32_t result);
+
+  // Camera3CallbackOps implementations.
+
+  // ProcessCaptureResult receives the result metadata as well as the filled
+  // buffer from camera HAL.  The result metadata may be divided and delivered
+  // in several stages.  Before all the result metadata is received the partial
+  // results are kept in |partial_results_|.
+  void ProcessCaptureResult(arc::mojom::Camera3CaptureResultPtr result) final;
+
+  // Notify receives the shutter time of capture requests and various errors
+  // from camera HAL.  The shutter time is used as the timestamp in the video
+  // frame delivered to Chrome.
+  void Notify(arc::mojom::Camera3NotifyMsgPtr message) final;
+  void HandleNotifyError(uint32_t frame_number,
+                         uint64_t error_stream_id,
+                         arc::mojom::Camera3ErrorMsgCode error_code);
+
+  // Submits the captured buffer of frame |frame_number_| to Chrome, then
+  // enqueues the buffer to free buffer queue for the next capture request.
+  void SubmitCaptureResult(uint32_t frame_number);
+

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

This class is quite big and has a large number of private methods. This is
typically an indicator that this class has too many responsibilities, some of
which should be extracted into classes of their own that, in-turn, can then be
tested, changed, and digested by readers in isolation, which should make the
life of future engineers reading and touching this code much easier.

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

I've split the buffer allocation and circulation into a StreamBufferManager
class. The CameraDeviceDelegate now opens and initializes the camera device,
while the StreamBufferManager allocates the buffers and sends / receives capture
requests / results.

This header file is still large, but the implementations are split into two .cc
files.

[chfremer, 2017/06/06 17:09:25]

Thanks. That is definitely an improvement.
From my previous read through, I believe there may be a couple more
responsibilities that could be extracted into classes of their own. If I see
them, I will try to point them out in my next read through.

+  const VideoCaptureDeviceDescriptor device_descriptor_;
+  const scoped_refptr<CameraHalDelegate> camera_hal_delegate_;
+
+  VideoCaptureParams chrome_capture_params_;
+  std::unique_ptr<VideoCaptureDevice::Client> client_;
+
+  // Stores the static camera characteristics of the camera device.
+  arc::mojom::CameraMetadataPtr static_metadata_;
+
+  // The state the CameraDeviceDelegate currently is in.
+  State state_;
+
+  // Clockwise rotation in degrees. This value should be 0, 90, 180, or 270.
+  int rotation_;
+
+  // Mojo proxy and binding for the camera HAL device ops and callback ops
+  // interfaces.
+  arc::mojom::Camera3DeviceOpsPtr device_ops_;
+  mojo::Binding<arc::mojom::Camera3CallbackOps> callback_ops_;
+
+  // Where all the Mojo IPC calls takes place.
+  const scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner_;
+
+  // The frame number.  Increased by one for each capture request sent; reset to
+  // zero in AllocateAndStart.
+  uint32_t frame_number_;
+
+  struct StreamContext {
+    // The actual pixel format used in the capture request.
+    VideoCaptureFormat capture_format;
+    // The camera HAL stream.
+    arc::mojom::Camera3StreamPtr stream;
+    // The request settings used in the capture request of this stream.
+    arc::mojom::CameraMetadataPtr request_settings;
+    // The allocated buffers of this stream.
+    std::vector<std::unique_ptr<base::SharedMemory>> buffers;
+    // The free buffers of this stream.  The queue stores indices into the
+    // |buffers| vector.
+    std::queue<size_t> free_buffers;
+  };
+
+  // The stream context of the preview stream.
+  std::unique_ptr<StreamContext> stream_context_;
+
+  // CaptureResult is used to hold the partial capture results for each frame.
+  struct CaptureResult {
+    CaptureResult() : metadata(arc::mojom::CameraMetadata::New()) {}
+    // |reference_time| and |timestamp| are derived from the shutter time of
+    // this frame.  They are be passed to |client_->OnIncomingCapturedData|
+    // along with the |buffers| when the captured frame is submitted.
+    base::TimeTicks reference_time;
+    base::TimeDelta timestamp;
+    // The result metadata.  Contains various information about the captured
+    // frame.
+    arc::mojom::CameraMetadataPtr metadata;
+    // The buffer handle that hold the captured data of this frame.
+    arc::mojom::Camera3StreamBufferPtr buffer;
+    // The set of the partial metadata received.  For each capture result, the
+    // total number of partial metadata should equal to |partial_result_count_|.
+    std::set<uint32_t> partial_metadata_received;
+  };
+
+  // The number of partial stages.  |partial_result_count_| is learned by
+  // querying |static_metadata_|.  In case the result count is absent in
+  // |static_metadata_|, it defaults to one which means all the result metadata
+  // and captured buffer of a frame are returned together in one shot.
+  uint32_t partial_result_count_;
+
+  // The shutter time of the first frame.  We derive the |timestamp| of a frame
+  // using the difference between the frame's shutter time and
+  // |first_frame_shutter_time_|.
+  base::TimeTicks first_frame_shutter_time_;
+
+  // Stores the partial capture results of the current in-flight frames.
+  std::map<uint32_t, CaptureResult> partial_results_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(CameraDeviceDelegate);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_VIDEO_CHROMEOS_CAMERA_DEVICE_DELEGATE_H_