media: handle StopAndDeAllocate after suspend correctly

Some application (e.g. Hangouts) closes the camera explicitly on system
suspend. As the Chrome OS VCD also closes the camera on system suspend
without going through the full destruction of the VideoCaptureDevice,
this results in StopAndDeAllocate being called back-to-back. The second
call to StopAndDeAllocate should simply return if the device context is
already destroyed.

BUG=b:73436939
TEST=unit tests
TEST=Verify that Hangouts call works correctly on Soraka when system
     suspends

Change-Id: Id6127b751ccc5bc71538d622bc0905bf03951e0d
Reviewed-on: https://chromium-review.googlesource.com/930605Reviewed-by: Wu-Cheng Li <wuchengli@chromium.org>
Commit-Queue: Ricky Liang <jcliang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539091}

media/capture/video/chromeos/camera_device_delegate.cc

@@ -86,14 +86,13 @@ void CameraDeviceDelegate::AllocateAndStart(
 }
 
 void CameraDeviceDelegate::StopAndDeAllocate(
-    base::Closure device_close_callback) {
+    base::OnceClosure device_close_callback) {
   DCHECK(ipc_task_runner_->BelongsToCurrentThread());
-  // StopAndDeAllocate may be called at any state except
-  // CameraDeviceContext::State::kStopping.
-  DCHECK_NE(device_context_->GetState(), CameraDeviceContext::State::kStopping);
 
-  if (device_context_->GetState() == CameraDeviceContext::State::kStopped ||
-      !stream_buffer_manager_) {
+  if (!device_context_ ||
+      device_context_->GetState() == CameraDeviceContext::State::kStopped ||
+      (device_context_->GetState() == CameraDeviceContext::State::kError &&
+       !stream_buffer_manager_)) {
     // In case of Mojo connection error the device may be stopped before
     // StopAndDeAllocate is called; in case of device open failure, the state
     // is set to kError and |stream_buffer_manager_| is uninitialized.
@@ -101,6 +100,10 @@ void CameraDeviceDelegate::StopAndDeAllocate(
     return;
   }
 
+  // StopAndDeAllocate may be called at any state except
+  // CameraDeviceContext::State::kStopping.
+  DCHECK_NE(device_context_->GetState(), CameraDeviceContext::State::kStopping);
+
   device_close_callback_ = std::move(device_close_callback);
   device_context_->SetState(CameraDeviceContext::State::kStopping);
   if (!device_ops_.is_bound()) {

media/capture/video/chromeos/camera_device_delegate.h

@@ -66,7 +66,7 @@ class CAPTURE_EXPORT CameraDeviceDelegate final {
   // Delegation methods for the VideoCaptureDevice interface.
   void AllocateAndStart(const VideoCaptureParams& params,
                         CameraDeviceContext* device_context);
-  void StopAndDeAllocate(base::Closure device_close_callback);
+  void StopAndDeAllocate(base::OnceClosure device_close_callback);
   void TakePhoto(VideoCaptureDevice::TakePhotoCallback callback);
   void GetPhotoState(VideoCaptureDevice::GetPhotoStateCallback callback);
   void SetPhotoOptions(mojom::PhotoSettingsPtr settings,
@@ -162,7 +162,7 @@ class CAPTURE_EXPORT CameraDeviceDelegate final {
   // Where all the Mojo IPC calls takes place.
   const scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner_;
 
-  base::Closure device_close_callback_;
+  base::OnceClosure device_close_callback_;
 
   base::WeakPtrFactory<CameraDeviceDelegate> weak_ptr_factory_;
 

media/capture/video/chromeos/camera_device_delegate_unittest.cc

@@ -576,4 +576,46 @@ TEST_F(CameraDeviceDelegateTest, FailToOpenDevice) {
   ResetDevice();
 }
 
+// Test that the class handles it correctly when StopAndDeAllocate is called
+// multiple times.
+TEST_F(CameraDeviceDelegateTest, DoubleStopAndDeAllocate) {
+  AllocateDeviceWithDescriptor(kDefaultDescriptor);
+
+  VideoCaptureParams params;
+  params.requested_format = kDefaultCaptureFormat;
+
+  auto* mock_client = ResetDeviceContext();
+  mock_client->SetFrameCb(BindToCurrentLoop(base::BindOnce(
+      &CameraDeviceDelegateTest::QuitRunLoop, base::Unretained(this))));
+  mock_client->SetQuitCb(BindToCurrentLoop(base::BindOnce(
+      &CameraDeviceDelegateTest::QuitRunLoop, base::Unretained(this))));
+  SetUpExpectationUntilCapturing(mock_client);
+  SetUpExpectationForCaptureLoop();
+
+  device_delegate_thread_.task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&CameraDeviceDelegate::AllocateAndStart,
+                                camera_device_delegate_->GetWeakPtr(), params,
+                                base::Unretained(device_context_.get())));
+
+  // Wait until a frame is received.  MockVideoCaptureClient calls QuitRunLoop()
+  // to stop the run loop.
+  DoLoop();
+
+  EXPECT_EQ(CameraDeviceContext::State::kCapturing, GetState());
+
+  SetUpExpectationForClose();
+
+  WaitForDeviceToClose();
+
+  device_delegate_thread_.task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&CameraDeviceDelegate::StopAndDeAllocate,
+                                camera_device_delegate_->GetWeakPtr(),
+                                BindToCurrentLoop(base::BindOnce(
+                                    &CameraDeviceDelegateTest::QuitRunLoop,
+                                    base::Unretained(this)))));
+  DoLoop();
+
+  ResetDevice();
+}
+
 }  // namespace media

media/capture/video/chromeos/video_capture_device_arc_chromeos.cc

@@ -138,6 +138,9 @@ void VideoCaptureDeviceArcChromeOS::SuspendDone(
 void VideoCaptureDeviceArcChromeOS::OpenDevice() {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
+  if (!camera_device_delegate_) {
+    return;
+  }
   // It's safe to pass unretained |device_context_| here since
   // VideoCaptureDeviceArcChromeOS owns |camera_device_delegate_| and makes
   // sure |device_context_| outlives |camera_device_delegate_|.