[Fuchsia] Improve error handling in VideoCaptureDeviceFactoryFuchsia

Previously VideoCaptureDeviceFactoryFuchsia was logging an ERROR when
DeviceWatcher connection is disconnected, while this error is expected
in some environments. In this change:
  1. Fixed CastRunner to close DeviceWatcher with ZX_ERR_UNAVAILABLE
     when no applications have camera access.
  2. Fixed VideoCaptureDeviceFactoryFuchsia to avoid logging errors for
     ZX_ERR_UNAVAILABLE.
  3. Improved handling of disconnected state in
     VideoCaptureDeviceFactoryFuchsia: now it will reconnect the channel
     in CreateDevice().
  4. Added unittests for (3).

TBR=guidou@chromium.org

Bug: fuchsia:54060
Change-Id: Ib39fd1ef0d1a787f762bcd961196800105e45b97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246877
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790104}

fuchsia/runners/cast/cast_runner.cc

@@ -343,9 +343,11 @@ void CastRunner::OnCameraServiceRequest(
     return;
   }
 
-  LOG(WARNING) << "fuchsia.camera3.DeviceWatcher request was received while no "
-                  "apps with the CAMERA permission are running.";
-  // Drop the request.
+  // fuchsia.camera3.DeviceWatcher may be requested while none of the running
+  // apps have the CAMERA permission. Return ZX_ERR_UNAVAILABLE, which implies
+  // that the client should try connecting again later, since the service may
+  // become available after a web.Frame with camera access is created.
+  request.Close(ZX_ERR_UNAVAILABLE);
 }
 
 void CastRunner::OnMetricsRecorderServiceRequest(

media/capture/BUILD.gn

@@ -403,7 +403,10 @@ test("capture_unittests") {
       "//third_party/fuchsia-sdk/sdk/fidl/fuchsia.sysmem",
       "//third_party/fuchsia-sdk/sdk/pkg/sys_cpp",
     ]
-    sources += [ "video/fuchsia/video_capture_device_fuchsia_test.cc" ]
+    sources += [
+      "video/fuchsia/video_capture_device_factory_fuchsia_test.cc",
+      "video/fuchsia/video_capture_device_fuchsia_test.cc",
+    ]
   }
 
   if (is_win) {

media/capture/video/fuchsia/video_capture_device_factory_fuchsia.cc

@@ -128,6 +128,11 @@ VideoCaptureDeviceFactoryFuchsia::CreateDevice(
   if (!converted)
     return nullptr;
 
+  // CreateDevice() may be called before GetDeviceDescriptors(). Make sure
+  // |device_watcher_| is initialized.
+  if (!device_watcher_)
+    Initialize();
+
   fidl::InterfaceHandle<fuchsia::camera3::Device> device;
   device_watcher_->ConnectToDevice(device_id, device.NewRequest());
   return std::make_unique<VideoCaptureDeviceFuchsia>(std::move(device));
@@ -139,12 +144,14 @@ void VideoCaptureDeviceFactoryFuchsia::GetDeviceDescriptors(
 
   device_descriptors->clear();
 
-  if (!device_watcher_) {
-    DCHECK(!first_update_run_loop_);
-    DCHECK(devices_.empty());
-
+  if (!device_watcher_)
     Initialize();
 
+  // Even if |device_watcher_| was already connected, we may need need to wait
+  // for the first WatchDevices() response.
+  if (!received_first_update_) {
+    DCHECK(!first_update_run_loop_);
+
     // The RunLoop will quit when either we've received the first WatchDevices()
     // response or DeviceWatcher fails. |devices_| will be empty in case of a
     // failure.
@@ -185,6 +192,7 @@ void VideoCaptureDeviceFactoryFuchsia::Initialize() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(!device_watcher_);
   DCHECK(devices_.empty());
+  DCHECK(!received_first_update_);
 
   base::ComponentContextForProcess()->svc()->Connect(
       device_watcher_.NewRequest());
@@ -199,9 +207,20 @@ void VideoCaptureDeviceFactoryFuchsia::OnDeviceWatcherDisconnected(
     zx_status_t status) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
-  ZX_LOG(ERROR, status) << "fuchsia.camera3.DeviceWatcher disconnected.";
+  // CastRunner may close the channel with ZX_ERR_UNAVAILABLE error code when
+  // none of the running applications have access to camera. No need to log the
+  // error in that case.
+  if (status != ZX_ERR_UNAVAILABLE)
+    ZX_LOG(ERROR, status) << "fuchsia.camera3.DeviceWatcher disconnected.";
+
+  // Clear the list of devices and reset |received_first_update_| so we
+  // don't report any camera devices while DeviceWatcher is disconnected. We
+  // will try connecting DeviceWatcher again when GetDeviceDescriptors() is
+  // called.
   devices_.clear();
 
+  received_first_update_ = false;
+
   if (first_update_run_loop_)
     first_update_run_loop_->Quit();
 }
@@ -250,6 +269,8 @@ void VideoCaptureDeviceFactoryFuchsia::OnWatchDevicesResult(
         id, std::make_unique<DeviceInfoFetcher>(id, std::move(device)));
   }
 
+  received_first_update_ = true;
+
   if (first_update_run_loop_)
     first_update_run_loop_->Quit();
 

media/capture/video/fuchsia/video_capture_device_factory_fuchsia.h

@@ -51,6 +51,10 @@ class CAPTURE_EXPORT VideoCaptureDeviceFactoryFuchsia
   base::small_map<std::map<uint64_t, std::unique_ptr<DeviceInfoFetcher>>>
       devices_;
 
+  // Indicates whether the first update has been received from
+  // |device_watcher_|.
+  bool received_first_update_ = false;
+
   // RunLoop used to wait for the first WatchDevices() response. Currently
   // required because GetDeviceDescriptors() is synchronous.
   // TODO(crbug.com/1072932) Refactor interface to allow asynchronous

media/capture/video/fuchsia/video_capture_device_factory_fuchsia_test.cc

+// Copyright 2020 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/fuchsia/video_capture_device_factory_fuchsia.h"
+
+#include "base/fuchsia/test_component_context_for_process.h"
+#include "base/run_loop.h"
+#include "base/test/task_environment.h"
+#include "media/fuchsia/camera/fake_fuchsia_camera.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+class VideoCaptureDeviceFactoryFuchsiaTest : public testing::Test {
+ protected:
+  base::test::SingleThreadTaskEnvironment task_environment_{
+      base::test::SingleThreadTaskEnvironment::MainThreadType::IO};
+  base::TestComponentContextForProcess test_context_;
+
+  FakeCameraDeviceWatcher fake_device_watcher_{
+      test_context_.additional_services()};
+
+  VideoCaptureDeviceFactoryFuchsia device_factory_;
+};
+
+TEST_F(VideoCaptureDeviceFactoryFuchsiaTest, EnumerateDevices) {
+  media::VideoCaptureDeviceDescriptors device_descriptors;
+  device_factory_.GetDeviceDescriptors(&device_descriptors);
+
+  EXPECT_EQ(device_descriptors.size(), 1U);
+}
+
+TEST_F(VideoCaptureDeviceFactoryFuchsiaTest, EnumerateDevicesAfterDisconnect) {
+  media::VideoCaptureDeviceDescriptors device_descriptors;
+  device_factory_.GetDeviceDescriptors(&device_descriptors);
+
+  EXPECT_EQ(device_descriptors.size(), 1U);
+
+  device_descriptors.clear();
+
+  // Disconnect DeviceWatcher and run the run loop so |device_factory_| can
+  // handle the disconnect.
+  fake_device_watcher_.DisconnectClients();
+  base::RunLoop().RunUntilIdle();
+
+  // Try enumerating devices again. DeviceWatcher is expected to be reconnected.
+  device_factory_.GetDeviceDescriptors(&device_descriptors);
+
+  EXPECT_EQ(device_descriptors.size(), 1U);
+}
+
+}  // namespace media

media/capture/video/fuchsia/video_capture_device_fuchsia_test.cc

@@ -4,7 +4,9 @@
 
 #include "media/capture/video/fuchsia/video_capture_device_fuchsia.h"
 
+#include "base/fuchsia/test_component_context_for_process.h"
 #include "base/test/task_environment.h"
+#include "media/capture/video/fuchsia/video_capture_device_factory_fuchsia.h"
 #include "media/fuchsia/camera/fake_fuchsia_camera.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/geometry/rect.h"
@@ -188,15 +190,25 @@ class TestVideoCaptureClient : public VideoCaptureDevice::Client {
 class VideoCaptureDeviceFuchsiaTest : public testing::Test {
  public:
   VideoCaptureDeviceFuchsiaTest() {
-    fidl::InterfaceHandle<fuchsia::camera3::Device> device_handle;
-    fake_device_.Bind(device_handle.NewRequest());
-    device_ =
-        std::make_unique<VideoCaptureDeviceFuchsia>(std::move(device_handle));
+    test_context_.AddService("fuchsia.sysmem.Allocator");
   }
 
-  ~VideoCaptureDeviceFuchsiaTest() override { device_->StopAndDeAllocate(); }
+  ~VideoCaptureDeviceFuchsiaTest() override {
+    if (device_)
+      device_->StopAndDeAllocate();
+  }
+
+  void CreateDevice() {
+    VideoCaptureDeviceDescriptors device_descriptors;
+    device_factory_.GetDeviceDescriptors(&device_descriptors);
+    ASSERT_EQ(device_descriptors.size(), 1U);
+    device_ = device_factory_.CreateDevice(device_descriptors[0]);
+  }
 
   void StartCapturer() {
+    if (!device_)
+      CreateDevice();
+
     VideoCaptureParams params;
     params.requested_format.frame_size = FakeCameraStream::kDefaultFrameSize;
     params.requested_format.frame_rate = 30.0;
@@ -206,16 +218,19 @@ class VideoCaptureDeviceFuchsiaTest : public testing::Test {
     client_ = client.get();
     device_->AllocateAndStart(params, std::move(client));
 
-    EXPECT_TRUE(fake_stream_.WaitBuffersAllocated());
+    EXPECT_TRUE(fake_device_watcher_.stream()->WaitBuffersAllocated());
   }
 
  protected:
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::SingleThreadTaskEnvironment::MainThreadType::IO};
+  base::TestComponentContextForProcess test_context_;
 
-  FakeCameraStream fake_stream_;
-  FakeCameraDevice fake_device_{&fake_stream_};
-  std::unique_ptr<VideoCaptureDeviceFuchsia> device_;
+  FakeCameraDeviceWatcher fake_device_watcher_{
+      test_context_.additional_services()};
+
+  VideoCaptureDeviceFactoryFuchsia device_factory_;
+  std::unique_ptr<VideoCaptureDevice> device_;
   TestVideoCaptureClient* client_ = nullptr;
 };
 
@@ -227,7 +242,7 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, SendFrame) {
   StartCapturer();
 
   auto frame_timestamp = base::TimeTicks::Now();
-  fake_stream_.ProduceFrame(frame_timestamp, 1);
+  fake_device_watcher_.stream()->ProduceFrame(frame_timestamp, 1);
   client_->WaitFrame();
 
   ASSERT_EQ(client_->received_frames().size(), 1U);
@@ -239,14 +254,14 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, SendFrame) {
 TEST_F(VideoCaptureDeviceFuchsiaTest, MultipleFrames) {
   StartCapturer();
 
-  EXPECT_TRUE(fake_stream_.WaitBuffersAllocated());
+  EXPECT_TRUE(fake_device_watcher_.stream()->WaitBuffersAllocated());
 
   for (size_t i = 0; i < 10; ++i) {
-    ASSERT_TRUE(fake_stream_.WaitFreeBuffer());
+    ASSERT_TRUE(fake_device_watcher_.stream()->WaitFreeBuffer());
 
     auto frame_timestamp =
         base::TimeTicks() + base::TimeDelta::FromMilliseconds(i * 16);
-    fake_stream_.ProduceFrame(frame_timestamp, i);
+    fake_device_watcher_.stream()->ProduceFrame(frame_timestamp, i);
     client_->WaitFrame();
 
     ASSERT_EQ(client_->received_frames().size(), i + 1);
@@ -258,11 +273,11 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, MultipleFrames) {
 
 TEST_F(VideoCaptureDeviceFuchsiaTest, FrameRotation) {
   const gfx::Size kResolution(4, 2);
-  fake_stream_.SetFakeResolution(kResolution);
+  fake_device_watcher_.stream()->SetFakeResolution(kResolution);
 
   StartCapturer();
 
-  EXPECT_TRUE(fake_stream_.WaitBuffersAllocated());
+  EXPECT_TRUE(fake_device_watcher_.stream()->WaitBuffersAllocated());
 
   for (int i = static_cast<int>(fuchsia::camera3::Orientation::UP);
        i <= static_cast<int>(fuchsia::camera3::Orientation::RIGHT_FLIPPED);
@@ -271,9 +286,9 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, FrameRotation) {
 
     auto orientation = static_cast<fuchsia::camera3::Orientation>(i);
 
-    ASSERT_TRUE(fake_stream_.WaitFreeBuffer());
-    fake_stream_.SetFakeOrientation(orientation);
-    fake_stream_.ProduceFrame(base::TimeTicks::Now(), i);
+    ASSERT_TRUE(fake_device_watcher_.stream()->WaitFreeBuffer());
+    fake_device_watcher_.stream()->SetFakeOrientation(orientation);
+    fake_device_watcher_.stream()->ProduceFrame(base::TimeTicks::Now(), i);
     client_->WaitFrame();
 
     gfx::Size expected_size = kResolution;
@@ -289,11 +304,11 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, FrameRotation) {
 
 TEST_F(VideoCaptureDeviceFuchsiaTest, FrameDimensionsNotDivisibleBy2) {
   const gfx::Size kOddResolution(21, 7);
-  fake_stream_.SetFakeResolution(kOddResolution);
+  fake_device_watcher_.stream()->SetFakeResolution(kOddResolution);
 
   StartCapturer();
 
-  fake_stream_.ProduceFrame(base::TimeTicks::Now(), 1);
+  fake_device_watcher_.stream()->ProduceFrame(base::TimeTicks::Now(), 1);
   client_->WaitFrame();
 
   ASSERT_EQ(client_->received_frames().size(), 1U);
@@ -304,14 +319,14 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, MidStreamResolutionChange) {
   StartCapturer();
 
   // Capture the first frame at the default resolution.
-  fake_stream_.ProduceFrame(base::TimeTicks::Now(), 1);
+  fake_device_watcher_.stream()->ProduceFrame(base::TimeTicks::Now(), 1);
   client_->WaitFrame();
-  ASSERT_TRUE(fake_stream_.WaitFreeBuffer());
+  ASSERT_TRUE(fake_device_watcher_.stream()->WaitFreeBuffer());
 
   // Update resolution and produce another frames.
   const gfx::Size kUpdatedResolution(3, 14);
-  fake_stream_.SetFakeResolution(kUpdatedResolution);
-  fake_stream_.ProduceFrame(base::TimeTicks::Now(), 1);
+  fake_device_watcher_.stream()->SetFakeResolution(kUpdatedResolution);
+  fake_device_watcher_.stream()->ProduceFrame(base::TimeTicks::Now(), 1);
   client_->WaitFrame();
 
   // Verify that we get captured frames with correct resolution.
@@ -321,4 +336,26 @@ TEST_F(VideoCaptureDeviceFuchsiaTest, MidStreamResolutionChange) {
   ValidateReceivedFrame(client_->received_frames()[1], kUpdatedResolution, 1);
 }
 
+TEST_F(VideoCaptureDeviceFuchsiaTest,
+       CreateDeviceAfterDeviceWatcherDisconnect) {
+  VideoCaptureDeviceDescriptors device_descriptors;
+  device_factory_.GetDeviceDescriptors(&device_descriptors);
+  ASSERT_EQ(device_descriptors.size(), 1U);
+
+  // Disconnect DeviceWatcher and run the run loop so |device_factory_| can
+  // handle the disconnect.
+  fake_device_watcher_.DisconnectClients();
+  base::RunLoop().RunUntilIdle();
+
+  // The factory is expected to reconnect DeviceWatcher.
+  device_ = device_factory_.CreateDevice(device_descriptors[0]);
+
+  StartCapturer();
+
+  fake_device_watcher_.stream()->ProduceFrame(base::TimeTicks::Now(), 1);
+  client_->WaitFrame();
+
+  ASSERT_EQ(client_->received_frames().size(), 1U);
+}
+
 }  // namespace media

media/fuchsia/camera/fake_fuchsia_camera.cc

@@ -439,13 +439,13 @@ void FakeCameraStream::OnZxHandleSignalled(zx_handle_t handle,
     wait_free_buffer_run_loop_->Quit();
 }
 FakeCameraDevice::FakeCameraDevice(FakeCameraStream* stream)
-    : binding_(this), stream_(stream) {}
+    : stream_(stream) {}
 
 FakeCameraDevice::~FakeCameraDevice() = default;
 
 void FakeCameraDevice::Bind(
     fidl::InterfaceRequest<fuchsia::camera3::Device> request) {
-  binding_.Bind(std::move(request));
+  bindings_.AddBinding(this, std::move(request));
 }
 
 void FakeCameraDevice::GetIdentifier(GetIdentifierCallback callback) {
@@ -487,6 +487,10 @@ FakeCameraDeviceWatcher::FakeCameraDeviceWatcher(
 
 FakeCameraDeviceWatcher::~FakeCameraDeviceWatcher() = default;
 
+void FakeCameraDeviceWatcher::DisconnectClients() {
+  bindings_.CloseAll();
+}
+
 FakeCameraDeviceWatcher::Client::Client(FakeCameraDevice* device)
     : device_(device) {}
 FakeCameraDeviceWatcher::Client::~Client() {}

media/fuchsia/camera/fake_fuchsia_camera.h

@@ -147,7 +147,7 @@ class FakeCameraDevice : public fuchsia::camera3::testing::Device_TestBase {
   // fuchsia::camera3::testing::Device_TestBase override.
   void NotImplemented_(const std::string& name) override;
 
-  fidl::Binding<fuchsia::camera3::Device> binding_;
+  fidl::BindingSet<fuchsia::camera3::Device> bindings_;
   FakeCameraStream* const stream_;
 };
 
@@ -159,6 +159,10 @@ class FakeCameraDeviceWatcher {
   FakeCameraDeviceWatcher(const FakeCameraDeviceWatcher&) = delete;
   FakeCameraDeviceWatcher& operator=(const FakeCameraDeviceWatcher&) = delete;
 
+  void DisconnectClients();
+
+  FakeCameraStream* stream() { return &stream_; }
+
  private:
   class Client : public fuchsia::camera3::testing::DeviceWatcher_TestBase {
    public: