getUserMedia: handle the device starting status report.

content/renderer/media/video_capture_impl_unittest.cc

Index: content/renderer/media/video_capture_impl_unittest.cc
diff --git a/content/renderer/media/video_capture_impl_unittest.cc b/content/renderer/media/video_capture_impl_unittest.cc
index f130682f5367728943b054dad3a018f61f1d8a16..e91a7999cad5371e519e8bc13bcff64b643b1ed6 100644
--- a/content/renderer/media/video_capture_impl_unittest.cc
+++ b/content/renderer/media/video_capture_impl_unittest.cc
@@ -15,6 +15,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
+using ::testing::InSequence;
 using ::testing::Invoke;
 using ::testing::InvokeWithoutArgs;
 using ::testing::SaveArg;
@@ -176,7 +177,13 @@ class VideoCaptureImplTest : public ::testing::Test {
 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_);
@@ -184,9 +191,12 @@ TEST_F(VideoCaptureImplTest, Simple) {
 }
 
 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_);
@@ -196,9 +206,12 @@ TEST_F(VideoCaptureImplTest, TwoClientsInSequence) {
 }
 
 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_);
@@ -208,9 +221,12 @@ TEST_F(VideoCaptureImplTest, LargeAndSmall) {
 }
 
 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_);
@@ -262,7 +278,10 @@ TEST_F(VideoCaptureImplTest, BufferReceived) {
   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);
@@ -287,7 +306,10 @@ TEST_F(VideoCaptureImplTest, BufferReceivedAfterStop) {
   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, _));
 
@@ -303,10 +325,14 @@ TEST_F(VideoCaptureImplTest, BufferReceivedAfterStop) {
 
 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_);
@@ -319,7 +345,10 @@ TEST_F(VideoCaptureImplTest, AlreadyStarted) {
 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_);
 
@@ -331,7 +360,10 @@ TEST_F(VideoCaptureImplTest, EndedBeforeStop) {
 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_);
 
@@ -340,4 +372,31 @@ TEST_F(VideoCaptureImplTest, ErrorBeforeStop) {
   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