[Mojo Video Capture] Split OnIncomingCapturedVideoFrame() to OnNewBuffer() and OnFrameReadyInBuffer(

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 6d12e40bac04bd0b369c4a3a952206eb0b8efa07..4e44373e80d7d2dd5f9f0d689c4f6558f34a5aa7 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -19,6 +19,7 @@
 #include "components/display_compositor/gl_helper.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/media/video_capture_manager.h"
+#include "content/common/video_capture.mojom.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "media/base/video_frame.h"
@@ -93,13 +94,13 @@ VideoCaptureController::BufferContext::BufferContext(
     int buffer_context_id,
     int buffer_id,
     media::VideoFrameConsumerFeedbackObserver* consumer_feedback_observer,
-    media::FrameBufferPool* frame_buffer_pool)
+    mojo::ScopedSharedBufferHandle handle)
     : buffer_context_id_(buffer_context_id),
       buffer_id_(buffer_id),
       is_retired_(false),
       frame_feedback_id_(0),
       consumer_feedback_observer_(consumer_feedback_observer),
-      frame_buffer_pool_(frame_buffer_pool),
+      buffer_handle_(std::move(handle)),
       max_consumer_utilization_(
           media::VideoFrameConsumerFeedbackObserver::kNoUtilizationRecorded),
       consumer_hold_count_(0) {}
@@ -107,10 +108,10 @@ VideoCaptureController::BufferContext::BufferContext(
 VideoCaptureController::BufferContext::~BufferContext() = default;
 
 VideoCaptureController::BufferContext::BufferContext(
-    const VideoCaptureController::BufferContext& other) = default;
+    VideoCaptureController::BufferContext&& other) = default;
 
 VideoCaptureController::BufferContext& VideoCaptureController::BufferContext::
-operator=(const BufferContext& other) = default;
+operator=(BufferContext&& other) = default;
 
 void VideoCaptureController::BufferContext::RecordConsumerUtilization(
     double utilization) {
@@ -121,9 +122,6 @@ void VideoCaptureController::BufferContext::RecordConsumerUtilization(
 }
 
 void VideoCaptureController::BufferContext::IncreaseConsumerCount() {
-  if (consumer_hold_count_ == 0)
-    if (frame_buffer_pool_ != nullptr)
-      frame_buffer_pool_->SetBufferHold(buffer_id_);
   consumer_hold_count_++;
 }
 
@@ -136,20 +134,27 @@ void VideoCaptureController::BufferContext::DecreaseConsumerCount() {
       consumer_feedback_observer_->OnUtilizationReport(
           frame_feedback_id_, max_consumer_utilization_);
     }
-    if (frame_buffer_pool_ != nullptr)
-      frame_buffer_pool_->ReleaseBufferHold(buffer_id_);
+    buffer_read_permission_.reset();
     max_consumer_utilization_ =
         media::VideoFrameConsumerFeedbackObserver::kNoUtilizationRecorded;
   }
 }
 
