getUserMedia: handle the device starting status report.

content/renderer/media/video_capture_impl_unittest.cc

 // Copyright (c) 2012 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 <stddef.h>
 
 #include "base/macros.h"
 #include "base/memory/shared_memory.h"
 #include "base/message_loop/message_loop.h"
 #include "content/child/child_process.h"
 #include "content/common/video_capture.mojom.h"
 #include "content/renderer/media/video_capture_impl.h"
 #include "mojo/public/cpp/system/platform_handle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
+using ::testing::InSequence;
 using ::testing::Invoke;
 using ::testing::InvokeWithoutArgs;
 using ::testing::SaveArg;
 using ::testing::WithArgs;
 
 namespace content {
 
 const int kSessionId = 11;
 
 void RunEmptyFormatsCallback(const VideoCaptureDeviceFormatsCB& callback) {
@@ skipping 141 matching lines @@
   media::VideoCaptureParams params_small_;
   media::VideoCaptureParams params_large_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureImplTest);
 };
 
 TEST_F(VideoCaptureImplTest, Simple) {
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED));
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  // Simulate the device started event here (and for all tests below), since we
+  // can't call the |onStateChange| through |mojom::VideoCaptureObserverPtr| in
+  // |MockMojoVideoCaptureHost::Start()| by far.

[chfremer, 2017/03/02 17:56:10]

I understand the added code in the EXPECT_CALL, and I also understand why it is
needed. But I do not understand the explanation given in this comment. I would
be okay with just removing it.

[braveyao, 2017/03/03 17:53:12]

Done.

+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));

[chfremer, 2017/03/02 17:56:10]

Instead of adding this to all the test cases, you may be able to do an
ON_CALL(...).WillByDefault(...) in the constructor or SetUp() method of
VideoCaptureImplTest.

[braveyao, 2017/03/03 17:53:12]

Done.
Thanks a lot for the suggestion!

   EXPECT_CALL(mock_video_capture_host_, Stop(_));
 
   StartCapture(0, params_small_);
   StopCapture(0);
 }
 
 TEST_F(VideoCaptureImplTest, TwoClientsInSequence) {
-  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED)).Times(2);
+  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
 
   StartCapture(0, params_small_);
   StopCapture(0);
   StartCapture(1, params_small_);
   StopCapture(1);
 }
 
 TEST_F(VideoCaptureImplTest, LargeAndSmall) {
-  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED)).Times(2);
+  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_large_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_large_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
 
   StartCapture(0, params_large_);
   StopCapture(0);
   StartCapture(1, params_small_);
   StopCapture(1);
 }
 
 TEST_F(VideoCaptureImplTest, SmallAndLarge) {
-  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED)).Times(2);
+  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
 
   StartCapture(0, params_small_);
   StopCapture(0);
   StartCapture(1, params_large_);
   StopCapture(1);
 }
 
 // Checks that a request to GetDeviceSupportedFormats() ends up eventually in
 // the provided callback.
