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

media/capture/video/chromeos/video_capture_device_arc_chromeos.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/video_capture_device_arc_chromeos.h"
+
+#include "base/threading/platform_thread.h"
+#include "media/capture/video/chromeos/camera_device_delegate.h"
+#include "media/capture/video/chromeos/camera_hal_delegate.h"
+#include "ui/display/display.h"
+#include "ui/display/display_observer.h"
+#include "ui/display/screen.h"
+
+namespace media {
+
+// This is a delegate class used to transfer Display change events from the UI
+// thread to the media thread.

[chfremer, 2017/05/24 23:36:43]

What is "the media thread"?

[jcliang, 2017/05/25 14:23:47]

I believe it's the media capture thread on which the VCD interface methods are
called.

The ScreenObserverDelegate class is copied from
linux/video_capture_device_chromeos.cc:

https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_...

[chfremer, 2017/05/25 17:13:07]

Oh, I see. In that case, can we please avoid duplicating this code by moving the
class ScreenObserverDelegate to a pair of files of its own and reuse it in both
places? Instead of the concrete types for |capture_device|, use an new interface
DisplayRotationObserver and make both VideoCaptureDeviceArcChromeOS and
VideoCaptureDeviceChromeOS implement it.

IMHO, the reference to "media thread" is not a good idea, because it makes
assumptions on the context the class is used in. It would be better to just
describe what the class itself does independently of its context, which is
"Registers itself as an observer at |display::Screen::GetScreen()| and forwards
rotation change events to the given DisplayRotationObserver on the thread where
ScreenObserverDelegate was created."

[jcliang, 2017/05/26 04:33:33]

Agreed. I've extracted the common codes to display_rotation_observer.h/.cc.

I didn't extract the codes before because originally our plan was to completely
remove linux/video_capture_device_chromeos.h/.cc and migrate to
chromeos/video_capture_device_arc_chromeos.h/.cc, but now it looks like the two
VCDs have to co-exist for some time during the transition period.

+class VideoCaptureDeviceArcChromeOS::ScreenObserverDelegate
+    : public display::DisplayObserver,
+      public base::RefCountedThreadSafe<ScreenObserverDelegate> {
+ public:
+  // It is safe to use the raw pointer |capture_device| as
+  // VideoCaptureDeviceArcChromeOS owns the ScreenObserverDelegate instance and
+  // we make sure the VideoCaptureDeviceArcChromeOS instance outlives the
+  // ScreenObserverDelegate instance.

[chfremer, 2017/05/24 23:36:43]

I am not convinced that this is accurate.

Looking at the implementation, it seems that ~VideoCaptureDeviceArcChromeOS()
calls RemoveObserver(), which, in turn, posts to ui_task_runner with shared
ownership to |this|. Could it be that the SceenObserverDelegate does outlive
|capture_device| and the reason the raw pointer is safe is because we reset it
to nullptr in RemoveObserver()?

[jcliang, 2017/05/25 14:23:47]

Yes you are correct. I've updated the comments.

+  ScreenObserverDelegate(
+      VideoCaptureDeviceArcChromeOS* capture_device,
+      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
+      : capture_device_(capture_device),
+        ui_task_runner_(ui_task_runner),
+        capture_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&ScreenObserverDelegate::AddObserverOnUIThread, this));
+  }
+
+  void RemoveObserver() {
+    DCHECK(capture_task_runner_->BelongsToCurrentThread());
+    capture_device_ = nullptr;
+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&ScreenObserverDelegate::RemoveObserverOnUIThread, this));
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<ScreenObserverDelegate>;
+
+  ~ScreenObserverDelegate() override { DCHECK(!capture_device_); }
+
+  void OnDisplayAdded(const display::Display& /*new_display*/) override {}
+  void OnDisplayRemoved(const display::Display& /*old_display*/) override {}
+  void OnDisplayMetricsChanged(const display::Display& display,
+                               uint32_t metrics) override {
+    DCHECK(ui_task_runner_->BelongsToCurrentThread());
+    if (!(metrics & DISPLAY_METRIC_ROTATION))
+      return;
+    SendDisplayRotation(display);
+  }
+
+  void AddObserverOnUIThread() {
+    DCHECK(ui_task_runner_->BelongsToCurrentThread());
+    display::Screen* screen = display::Screen::GetScreen();

[chfremer, 2017/05/24 23:36:43]

When I see a dependency on a global like this, I get worried about testability.
It is probably okay to not unittest SceenObserverDelegate, since it is not very
complex.
Not sure about VideoCaptureDeviceArcChromeOS, though. We should consider
injecting a ScreenObserverDelegate instance into its constructor instead of
creating one in there. This would break the dependency on the global and allow
testing in environments where display::Screen::GetScreen() is not available.

[jcliang, 2017/05/25 14:23:47]

I agree having direct dependency on ScreenObserverDelegate in
VideoCaptureDeviceArcChromeOS is bad for testing. I've kept the logic in
VideoCaptureDeviceArcChromeOS simple and delegating all the heavy lifting to
CameraDeviceDelegate. I'm testing CameraDeviceDelegate instead.

If somehow we need to test VideoCaptureDeviceArcChromeOS I can refactor the code
in another CL. Since the implementation of ScreenObserverDelegate is copied from
linux/video_capture_device_chromeos.cc we should fix both places.

[chfremer, 2017/05/25 17:13:07]

Sounds good. Agreed that VideoCaptureDeviceArcChromeOS is a relatively thin
adapter class, which probably does not need to be tested in isolation.

+    if (screen) {
+      screen->AddObserver(this);
+      SendDisplayRotation(screen->GetPrimaryDisplay());
+    }
+  }
+
+  void RemoveObserverOnUIThread() {
+    DCHECK(ui_task_runner_->BelongsToCurrentThread());
+    display::Screen* screen = display::Screen::GetScreen();
+    if (screen)
+      screen->RemoveObserver(this);
+  }
+
+  // Post the screen rotation change from the UI thread to capture thread
+  void SendDisplayRotation(const display::Display& display) {
+    DCHECK(ui_task_runner_->BelongsToCurrentThread());
+    capture_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&ScreenObserverDelegate::SendDisplayRotationOnCaptureThread,
+                   this, display));
+  }
+
+  void SendDisplayRotationOnCaptureThread(const display::Display& display) {
+    DCHECK(capture_task_runner_->BelongsToCurrentThread());
+    if (capture_device_)
+      capture_device_->SetDisplayRotation(display);
+  }
+
+  VideoCaptureDeviceArcChromeOS* capture_device_;
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner_;
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ScreenObserverDelegate);
+};
+
+VideoCaptureDeviceArcChromeOS::VideoCaptureDeviceArcChromeOS(
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
+    const VideoCaptureDeviceDescriptor& device_descriptor,
+    scoped_refptr<CameraHalDelegate> camera_hal_delegate)
+    : device_descriptor_(device_descriptor),
+      camera_hal_delegate_(camera_hal_delegate),
+      capture_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      device_thread_("CameraDeviceThread"),
+      screen_observer_delegate_(
+          new ScreenObserverDelegate(this, std::move(ui_task_runner))),
+      lens_facing_(device_descriptor.facing),
+      camera_orientation_(0),
+      // External cameras have lens_facing as MEDIA_VIDEO_FACING_NONE.
+      // We don't want to rotate the frame even if the device rotates.
+      rotates_with_device_(lens_facing_ !=
+                           VideoFacingMode::MEDIA_VIDEO_FACING_NONE) {}
+
+VideoCaptureDeviceArcChromeOS::~VideoCaptureDeviceArcChromeOS() {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  DCHECK(!device_thread_.IsRunning());
+  screen_observer_delegate_->RemoveObserver();
+}
+
+// static
+VideoPixelFormat VideoCaptureDeviceArcChromeOS::PixFormatHalToChromium(
+    arc::mojom::HalPixelFormat from) {
+  return CameraDeviceDelegate::PixFormatHalToChromium(from);

[chfremer, 2017/05/24 23:36:43]

nit: Since these are public static both here and in CameraDeviceDelegate, we
could probably just extract them into a separate class that clients could use
directly. This would eliminate the need for these forwarding methods.

[jcliang, 2017/05/25 14:23:47]

Done. Moved to pixel_format_utils.h/.cc

+}
+
+// static
+uint32_t VideoCaptureDeviceArcChromeOS::PixFormatChromiumToDrm(
+    VideoPixelFormat from) {
+  return CameraDeviceDelegate::PixFormatChromiumToDrm(from);
+}
+
+// VideoCaptureDevice implementation.
+void VideoCaptureDeviceArcChromeOS::AllocateAndStart(
+    const VideoCaptureParams& params,
+    std::unique_ptr<Client> client) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  DCHECK(!camera_device_delegate_);
+
+  if (!device_thread_.Start()) {
+    std::string error_msg = "Failed to start device thread";
+    LOG(ERROR) << error_msg;
+    client->OnError(FROM_HERE, error_msg);
+    return;
+  }
+  camera_device_delegate_ = new CameraDeviceDelegate(
+      device_descriptor_, camera_hal_delegate_, device_thread_.task_runner());
+  device_thread_.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&CameraDeviceDelegate::AllocateAndStart,
+                 camera_device_delegate_, params, base::Passed(&client)));
+}
+
+void VideoCaptureDeviceArcChromeOS::StopAndDeAllocate() {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+

[wuchengli, 2017/05/23 02:56:02]

if (!camera_device_delegate_)
  return;

In case device_thread_ fails to start in line 139.

[jcliang, 2017/05/25 14:23:47]

Done.

+  DCHECK(camera_device_delegate_);
+  device_thread_.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&CameraDeviceDelegate::StopAndDeAllocate,
+                            camera_device_delegate_));
+  // Wait until all other references to |camera_device_delegate_| are dropped to
+  // make sure all the IPC calls are done.
+  base::TimeTicks wait_until =
+      base::TimeTicks() + base::TimeDelta::FromSeconds(3);
+  while (!camera_device_delegate_->HasOneRef()) {

[wuchengli, 2017/05/23 02:56:02]

Note to myself: need to think about this code.

[chfremer, 2017/05/24 23:36:43]

I also have a strange feeling about this spinning and polling. Feels like a we
are using the reference count as a hidden communication channel between this and
other modules. If we need to wait for other modules to release their references
here, could these modules send us a callback when they are done, instead?

[wuchengli, 2017/05/25 07:59:06]

Ricky. You can consider to move device_thread_ to CameraDeviceDelegate.
CameraDeviceDelegate can use WaitableEvenet internally.

[jcliang, 2017/05/25 14:23:47]

I'll look into it, but it'll likely be a sizable refactor. Let me post a new
patch set addressing other comments first.

+    if (base::TimeTicks() >= wait_until) {
+      LOG(ERROR) << "Timed out waiting capture device to stop";
+      break;
+    }
+    base::PlatformThread::YieldCurrentThread();
+  }
+  device_thread_.Stop();
+  camera_device_delegate_ = nullptr;
+}
+
+void VideoCaptureDeviceArcChromeOS::TakePhoto(TakePhotoCallback callback) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  DCHECK(camera_device_delegate_);
+  device_thread_.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&CameraDeviceDelegate::TakePhoto,
+                            camera_device_delegate_, base::Passed(&callback)));
+}
+
+void VideoCaptureDeviceArcChromeOS::GetPhotoCapabilities(
+    GetPhotoCapabilitiesCallback callback) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  device_thread_.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&CameraDeviceDelegate::GetPhotoCapabilities,
+                            camera_device_delegate_, base::Passed(&callback)));
+}
+
+void VideoCaptureDeviceArcChromeOS::SetPhotoOptions(
+    mojom::PhotoSettingsPtr settings,
+    SetPhotoOptionsCallback callback) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  device_thread_.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&CameraDeviceDelegate::SetPhotoOptions,
+                            camera_device_delegate_, base::Passed(&settings),
+                            base::Passed(&callback)));
+}
+
+void VideoCaptureDeviceArcChromeOS::SetRotation(int rotation) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  if (!rotates_with_device_) {
+    rotation = 0;
+  } else if (lens_facing_ == VideoFacingMode::MEDIA_VIDEO_FACING_ENVIRONMENT) {
+    // Original frame when |rotation| = 0
+    // -----------------------
+    // |          *          |
+    // |         * *         |
+    // |        *   *        |
+    // |       *******       |
+    // |      *       *      |
+    // |     *         *     |
+    // -----------------------
+    //
+    // |rotation| = 90, this is what back camera sees
+    // -----------------------
+    // |    ********         |
+    // |       *   ****      |
+    // |       *      ***    |
+    // |       *      ***    |
+    // |       *   ****      |
+    // |    ********         |
+    // -----------------------
+    //
+    // |rotation| = 90, this is what front camera sees
+    // -----------------------
+    // |         ********    |
+    // |      ****   *       |
+    // |    ***      *       |
+    // |    ***      *       |
+    // |      ****   *       |
+    // |         ********    |
+    // -----------------------
+    //
+    // Therefore, for back camera, we need to rotate (360 - |rotation|).
+    rotation = (360 - rotation) % 360;
+  }
+  // Take into account camera orientation w.r.t. the display. External cameras
+  // would have camera_orientation_ as 0.
+  rotation = (rotation + camera_orientation_) % 360;
+  if (device_thread_.IsRunning()) {

[chfremer, 2017/05/24 23:36:42]

This check seems to indicate that rotation events may arrive while the device is
stopped.
If that is the case, do we need to cache the rotation information and provide it
to the device once it is started?
Or does it get the rotation information from somewhere else when it is started?

[jcliang, 2017/05/25 14:23:47]

Oh yes! We should preserve the rotation information and set it to
|camera_device_delegate_| upon StartAndAllocate.

+    device_thread_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(&CameraDeviceDelegate::SetRotation,
+                              camera_device_delegate_, rotation));
+  }
+}
+
+void VideoCaptureDeviceArcChromeOS::SetDisplayRotation(
+    const display::Display& display) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+  if (display.IsInternal())
+    SetRotation(display.rotation() * 90);
+}
+
+}  // namespace media