-bool VideoCaptureController::BufferContext::HasZeroConsumerHoldCount() {
+bool VideoCaptureController::BufferContext::HasZeroConsumerHoldCount() const {
   return consumer_hold_count_ == 0;
 }
 
+bool VideoCaptureController::BufferContext::HasConsumers() const {
+  return consumer_hold_count_ > 0;
+}

[mcasas, 2017/02/16 22:35:56]

Agree with miu@, keep HasConsumers() and change 
the calls sites of HasZeroConsumerHoldCount()
with !HasConsumers()  (which seems quite understandable
to me). 

[chfremer, 2017/02/16 23:37:56]

Done.

+
+mojo::ScopedSharedBufferHandle
+VideoCaptureController::BufferContext::CreateHandleCopy() {
+  return buffer_handle_->Clone();
+}
+
 VideoCaptureController::VideoCaptureController()
-    : frame_buffer_pool_(nullptr),
-      consumer_feedback_observer_(nullptr),
+    : consumer_feedback_observer_(nullptr),
       state_(VIDEO_CAPTURE_STATE_STARTED),
       has_received_frames_(false),
       weak_ptr_factory_(this) {
@@ -163,15 +168,6 @@ VideoCaptureController::GetWeakPtrForIOThread() {
   return weak_ptr_factory_.GetWeakPtr();
 }
 
-void VideoCaptureController::SetFrameBufferPool(
-    std::unique_ptr<media::FrameBufferPool> frame_buffer_pool) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  frame_buffer_pool_ = std::move(frame_buffer_pool);
-  // Update existing BufferContext entries.
-  for (auto& entry : buffer_contexts_)
-    entry.set_frame_buffer_pool(frame_buffer_pool_.get());
-}
-
 void VideoCaptureController::SetConsumerFeedbackObserver(
     std::unique_ptr<media::VideoFrameConsumerFeedbackObserver>
         consumer_feedback_observer) {
@@ -359,54 +355,41 @@ const media::VideoCaptureFormat& VideoCaptureController::GetVideoCaptureFormat()
   return video_capture_format_;
 }
 
-void VideoCaptureController::OnIncomingCapturedVideoFrame(
-    media::VideoCaptureDevice::Client::Buffer buffer,
-    scoped_refptr<VideoFrame> frame) {
+void VideoCaptureController::OnNewBufferHandle(
+    int buffer_id,
+    std::unique_ptr<media::VideoCaptureDevice::Client::Buffer::HandleProvider>
+        handle_provider) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  const int buffer_id_from_producer = buffer.id();
-  DCHECK_NE(buffer_id_from_producer, media::VideoCaptureBufferPool::kInvalidId);
-  auto buffer_context_iter =
-      FindUnretiredBufferContextFromBufferId(buffer_id_from_producer);
-  if (buffer_context_iter == buffer_contexts_.end()) {
-    // A new buffer has been shared with us. Create new BufferContext.
-    buffer_contexts_.emplace_back(
-        next_buffer_context_id_++, buffer_id_from_producer,
-        consumer_feedback_observer_.get(), frame_buffer_pool_.get());
-    buffer_context_iter = buffer_contexts_.end() - 1;
-  }
-  buffer_context_iter->set_frame_feedback_id(buffer.frame_feedback_id());
+  DCHECK(FindUnretiredBufferContextFromBufferId(buffer_id) ==
+         buffer_contexts_.end());
+  buffer_contexts_.emplace_back(
+      next_buffer_context_id_++, buffer_id, consumer_feedback_observer_.get(),
+      handle_provider->GetHandleForInterProcessTransit());
+}
+
+void VideoCaptureController::OnFrameReadyInBuffer(
+    int buffer_id,
+    int frame_feedback_id,
+    std::unique_ptr<
+        media::VideoCaptureDevice::Client::Buffer::ScopedAccessPermission>
+        buffer_read_permission,
+    media::mojom::VideoFrameInfoPtr frame_info) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_NE(buffer_id, media::VideoCaptureBufferPool::kInvalidId);
+
+  auto buffer_context_iter = FindUnretiredBufferContextFromBufferId(buffer_id);
+  DCHECK(buffer_context_iter != buffer_contexts_.end());
+  buffer_context_iter->set_frame_feedback_id(frame_feedback_id);
   DCHECK(buffer_context_iter->HasZeroConsumerHoldCount());
 
   if (state_ == VIDEO_CAPTURE_STATE_STARTED) {
-    if (!frame->metadata()->HasKey(VideoFrameMetadata::FRAME_RATE)) {
-      frame->metadata()->SetDouble(VideoFrameMetadata::FRAME_RATE,
-                                   video_capture_format_.frame_rate);
-    }
-    std::unique_ptr<base::DictionaryValue> metadata =
-        frame->metadata()->CopyInternalValues();
-
-    // Only I420 and Y16 pixel formats are currently supported.
-    DCHECK(frame->format() == media::PIXEL_FORMAT_I420 ||
-           frame->format() == media::PIXEL_FORMAT_Y16)
-        << "Unsupported pixel format: "
-        << media::VideoPixelFormatToString(frame->format());
-
-    // Sanity-checks to confirm |frame| is actually being backed by |buffer|.
-    auto buffer_access =
-        buffer.handle_provider()->GetHandleForInProcessAccess();
-    DCHECK(frame->storage_type() == media::VideoFrame::STORAGE_SHMEM);
-    DCHECK(frame->data(media::VideoFrame::kYPlane) >= buffer_access->data() &&
-           (frame->data(media::VideoFrame::kYPlane) <
-            (buffer_access->data() + buffer_access->mapped_size())))
-        << "VideoFrame does not appear to be backed by Buffer";
-
     const int buffer_context_id = buffer_context_iter->buffer_context_id();
     for (const auto& client : controller_clients_) {
       if (client->session_closed || client->paused)
         continue;
 
-      // On the first use of a BufferContext on a client, share the memory
-      // handles.
+      // On the first use of a BufferContext for a particular client, call
+      // OnBufferCreated().
       auto known_buffers_entry_iter = std::find(
           std::begin(client->known_buffer_context_ids),
           std::end(client->known_buffer_context_ids), buffer_context_id);
@@ -417,14 +400,18 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
         is_new_buffer = true;
       }
       if (is_new_buffer) {

[mcasas, 2017/02/16 22:35:55]

Alternative for l. 393-401 (not your problem, but since you're
in the area):

      bool is_new_buffer = false;
      if (!base::ContainsValue(client->known_buffer_context_ids,
                               buffer_context_id)) {
        is_new_buffer = true;
        client->known_buffer_context_ids.push_back(buffer_context_id);
      }

And maybe you could even remove |is_new_buffer|
and do inside the if(!...) the new commands in l.403-411.

[chfremer, 2017/02/16 23:37:56]

Excellent suggestion. Thanks.
Done.

-        mojo::ScopedSharedBufferHandle handle =
-            buffer.handle_provider()->GetHandleForInterProcessTransit();
+        size_t mapped_size =

[mcasas, 2017/02/16 22:35:56]

const ?

[chfremer, 2017/02/16 23:37:56]

Done.

+            media::VideoCaptureFormat(frame_info->coded_size, 0.0f,
+                                      frame_info->pixel_format,
+                                      frame_info->storage_type)
+                .ImageAllocationSize();
         client->event_handler->OnBufferCreated(
-            client->controller_id, std::move(handle),
-            buffer_access->mapped_size(), buffer_context_id);
+            client->controller_id, buffer_context_iter->CreateHandleCopy(),
+            mapped_size, buffer_context_id);
       }
+
       client->event_handler->OnBufferReady(client->controller_id,
-                                           buffer_context_id, frame);
+                                           buffer_context_id, frame_info);
 
       auto buffers_in_use_entry_iter =
           std::find(std::begin(client->buffers_in_use),
@@ -433,21 +420,27 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
         client->buffers_in_use.push_back(buffer_context_id);
       else
         DCHECK(false) << "Unexpected duplicate buffer: " << buffer_context_id;

[mcasas, 2017/02/16 22:35:56]

What about l416-422 just:

  if (!base::ContainsValue(client->buffers_in_use, buffer_context_id))
    client->buffers_in_use.push_back(buffer_context_id);
  else 
    NOTREACHED() << "Unexpected duplicate buffer: " << buffer_context_id;

[chfremer, 2017/02/16 23:37:57]

Done.

+
       buffer_context_iter->IncreaseConsumerCount();
     }
