Remove media::VideoFrame from media::VideoCaptureDevice::Client interface

content/browser/renderer_host/media/video_capture_controller.cc

Index: content/browser/renderer_host/media/video_capture_controller.cc
diff --git a/content/browser/renderer_host/media/video_capture_controller.cc b/content/browser/renderer_host/media/video_capture_controller.cc
index 40e94bd7645b85793f39d5a6cf08fe3a48f4e1fe..8a1b56168e921ce4b3b87cf2992af23b97b877e5 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -1,5 +1,4 @@
 // 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 "content/browser/renderer_host/media/video_capture_controller.h"
@@ -8,7 +7,6 @@
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/media/video_capture_manager.h"
@@ -23,8 +21,27 @@
 
 namespace content {
 
+namespace {
+
 // The number of buffers that VideoCaptureBufferPool should allocate.
-static const int kNoOfBuffers = 3;
+const int kNoOfBuffers = 3;
+
+class PoolBuffer : public media::VideoCaptureDevice::Client::Buffer {
+ public:
+  PoolBuffer(const scoped_refptr<VideoCaptureBufferPool>& pool,
+             int buffer_id,
+             void* data,
+             size_t size)
+      : Buffer(data, size), pool_(pool), buffer_id_(buffer_id) {}
+
+ private:
+  virtual ~PoolBuffer() { pool_->RelinquishProducerReservation(buffer_id_); }
+
+  const scoped_refptr<VideoCaptureBufferPool> pool_;
+  const int buffer_id_;
+};
+
+}  // anonymous namespace
 
 struct VideoCaptureController::ControllerClient {
   ControllerClient(
@@ -85,7 +102,8 @@ class VideoCaptureController::VideoCaptureDeviceClient
   virtual ~VideoCaptureDeviceClient();
 
   // VideoCaptureDevice::Client implementation.
-  virtual scoped_refptr<media::VideoFrame> ReserveOutputBuffer(
+  virtual scoped_refptr<Buffer> ReserveOutputBuffer(
+      media::VideoFrame::Format format,
       const gfx::Size& size) OVERRIDE;
   virtual void OnIncomingCapturedFrame(const uint8* data,
                                        int length,
@@ -93,9 +111,10 @@ class VideoCaptureController::VideoCaptureDeviceClient
                                        int rotation,
                                        bool flip_vert,
                                        bool flip_horiz) OVERRIDE;
-  virtual void OnIncomingCapturedVideoFrame(
-      const scoped_refptr<media::VideoFrame>& frame,
-      base::Time timestamp) OVERRIDE;
+  virtual void OnIncomingCapturedBuffer(const scoped_refptr<Buffer>& buffer,
+                                        media::VideoFrame::Format format,
+                                        const gfx::Size& dimensions,
+                                        base::Time timestamp) OVERRIDE;
   virtual void OnError() OVERRIDE;
   virtual void OnFrameInfo(
       const media::VideoCaptureCapability& info) OVERRIDE;
@@ -103,9 +122,9 @@ class VideoCaptureController::VideoCaptureDeviceClient
       const media::VideoCaptureCapability& info) OVERRIDE;
 
  private:
-  scoped_refptr<media::VideoFrame> DoReserveI420VideoFrame(
-      const gfx::Size& size,
-      int rotation);
+  scoped_refptr<Buffer> DoReserveOutputBuffer(media::VideoFrame::Format format,
+                                              const gfx::Size& dimensions,
+                                              int rotation);
 
   // The controller to which we post events.
   const base::WeakPtr<VideoCaptureController> controller_;
@@ -113,6 +132,9 @@ class VideoCaptureController::VideoCaptureDeviceClient
   // The pool of shared-memory buffers used for capturing.
   const scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
 
+  // The set of buffers that have been used for rotated capturing.
+  std::set<int> rotated_buffers_;

[jiayl, 2013/11/08 22:16:46]

I don't find where this is used. Plus don't we need to store the degree of
rotation?

[sheu, 2013/11/09 00:59:30]

It's used in DoReserveOutputBuffers.  See the comment there.

+
   // Chopped pixels in width/height in case video capture device has odd
   // numbers for width/height.
   int chopped_width_;
@@ -238,10 +260,11 @@ void VideoCaptureController::ReturnBuffer(
   buffer_pool_->RelinquishConsumerHold(buffer_id, 1);
 }
 
-scoped_refptr<media::VideoFrame>
+scoped_refptr<media::VideoCaptureDevice::Client::Buffer>
 VideoCaptureController::VideoCaptureDeviceClient::ReserveOutputBuffer(
+    media::VideoFrame::Format format,
     const gfx::Size& size) {
-  return DoReserveI420VideoFrame(size, 0);
+  return DoReserveOutputBuffer(format, size, 0);
 }
 
 void VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedFrame(
@@ -256,16 +279,22 @@ void VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedFrame(
   if (!frame_info_.IsValid())
     return;
 
-  scoped_refptr<media::VideoFrame> dst = DoReserveI420VideoFrame(
-      gfx::Size(frame_info_.width, frame_info_.height), rotation);
+  const gfx::Size dimensions(frame_info_.width, frame_info_.height);
+  scoped_refptr<Buffer> buffer =
+      DoReserveOutputBuffer(media::VideoFrame::I420, dimensions, rotation);
 
-  if (!dst.get())
+  if (!buffer)
     return;
 #if !defined(AVOID_LIBYUV_FOR_ANDROID_WEBVIEW)
-
-  uint8* yplane = dst->data(media::VideoFrame::kYPlane);
-  uint8* uplane = dst->data(media::VideoFrame::kUPlane);
-  uint8* vplane = dst->data(media::VideoFrame::kVPlane);
+  uint8* yplane = reinterpret_cast<uint8*>(buffer->data());
+  uint8* uplane =
+      yplane +
+      media::VideoFrame::PlaneAllocationSize(
+          media::VideoFrame::I420, media::VideoFrame::kYPlane, dimensions);
+  uint8* vplane =
+      uplane +
+      media::VideoFrame::PlaneAllocationSize(
+          media::VideoFrame::I420, media::VideoFrame::kUPlane, dimensions);
   int yplane_stride = frame_info_.width;
   int uv_plane_stride = (frame_info_.width + 1) / 2;
   int crop_x = 0;
@@ -388,30 +417,37 @@ void VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedFrame(
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
-      base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
-                 controller_,
-                 dst,
-                 frame_info_.frame_rate,
-                 timestamp));
+      base::Bind(
+          &VideoCaptureController::DoIncomingCapturedI420BufferOnIOThread,
+          controller_,
+          buffer,
+          dimensions,
+          frame_info_.frame_rate,
+          timestamp));
 }
 
-void
-VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedVideoFrame(
-    const scoped_refptr<media::VideoFrame>& frame,
+void VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedBuffer(
+    const scoped_refptr<Buffer>& buffer,
+    media::VideoFrame::Format format,
+    const gfx::Size& dimensions,
     base::Time timestamp) {
+  // The capture pipeline expects I420 for now.
+  DCHECK_EQ(format, media::VideoFrame::I420)
+      << "Non-I420 output buffer returned";
+  DCHECK_NE(buffer_pool_->RecognizeReservedBuffer(buffer->data()),
+            VideoCaptureBufferPool::kInvalidId)
+      << "Non-reserved buffer returned";
 
-  // If this is a frame that belongs to the buffer pool, we can forward it
-  // directly to the IO thread and be done.
-  if (buffer_pool_->RecognizeReservedBuffer(
-      frame->shared_memory_handle()) >= 0) {
-    BrowserThread::PostTask(BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
-                   controller_, frame, frame_info_.frame_rate, timestamp));
-    return;
-  }
-
-  NOTREACHED() << "Frames should always belong to the buffer pool.";
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(
+          &VideoCaptureController::DoIncomingCapturedI420BufferOnIOThread,
+          controller_,
+          buffer,
+          dimensions,
+          frame_info_.frame_rate,
+          timestamp));
 }
 
 void VideoCaptureController::VideoCaptureDeviceClient::OnError() {
@@ -443,20 +479,52 @@ void VideoCaptureController::VideoCaptureDeviceClient::OnFrameInfoChanged(
   OnFrameInfo(info);
 }
 
-scoped_refptr<media::VideoFrame>
-VideoCaptureController::VideoCaptureDeviceClient::DoReserveI420VideoFrame(
-    const gfx::Size& size,
+scoped_refptr<media::VideoCaptureDevice::Client::Buffer>
+VideoCaptureController::VideoCaptureDeviceClient::DoReserveOutputBuffer(
+    media::VideoFrame::Format format,
+    const gfx::Size& dimensions,
     int rotation) {
+  // The capture pipeline expects I420 for now.
+  DCHECK_EQ(format, media::VideoFrame::I420)
+      << "Non-I420 output buffer requested";
+
   int buffer_id_to_drop = VideoCaptureBufferPool::kInvalidId;
-  scoped_refptr<media::VideoFrame> frame =
-      buffer_pool_->ReserveI420VideoFrame(size, rotation, &buffer_id_to_drop);
+  const size_t frame_bytes =
+      media::VideoFrame::AllocationSize(format, dimensions);
+
+  int buffer_id =
+      buffer_pool_->ReserveForProducer(frame_bytes, &buffer_id_to_drop);
+  if (buffer_id == VideoCaptureBufferPool::kInvalidId)
+    return NULL;
+  void* data;
+  size_t size;
+  buffer_pool_->GetBufferInfo(buffer_id, &data, &size);
+
+  scoped_refptr<media::VideoCaptureDevice::Client::Buffer> output_buffer(
+      new PoolBuffer(buffer_pool_, buffer_id, data, size));
+  buffer_id = VideoCaptureBufferPool::kInvalidId;

[ncarter (slow), 2013/11/07 02:25:28]

If you want to protect against misuse of buffer_id, it might be better to just
use scope for it (e.g. a helper function that does the wrapping) ... [continued]

[sheu, 2013/11/08 19:26:32]

I think I just went a little overboard with the ghetto scoping.  Removed.

+
   if (buffer_id_to_drop != VideoCaptureBufferPool::kInvalidId) {
     BrowserThread::PostTask(BrowserThread::IO,
         FROM_HERE,
         base::Bind(&VideoCaptureController::DoBufferDestroyedOnIOThread,
                    controller_, buffer_id_to_drop));
+    rotated_buffers_.erase(buffer_id_to_drop);
   }
-  return frame;
+
+  // If rotation is required, and the returned frame has not been rotated,
+  // perform a clear to clear the letterbox borders.
+  if ((rotation % 180) == 0) {

[jiayl, 2013/11/08 22:16:46]

why 180 not 360? Could you add comments to explain?

[sheu, 2013/11/09 00:59:30]

Updated comment.  Basically: a 180 degree rotation would not require
letterboxing (since the aspect ratio remains the same), and no clearing is
required.

+    rotated_buffers_.erase(buffer_id);

[ncarter (slow), 2013/11/07 02:25:28]

... [continued] to avoid the bugs like this (buffer_id is always kInvalidId
here).

[sheu, 2013/11/08 19:26:32]

/le sigh

Done.

+  } else {
+    if (!rotated_buffers_.count(buffer_id)) {

[ncarter (slow), 2013/11/07 02:25:28]

You can make this an .insert(); the return value will tell you whether or not it
was already present.

[sheu, 2013/11/08 19:26:32]

Done.

+      // TODO(jiayl): Generalize the |rotation| mechanism.

[jiayl, 2013/11/08 22:16:46]

What does it mean?

[sheu, 2013/11/09 00:59:30]

This block is copied out from the VideoCaptureBufferPool, so it means what it
meant in that context.  I didn't add it :-)

Do you have the context for this comment in the old location?

[ncarter (slow), 2013/11/09 01:10:34]

Just remove the TODO. You've mostly accomplished it here, though the context was
really about moving this same rotation logic up into the capture device impl
itself.

[sheu, 2013/11/11 22:37:44]

Done.

+      memset(output_buffer->data(), 0, output_buffer->size());
+      rotated_buffers_.insert(buffer_id);
+    }
+  }
+
+  return output_buffer;
 }
 
 VideoCaptureController::~VideoCaptureController() {
@@ -464,22 +532,25 @@ VideoCaptureController::~VideoCaptureController() {
                              controller_clients_.end());
 }
 
-void VideoCaptureController::DoIncomingCapturedFrameOnIOThread(
-    const scoped_refptr<media::VideoFrame>& reserved_frame,
+void VideoCaptureController::DoIncomingCapturedI420BufferOnIOThread(
+    scoped_refptr<media::VideoCaptureDevice::Client::Buffer> buffer,
+    const gfx::Size& dimensions,
     int frame_rate,
     base::Time timestamp) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  int buffer_id = buffer_pool_->RecognizeReservedBuffer(
-      reserved_frame->shared_memory_handle());
+  int buffer_id = buffer_pool_->RecognizeReservedBuffer(buffer->data());
   if (buffer_id < 0) {
     NOTREACHED();
     return;
   }
 
+  // |buffer_id| will remain valid as long as |buffer| remains valid, so
+  // it will be valid for the remainder of this function.
+
   media::VideoCaptureFormat frame_format(
-      reserved_frame->coded_size().width(),
-      reserved_frame->coded_size().height(),
+      dimensions.width(),
+      dimensions.height(),
       frame_rate,
       media::VariableResolutionVideoCaptureDevice);