[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..694922efe5985eac058b65fcc297531dfe5bad45 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"
@@ -45,6 +46,21 @@ static const int kInfiniteRatio = 99999;
   UMA_HISTOGRAM_SPARSE_SLOWLY(                          \
       name, (height) ? ((width)*100) / (height) : kInfiniteRatio);
 
+// This manual copy routine is needed because base::DictionaryValue does not

[miu, 2017/02/14 00:41:27]

Maybe it'd be better to fix the mojo impl? Meaning, the copy ctor generated by
mojo should handle dictionary values correctly, because we use those all over
the Chromium project. Not sure if this means fixing typemaps, or something
internal to the mojo code generator.

The danger for leaving this as-is, is that someone could add fields to
media::mojom::VideoFrameInfo, and not account for them here. You could, I
suppose, add a DCHECK that does an "equals" comparison between |frame_info| and
|result|.

[chfremer, 2017/02/14 21:15:16]

Excellent suggestion. I didn't fully realize how dangerous this copy routine
was.
I had a look at the Mojo code, and found that it should be relatively simple to 
support cloning DictionaryValue by adding a specialization for it to template 
CloneTraits. Added to next patch set and adding rockot@ to check if that is 
a valid approach.

+// allow the default copy operator.
+media::mojom::VideoFrameInfoPtr CloneFrameInfo(
+    media::mojom::VideoFrameInfoPtr* frame_info) {

[miu, 2017/02/14 00:41:27]

Seems that the type of this argument should be:

  const media::mojom::VideoFrameInfo& frame_info

[chfremer, 2017/02/14 21:15:16]

Done.

+  auto result = media::mojom::VideoFrameInfo::New();
+  result->timestamp = (*frame_info)->timestamp;
+  result->pixel_format = (*frame_info)->pixel_format;
+  result->storage_type = (*frame_info)->storage_type;
+  result->coded_size = (*frame_info)->coded_size;
+  result->visible_rect = (*frame_info)->visible_rect;
+  result->metadata = base::MakeUnique<base::DictionaryValue>();
+  result->metadata->MergeDictionary((*frame_info)->metadata.get());
+  return result;
+}
+
 }  // anonymous namespace
 
 struct VideoCaptureController::ControllerClient {
@@ -93,13 +109,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 +123,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 +137,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,8 +149,7 @@ 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;
   }
@@ -147,9 +159,13 @@ bool VideoCaptureController::BufferContext::HasZeroConsumerHoldCount() {
   return consumer_hold_count_ == 0;
 }
 
+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 +179,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,53 +366,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
+      // On the first use of a BufferContext on a client, call
+      // OnBufferCreated().
       // handles.
       auto known_buffers_entry_iter = std::find(
           std::begin(client->known_buffer_context_ids),
@@ -417,14 +412,19 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
         is_new_buffer = true;
       }
       if (is_new_buffer) {
-        mojo::ScopedSharedBufferHandle handle =
-            buffer.handle_provider()->GetHandleForInterProcessTransit();
+        size_t mapped_size =
+            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);
+
+      auto frame_info_copy = CloneFrameInfo(&frame_info);

[miu, 2017/02/14 00:41:27]

nit: You could just do:

  client->event_handler->OnBufferReady(
      client->controller_id, buffer_context_id,
      CloneFrameInfo(&frame_info));

[chfremer, 2017/02/14 21:15:16]

Done.

+      client->event_handler->OnBufferReady(
+          client->controller_id, buffer_context_id, std::move(frame_info_copy));
 
       auto buffers_in_use_entry_iter =
           std::find(std::begin(client->buffers_in_use),
@@ -433,21 +433,29 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
         client->buffers_in_use.push_back(buffer_context_id);
       else
         DCHECK(false) << "Unexpected duplicate buffer: " << buffer_context_id;
+
+      // Use if-check, because we are in a loop and only want to do this

[miu, 2017/02/14 00:41:27]

style nit: Don't use 2nd person in comments (i.e., "we" is not the focus).

[chfremer, 2017/02/14 21:15:16]

Done.

+      // move operation once.
+      if (buffer_read_permission) {
+        buffer_context_iter->set_read_permission(

[miu, 2017/02/14 00:41:27]

Perhaps this could be simplified by just calling set_read_permission() once
after the loop. Something like:

    for (...) {
      ...
      buffer_context_iter->IncreaseConsumerCount();
    }

    if (buffer_context_iter->HasConsumers()) {
      buffer_context_iter->set_read_permission(
          std::move(buffer_read_permission));
    }
  }

...or, if it's perfectly acceptable to always transfer the read permission (even
if it turns out there are no consumers), you could call set_read_permission()
unconditionally, either before or after the loop.

[chfremer, 2017/02/14 21:15:16]

Thanks. Moving the check to after the loop makes it much more clear and
eliminates 
the need for the comment trying to explain it :-).

+            std::move(buffer_read_permission));
+      }
       buffer_context_iter->IncreaseConsumerCount();
     }
   }
 
   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 +463,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 +494,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,