VideoCapture: migrate last Renderer-->Host msg and start Host-->Renderer migration

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/media/video_capture_messages.h"
 #include "content/common/video_capture.mojom.h"
 #include "content/renderer/media/video_capture_impl.h"
 #include "media/base/bind_to_current_loop.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
 using ::testing::Invoke;
+using ::testing::InvokeWithoutArgs;
 using ::testing::SaveArg;
 using ::testing::WithArgs;
 
 namespace content {
 
 const int kSessionId = 1;
 
 void RunEmptyFormatsCallback(const VideoCaptureDeviceFormatsCB& callback) {
   media::VideoCaptureFormats formats;
   callback.Run(formats);
 }
 
 // Mock implementation of the Mojo service. TODO(mcasas): Replace completely
 // MockVideoCaptureMessageFilter, https://crbug.com/651897
 class MockMojoVideoCaptureHost : public mojom::VideoCaptureHost {
  public:
-  MockMojoVideoCaptureHost() {
+  MockMojoVideoCaptureHost() : released_buffer_count_(0) {
     ON_CALL(*this, GetDeviceSupportedFormats(_, _, _))
         .WillByDefault(WithArgs<2>(Invoke(RunEmptyFormatsCallback)));
     ON_CALL(*this, GetDeviceFormatsInUse(_, _, _))
         .WillByDefault(WithArgs<2>(Invoke(RunEmptyFormatsCallback)));
+    ON_CALL(*this, ReleaseBuffer(_, _, _, _))
+        .WillByDefault(InvokeWithoutArgs(
+            this, &MockMojoVideoCaptureHost::increase_released_buffer_count));
   }
 
-  MOCK_METHOD3(Start, void(int32_t, int32_t, const media::VideoCaptureParams&));
+  // Start() can't be mocked directly due to move-only |observer|.
+  void Start(int32_t device_id,
+             int32_t session_id,
+             const media::VideoCaptureParams& params,
+             mojom::VideoCaptureObserverPtr observer) override {
+    DoStart(device_id, session_id, params);
+  }
+  MOCK_METHOD3(DoStart,
+               void(int32_t, int32_t, const media::VideoCaptureParams&));
   MOCK_METHOD1(Stop, void(int32_t));
   MOCK_METHOD1(Pause, void(int32_t));
   MOCK_METHOD3(Resume,
                void(int32_t, int32_t, const media::VideoCaptureParams&));
   MOCK_METHOD1(RequestRefreshFrame, void(int32_t));
+  MOCK_METHOD4(ReleaseBuffer,
+               void(int32_t, int32_t, const gpu::SyncToken&, double));
   MOCK_METHOD3(GetDeviceSupportedFormats,
                void(int32_t,
                     int32_t,
                     const GetDeviceSupportedFormatsCallback&));
   MOCK_METHOD3(GetDeviceFormatsInUse,
                void(int32_t, int32_t, const GetDeviceFormatsInUseCallback&));
 
+  int released_buffer_count() const { return released_buffer_count_; }
+  void increase_released_buffer_count() { released_buffer_count_++; }
+
  private:
+  int released_buffer_count_;
+
   DISALLOW_COPY_AND_ASSIGN(MockMojoVideoCaptureHost);
 };
 
 class MockVideoCaptureMessageFilter : public VideoCaptureMessageFilter {
  public:
   MockVideoCaptureMessageFilter() : VideoCaptureMessageFilter() {}
 
   // Filter implementation.
   MOCK_METHOD1(Send, bool(IPC::Message* message));
 
  protected:
   virtual ~MockVideoCaptureMessageFilter() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockVideoCaptureMessageFilter);
 };
 
 class VideoCaptureImplTest : public ::testing::Test {
  public:
-  class MockVideoCaptureImpl : public VideoCaptureImpl {
-   public:
-    MockVideoCaptureImpl(const media::VideoCaptureSessionId id,
-                         VideoCaptureMessageFilter* filter)
-        : VideoCaptureImpl(id, filter, base::ThreadTaskRunnerHandle::Get()),
-          received_buffer_count_(0) {}
-    ~MockVideoCaptureImpl() override {}
-
-    void Send(IPC::Message* message) override {
-      bool handled = true;
-      IPC_BEGIN_MESSAGE_MAP(MockVideoCaptureImpl, *message)
-        IPC_MESSAGE_HANDLER(VideoCaptureHostMsg_BufferReady,
-                            DeviceReceiveEmptyBuffer)
-        IPC_MESSAGE_UNHANDLED(handled = false)
-      IPC_END_MESSAGE_MAP()
-      EXPECT_TRUE(handled);
-      delete message;
-    }
-
-    void DeviceReceiveEmptyBuffer(int device_id,
-                                  int buffer_id,
-                                  const gpu::SyncToken& release_sync_token,
-                                  double consumer_resource_utilization) {
-      received_buffer_count_++;
-    }
-
-    void ReceiveStateChangeMessage(VideoCaptureState state) {
-      OnStateChanged(state);
-    }
-
-    int received_buffer_count() const { return received_buffer_count_; }
-
-   private:
-    int received_buffer_count_;
-  };
-
   VideoCaptureImplTest()
       : message_filter_(new MockVideoCaptureMessageFilter),
         video_capture_impl_(
-            new MockVideoCaptureImpl(kSessionId, message_filter_.get())) {
+            new VideoCaptureImpl(kSessionId,
+                                 message_filter_.get(),
+                                 base::ThreadTaskRunnerHandle::Get())) {
     params_small_.requested_format = media::VideoCaptureFormat(
         gfx::Size(176, 144), 30, media::PIXEL_FORMAT_I420);
     params_large_.requested_format = media::VideoCaptureFormat(
         gfx::Size(320, 240), 30, media::PIXEL_FORMAT_I420);
 
     video_capture_impl_->SetVideoCaptureHostForTesting(
         &mock_video_capture_host_);
     video_capture_impl_->device_id_ = 2;
   }
 
  protected:
+  // These four mocks are used to create callbacks for the different oeprations.
   MOCK_METHOD2(OnFrameReady,
                void(const scoped_refptr<media::VideoFrame>&, base::TimeTicks));
   MOCK_METHOD1(OnStateUpdate, void(VideoCaptureState));
   MOCK_METHOD1(OnDeviceFormatsInUse, void(const media::VideoCaptureFormats&));
   MOCK_METHOD1(OnDeviceSupportedFormats,
                void(const media::VideoCaptureFormats&));
 
   void StartCapture(int client_id, const media::VideoCaptureParams& params) {
     video_capture_impl_->StartCapture(
         client_id, params, base::Bind(&VideoCaptureImplTest::OnStateUpdate,
@@ skipping 10 matching lines @@
     video_capture_impl_->OnBufferCreated(
         base::SharedMemory::DuplicateHandle(shm.handle()),
         shm.mapped_size(), buffer_id);
   }
 
   void BufferReceived(int buffer_id, const gfx::Size& size) {
     base::TimeTicks now = base::TimeTicks::Now();
     base::TimeDelta timestamp = now - base::TimeTicks();
 
     media::VideoFrameMetadata frame_metadata;
-    frame_metadata.SetTimeTicks(media::VideoFrameMetadata::REFERENCE_TIME,
-                                now);
+    frame_metadata.SetTimeTicks(media::VideoFrameMetadata::REFERENCE_TIME, now);
     base::DictionaryValue metadata;
     frame_metadata.MergeInternalValuesInto(&metadata);
 
     video_capture_impl_->OnBufferReceived(
         buffer_id, timestamp, metadata, media::PIXEL_FORMAT_I420,
         media::VideoFrame::STORAGE_SHMEM, size, gfx::Rect(size));
   }
 
   void BufferDestroyed(int buffer_id) {
     video_capture_impl_->OnBufferDestroyed(buffer_id);
   }
 
   void GetDeviceSupportedFormats() {
     const base::Callback<void(const media::VideoCaptureFormats&)>
         callback = base::Bind(
             &VideoCaptureImplTest::OnDeviceSupportedFormats,
             base::Unretained(this));
     video_capture_impl_->GetDeviceSupportedFormats(callback);
   }
 
   void GetDeviceFormatsInUse() {
     const base::Callback<void(const media::VideoCaptureFormats&)>
         callback = base::Bind(
             &VideoCaptureImplTest::OnDeviceFormatsInUse,
             base::Unretained(this));
     video_capture_impl_->GetDeviceFormatsInUse(callback);
   }
 
+  void OnStateChanged(mojom::VideoCaptureState state) {
+    video_capture_impl_->OnStateChanged(state);
+  }
+
   const base::MessageLoop message_loop_;
   const ChildProcess child_process_;
   const scoped_refptr<MockVideoCaptureMessageFilter> message_filter_;
-  std::unique_ptr<MockVideoCaptureImpl> video_capture_impl_;
+  const std::unique_ptr<VideoCaptureImpl> video_capture_impl_;
   MockMojoVideoCaptureHost mock_video_capture_host_;
   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_, Start(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
   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_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, Start(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));

[chfremer, 2016/10/10 18:57:19]

Here and in the two tests below, why do we only expect a single invocation of
DoStart() and Stop()? Shouldn't we be expecting a sequence of invocations?

[mcasas, 2016/10/10 19:39:44]

It's misleading, but due to the current implementation, 
the first cycle of start-stop does not set VideoCaptureImpl
back in a STOPPED state, but in a STOPPING, which
essentially delays the next start-stop beyond the limits
of this unittest.  Probably something to address, but
not in this CL.

https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...

   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_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, Start(_, kSessionId, params_large_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_large_));
   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_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, Start(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
   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 29 matching lines @@
 
 TEST_F(VideoCaptureImplTest, BufferReceived) {
   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_, Start(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
+  EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, _, _, _)).Times(0);
 
   StartCapture(0, params_small_);
   NewBuffer(0, shm);
   BufferReceived(0, params_small_.requested_format.frame_size);
   StopCapture(0);
   BufferDestroyed(0);
+
+  EXPECT_EQ(mock_video_capture_host_.released_buffer_count(), 0);

[chfremer, 2016/10/10 18:57:19]

I am confused by this test case. 
We simulate that the host creates a new buffer and hands it to the client
(VideoCaptureImpl instance), which in turn asks the host to stop capturing. The
host then tells the client that it has destroyed the buffer with id 0, even
though the client never told the host to release the buffer.

What am I missing here?
Does StopCapture() implicitly tell the host that the client is releasing all
buffers?

[mcasas, 2016/10/10 19:39:44]

VideoCaptureImpl's client has a callback to release
the received buffer (bound in [1]). In this code, the
bound method is &VideoCaptureImplTest::OnFrameReady,
which is mocked but does nothing, so there is no
"releasing" of the received buffer, hence l.277 has a 
Times(0) to indicate that we don't expect to receive
anything there.

Since I just wanted to migrate IPC-->Mojom with as
little changes as possible, I didn't touch this, but I
might have made it more evident :)

I'm totally in favour of extending all these unit
tests though, and will probably do. Stay tuned.



[1]
https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...

 }
 
 TEST_F(VideoCaptureImplTest, BufferReceivedAfterStop) {
   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_, Start(_, kSessionId, params_large_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_large_));
   EXPECT_CALL(mock_video_capture_host_, Stop(_));
+  EXPECT_CALL(mock_video_capture_host_, ReleaseBuffer(_, _, _, _));
 
   StartCapture(0, params_large_);
   NewBuffer(0, shm);
   StopCapture(0);
+  // A buffer received after StopCapture() triggers an instant ReleaseBuffer().
   BufferReceived(0, params_large_.requested_format.frame_size);
   BufferDestroyed(0);
 
-  EXPECT_EQ(this->video_capture_impl_->received_buffer_count(), 1);
+  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_STOPPED)).Times(2);
-  EXPECT_CALL(mock_video_capture_host_, Start(_, kSessionId, _))
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, _))
       .WillOnce(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_, Start(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
 
   StartCapture(0, params_small_);
 
-  video_capture_impl_->ReceiveStateChangeMessage(VIDEO_CAPTURE_STATE_ENDED);
+  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_, Start(_, kSessionId, params_small_));
+  EXPECT_CALL(mock_video_capture_host_, DoStart(_, kSessionId, params_small_));
 
   StartCapture(0, params_small_);
 
-  video_capture_impl_->ReceiveStateChangeMessage(VIDEO_CAPTURE_STATE_ERROR);
+  OnStateChanged(mojom::VideoCaptureState::FAILED);
 
   StopCapture(0);
 }
 
 }  // namespace content