+    if (buffer_context_iter->HasConsumers()) {
+      buffer_context_iter->set_read_permission(
+          std::move(buffer_read_permission));
+    }
   }
 
   if (!has_received_frames_) {
     UMA_HISTOGRAM_COUNTS("Media.VideoCapture.Width",
-                         frame->visible_rect().width());
+                         frame_info->coded_size.width());
     UMA_HISTOGRAM_COUNTS("Media.VideoCapture.Height",
-                         frame->visible_rect().height());
+                         frame_info->coded_size.height());
     UMA_HISTOGRAM_ASPECT_RATIO("Media.VideoCapture.AspectRatio",
-                               frame->visible_rect().width(),
-                               frame->visible_rect().height());
+                               frame_info->coded_size.width(),
+                               frame_info->coded_size.height());
     double frame_rate = 0.0f;
-    if (!frame->metadata()->GetDouble(VideoFrameMetadata::FRAME_RATE,
-                                      &frame_rate)) {
+    media::VideoFrameMetadata metadata;
+    metadata.MergeInternalValuesFrom(*frame_info->metadata);
+    if (!metadata.GetDouble(VideoFrameMetadata::FRAME_RATE, &frame_rate)) {
       frame_rate = video_capture_format_.frame_rate;
     }
     UMA_HISTOGRAM_COUNTS("Media.VideoCapture.FrameRate", frame_rate);
@@ -455,6 +448,21 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
   }
 }
 
+void VideoCaptureController::OnBufferRetired(int buffer_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  auto buffer_context_iter = FindUnretiredBufferContextFromBufferId(buffer_id);
+  DCHECK(buffer_context_iter != buffer_contexts_.end());
+
+  // If there are any clients still using the buffer, we need to allow them
+  // to finish up. We need to hold on to the BufferContext entry until then,
+  // because it contains the consumer hold.
+  if (buffer_context_iter->HasZeroConsumerHoldCount())
+    ReleaseBufferContext(buffer_context_iter);
+  else
+    buffer_context_iter->set_is_retired();
+}
+
 void VideoCaptureController::OnError() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   state_ = VIDEO_CAPTURE_STATE_ERROR;
@@ -471,21 +479,6 @@ void VideoCaptureController::OnLog(const std::string& message) {
   MediaStreamManager::SendMessageToNativeLog("Video capture: " + message);
 }
 
-void VideoCaptureController::OnBufferRetired(int buffer_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  auto buffer_context_iter = FindUnretiredBufferContextFromBufferId(buffer_id);
-  DCHECK(buffer_context_iter != buffer_contexts_.end());
-
-  // If there are any clients still using the buffer, we need to allow them
-  // to finish up. We need to hold on to the BufferContext entry until then,
-  // because it contains the consumer hold.
-  if (buffer_context_iter->HasZeroConsumerHoldCount())
-    ReleaseBufferContext(buffer_context_iter);
-  else
-    buffer_context_iter->set_is_retired();
-}
-
 VideoCaptureController::ControllerClient* VideoCaptureController::FindClient(
     VideoCaptureControllerID id,
     VideoCaptureControllerEventHandler* handler,