@@ skipping 31 matching lines @@
   const int kBufferId = 11;
 
   base::SharedMemory shm;
   const size_t frame_size = media::VideoFrame::AllocationSize(
       media::PIXEL_FORMAT_I420, params_small_.requested_format.frame_size);
   ASSERT_TRUE(shm.CreateAndMapAnonymous(frame_size));
 
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED));
   EXPECT_CALL(*this, OnFrameReady(_, _));
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
   EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, kBufferId, _))
       .Times(0);
 
   StartCapture(0, params_small_);
   SimulateOnBufferCreated(kBufferId, shm);
   SimulateBufferReceived(kBufferId, params_small_.requested_format.frame_size);
   StopCapture(0);
   SimulateBufferDestroyed(kBufferId);
 
   EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 0);
 }
 
 TEST_F(VideoCaptureImplTest, BufferReceivedAfterStop) {
   const int kBufferId = 12;
 
   base::SharedMemory shm;
   const size_t frame_size = media::VideoFrame::AllocationSize(
       media::PIXEL_FORMAT_I420, params_large_.requested_format.frame_size);
   ASSERT_TRUE(shm.CreateAndMapAnonymous(frame_size));
 
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED));
   EXPECT_CALL(*this, OnFrameReady(_, _)).Times(0);
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_large_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_large_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
   EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, kBufferId, _));
 
   StartCapture(0, params_large_);
   SimulateOnBufferCreated(kBufferId, shm);
   StopCapture(0);
   // A buffer received after StopCapture() triggers an instant ReleaseBuffer().
   SimulateBufferReceived(kBufferId, params_large_.requested_format.frame_size);
   SimulateBufferDestroyed(kBufferId);
 
   EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 1);
 }
 
 TEST_F(VideoCaptureImplTest, AlreadyStarted) {
   media::VideoCaptureParams params = {};
-  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED)).Times(2);
+  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, _))
-      .WillOnce(SaveArg<2>(&params));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(DoAll(InvokeWithoutArgs([this]() {
+                        video_capture_impl_->OnStateChanged(
+                            mojom::VideoCaptureState::STARTED);
+                      }),
+                      SaveArg<2>(&params)));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
 
   StartCapture(0, params_small_);
   StartCapture(1, params_large_);
   StopCapture(0);
   StopCapture(1);
   DCHECK(params.requested_format == params_small_.requested_format);
 }
 
 TEST_F(VideoCaptureImplTest, EndedBeforeStop) {
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED));
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
 
   StartCapture(0, params_small_);
 
   OnStateChanged(mojom::VideoCaptureState::ENDED);
 
   StopCapture(0);
 }
 
 TEST_F(VideoCaptureImplTest, ErrorBeforeStop) {
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
   EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_ERROR));
-  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_))
+      .WillOnce(InvokeWithoutArgs([this]() {
+        video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+      }));
 
   StartCapture(0, params_small_);
 
   OnStateChanged(mojom::VideoCaptureState::FAILED);
 
   StopCapture(0);
 }
 
+TEST_F(VideoCaptureImplTest, BufferReceivedBeforeOnStarted) {

[chfremer, 2017/03/02 17:56:10]

Great to see a test case for this!

I can think of at least two more test cases worth testing.
1.) We want to verify that no RequestRefreshFrame() is sent in case that no
frames arrived before the OnStateChanged(STARTED) call.
2.) Assuming it is possible that additional OnStateChanged(STARTED) calls arrive
after the first one, we want to verify what happens in such cases.

[braveyao, 2017/03/03 17:53:12]

Done.

+  const int kBufferId = 16;
+
+  base::SharedMemory shm;
+  const size_t frame_size = media::VideoFrame::AllocationSize(
+      media::PIXEL_FORMAT_I420, params_small_.requested_format.frame_size);
+  ASSERT_TRUE(shm.CreateAndMapAnonymous(frame_size));
+
+  InSequence s;
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, kBufferId, _));

[chfremer, 2017/03/02 17:56:10]

Are these two EXPECT_CALL relevant for this test case? If not, please remove.

[braveyao, 2017/03/03 17:53:12]

Kinda. 
I want to verify that the frame is received and released correctly.

+  StartCapture(0, params_small_);
+  SimulateOnBufferCreated(kBufferId, shm);
+  SimulateBufferReceived(kBufferId, params_small_.requested_format.frame_size);
+
+  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STARTED));
+  EXPECT_CALL(mock_video_capture_host_, RequestRefreshFrame(_));
+  video_capture_impl_->OnStateChanged(mojom::VideoCaptureState::STARTED);
+
+  EXPECT_CALL(*this, OnStateUpdate(VIDEO_CAPTURE_STATE_STOPPED));
+  EXPECT_CALL(mock_video_capture_host_, Stop(_));
+  StopCapture(0);
+  SimulateBufferDestroyed(kBufferId);
+
+  EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 1);

[chfremer, 2017/03/02 17:56:10]

Is all of the shutdown logic starting at line 394 relevant for this test case?
If not, please remove.

[braveyao, 2017/03/03 17:53:12]

StopCapture and the above two lines are needed for this unittest. Remove others.

+}
+
 }  // namespace content