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

content/browser/renderer_host/media/web_contents_video_capture_device.cc

Index: content/browser/renderer_host/media/web_contents_video_capture_device.cc
diff --git a/content/browser/renderer_host/media/web_contents_video_capture_device.cc b/content/browser/renderer_host/media/web_contents_video_capture_device.cc
index d8a7ac7fcf90244e049456159ceeb11140e1e2d3..8dab1c163f94befbaec8a7807a6645bd4f3502a2 100644
--- a/content/browser/renderer_host/media/web_contents_video_capture_device.cc
+++ b/content/browser/renderer_host/media/web_contents_video_capture_device.cc
@@ -163,10 +163,11 @@ class ThreadSafeCaptureOracle
   virtual ~ThreadSafeCaptureOracle() {}
 
   // Callback invoked on completion of all captures.
-  void DidCaptureFrame(const scoped_refptr<media::VideoFrame>& frame,
-                       int frame_number,
-                       base::Time timestamp,
-                       bool success);
+  void DidCaptureFrame(
+      scoped_ptr<media::VideoCaptureDevice::Client::Buffer> buffer,
+      int frame_number,
+      base::Time timestamp,
+      bool success);
   // Protects everything below it.
   base::Lock lock_;
 
@@ -417,8 +418,8 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
   if (!client_)
     return false;  // Capture is stopped.
 
-  scoped_refptr<media::VideoFrame> output_buffer =
-      client_->ReserveOutputBuffer(capture_size_);
+  scoped_ptr<media::VideoCaptureDevice::Client::Buffer> output_buffer =
+      client_->ReserveOutputBuffer(media::VideoFrame::I420, capture_size_);
   const bool should_capture =
       oracle_->ObserveEventAndDecideCapture(event, event_time);
   const bool content_is_dirty =
@@ -430,14 +431,14 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
        "paint"));
 
   // Consider the various reasons not to initiate a capture.
-  if (should_capture && !output_buffer.get()) {
+  if (should_capture && !output_buffer) {
     TRACE_EVENT_INSTANT1("mirroring",
                          "EncodeLimited",
                          TRACE_EVENT_SCOPE_THREAD,
                          "trigger",
                          event_name);
     return false;
-  } else if (!should_capture && output_buffer.get()) {
+  } else if (!should_capture && output_buffer) {
     if (content_is_dirty) {
       // This is a normal and acceptable way to drop a frame. We've hit our
       // capture rate limit: for example, the content is animating at 60fps but
@@ -447,7 +448,7 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
                            "trigger", event_name);
     }
     return false;
-  } else if (!should_capture && !output_buffer.get()) {
+  } else if (!should_capture && !output_buffer) {
     // We decided not to capture, but we wouldn't have been able to if we wanted
     // to because no output buffer was available.
     TRACE_EVENT_INSTANT1("mirroring", "NearlyEncodeLimited",
@@ -459,9 +460,21 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
   TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get(),
                            "frame_number", frame_number,
                            "trigger", event_name);
-  *storage = output_buffer;
+
+  *storage = media::VideoFrame::WrapExternalPackedMemory(
+      media::VideoFrame::I420,
+      capture_size_,
+      gfx::Rect(capture_size_),
+      capture_size_,
+      static_cast<uint8*>(output_buffer->data()),
+      output_buffer->size(),
+      base::SharedMemory::NULLHandle(),
+      base::TimeDelta(),
+      base::Closure());
   *callback = base::Bind(&ThreadSafeCaptureOracle::DidCaptureFrame,
-                         this, output_buffer, frame_number);
+                         this,
+                         base::Passed(&output_buffer),

[ncarter (slow), 2013/10/29 18:42:17]

I think this ownership scheme will work today, but it's also possibly fragile,
because we're relying on a fairly indirect invariant: |callback| must outlive
all uses of the memory in |storage|. I don't think that's violated today, but I
wonder if it might be safer to do something like this:
 - scoped_ptr<output_buffer> is passed into a cleanup closure of the VideoFrame.
 - the delivery callback (DidCaptureFrame) captures a reference to the
VideoFrame object (guaranteeing its lifetime), and also takes a
base::Unretained() output_buffer?

[sheu, 2013/11/05 20:02:10]

That actually blew this whole thing wide open.  Long story short, I've made the
Buffer refcounted.  :-(

Problem is, the external user of the Buffer (in our case here, a
media::VideoFrame) expects to hold onto the Buffer past when it returns the
buffer back to the VideoCaptureController::Client.  So it needs ownership.

At the same time, passing it back to the VCC::Client, that also expects to be
able to maintain lifetime for the Buffer object while it's passing the Buffer
around to send to the renderer process.  So it needs ownership too.

+                         frame_number);
   return true;
 }
 
@@ -477,12 +490,12 @@ void ThreadSafeCaptureOracle::ReportError() {
 }
 
 void ThreadSafeCaptureOracle::DidCaptureFrame(
-    const scoped_refptr<media::VideoFrame>& frame,
+    scoped_ptr<media::VideoCaptureDevice::Client::Buffer> buffer,
     int frame_number,
     base::Time timestamp,
     bool success) {
   base::AutoLock guard(lock_);
-  TRACE_EVENT_ASYNC_END2("mirroring", "Capture", frame.get(),
+  TRACE_EVENT_ASYNC_END2("mirroring", "Capture", buffer.get(),
                          "success", success,
                          "timestamp", timestamp.ToInternalValue());
 
@@ -490,8 +503,10 @@ void ThreadSafeCaptureOracle::DidCaptureFrame(
     return;  // Capture is stopped.
 
   if (success) {
-    if (oracle_->CompleteCapture(frame_number, timestamp))
-      client_->OnIncomingCapturedVideoFrame(frame, timestamp);
+    if (oracle_->CompleteCapture(frame_number, timestamp)) {
+      client_->OnIncomingCapturedBuffer(
+          buffer.Pass(), media::VideoFrame::I420, capture_size_, timestamp);
+    }
   }
 }