[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

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 83f264cd0ec5687db042e9ec5300c65f62bcb45b..2bb1b0cc03df8d557297d468ad8c2c3350ed641e 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -46,40 +46,6 @@ static const int kInfiniteRatio = 99999;
         name, \
         (height) ? ((width) * 100) / (height) : kInfiniteRatio);
 
-class SyncTokenClientImpl : public VideoFrame::SyncTokenClient {
- public:
-  explicit SyncTokenClientImpl(display_compositor::GLHelper* gl_helper)
-      : gl_helper_(gl_helper) {}
-  ~SyncTokenClientImpl() override {}
-  void GenerateSyncToken(gpu::SyncToken* sync_token) override {
-    gl_helper_->GenerateSyncToken(sync_token);
-  }
-  void WaitSyncToken(const gpu::SyncToken& sync_token) override {
-    gl_helper_->WaitSyncToken(sync_token);
-  }
-
- private:
-  display_compositor::GLHelper* gl_helper_;
-};
-
-void ReturnVideoFrame(const scoped_refptr<VideoFrame>& video_frame,
-                      const gpu::SyncToken& sync_token) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-#if defined(OS_ANDROID)
-  NOTREACHED();
-#else
-  display_compositor::GLHelper* gl_helper =
-      ImageTransportFactory::GetInstance()->GetGLHelper();
-  // UpdateReleaseSyncToken() creates a new sync_token using |gl_helper|, so
-  // wait the given |sync_token| using |gl_helper|.
-  if (gl_helper) {
-    gl_helper->WaitSyncToken(sync_token);
-    SyncTokenClientImpl client(gl_helper);
-    video_frame->UpdateReleaseSyncToken(&client);
-  }
-#endif
-}
-
 std::unique_ptr<media::VideoCaptureJpegDecoder> CreateGpuJpegDecoder(
     const media::VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) {
   return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb);
@@ -150,10 +116,8 @@ struct VideoCaptureController::ControllerClient {
   // Buffers that are currently known to this client.
   std::set<int> known_buffers;
 
-  // Buffers currently held by this client, and sync token callback to call when
-  // they are returned from the client.
-  typedef std::map<int, scoped_refptr<VideoFrame>> ActiveBufferMap;
-  ActiveBufferMap active_buffers;
+  // Buffers currently held by this client.
+  std::set<int> buffers_in_use;
 
   // State of capture session, controlled by VideoCaptureManager directly. This
   // transitions to true as soon as StopSession() occurs, at which point the
@@ -172,10 +136,71 @@ struct VideoCaptureController::ControllerClient {
   bool paused;
 };
 
+VideoCaptureController::BufferState::BufferState()
+    : buffer_id_(0),
+      frame_receiver_observer_(nullptr),
+      max_consumer_utilization_(0),
+      consumer_hold_count_(0) {}
+
+VideoCaptureController::BufferState::BufferState(
+    int buffer_id,
+    media::FrameReceiverObserver* frame_receiver_observer,
+    const scoped_refptr<media::VideoCaptureBufferPool>& buffer_pool,
+    const scoped_refptr<media::VideoFrame>& frame)
+    : buffer_id_(buffer_id),
+      frame_receiver_observer_(frame_receiver_observer),
+      buffer_pool_(buffer_pool),
+      frame_(frame),
+      max_consumer_utilization_(
+          media::FrameReceiverObserver::no_utilization_recorded()),
+      consumer_hold_count_(0) {}
+
+VideoCaptureController::BufferState::BufferState(
+    VideoCaptureController::BufferState&& other) = default;
+
+VideoCaptureController::BufferState::~BufferState() = default;
+
+VideoCaptureController::BufferState& VideoCaptureController::BufferState::
+operator=(VideoCaptureController::BufferState&& other) = default;
+
+void VideoCaptureController::BufferState::RecordConsumerUtilization(
+    double utilization) {
+  if (std::isfinite(utilization) && utilization >= 0.0) {
+    max_consumer_utilization_ =
+        std::max(max_consumer_utilization_, utilization);
+  }
+}
+
+void VideoCaptureController::BufferState::IncreaseConsumerCount() {
+  if (consumer_hold_count_ == 0)
+    buffer_pool_->HoldForConsumers(buffer_id_, 1);

[miu, 2016/12/01 05:25:17]

Looks like the "hold count" tracking isn't needed in the buffer pool anymore.
Future cleanup?

[chfremer, 2016/12/02 01:28:28]

Excellent observation. I put it on the list of future cleanups in the design
doc.
Actually, with one of the follow-up CLs we may even no longer need to
differentiate between consumer hold and producer hold (but I am getting ahead of
myself here). We may be able to do apply both of those simplifications to the
buffer pool in one go.

+  consumer_hold_count_++;
+}
+
+void VideoCaptureController::BufferState::DecreaseConsumerCount() {
+  consumer_hold_count_--;
+  if (consumer_hold_count_ == 0) {
+    if (frame_receiver_observer_ != nullptr &&
+        max_consumer_utilization_ !=
+            media::FrameReceiverObserver::no_utilization_recorded()) {
+      frame_receiver_observer_->OnReceiverReportingUtilization(
+          buffer_id_, max_consumer_utilization_);
+    }
+    buffer_pool_->RelinquishConsumerHold(buffer_id_, 1);
+    max_consumer_utilization_ =
+        media::FrameReceiverObserver::no_utilization_recorded();
+  }
+}
+
+bool VideoCaptureController::BufferState::HasZeroConsumerHoldCount() {
+  return consumer_hold_count_ == 0;
+}
+
 VideoCaptureController::VideoCaptureController(int max_buffers)
     : buffer_pool_(new media::VideoCaptureBufferPoolImpl(
           base::MakeUnique<media::VideoCaptureBufferTrackerFactoryImpl>(),
           max_buffers)),
+      frame_receiver_observer_(nullptr),
       state_(VIDEO_CAPTURE_STATE_STARTED),
       has_received_frames_(false),
       weak_ptr_factory_(this) {
@@ -187,7 +212,13 @@ VideoCaptureController::GetWeakPtrForIOThread() {
   return weak_ptr_factory_.GetWeakPtr();
 }
 
-std::unique_ptr<media::VideoCaptureDevice::Client>
+void VideoCaptureController::SetFrameReceiverObserver(
+    media::FrameReceiverObserver* frame_receiver_observer) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  frame_receiver_observer_ = frame_receiver_observer;
+}
+
+std::unique_ptr<media::VideoCaptureDeviceClient>
 VideoCaptureController::NewDeviceClient() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   return base::MakeUnique<media::VideoCaptureDeviceClient>(
@@ -196,7 +227,8 @@ VideoCaptureController::NewDeviceClient() {
       buffer_pool_,
       base::Bind(&CreateGpuJpegDecoder,
                  base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame,
-                            this->GetWeakPtrForIOThread())));
+                            this->GetWeakPtrForIOThread())),
+      base::ThreadTaskRunnerHandle::Get());
 }
 
 void VideoCaptureController::AddClient(
@@ -259,9 +291,9 @@ int VideoCaptureController::RemoveClient(
     return kInvalidMediaCaptureSessionId;
 
   // Take back all buffers held by the |client|.
-  for (const auto& buffer : client->active_buffers)
-    buffer_pool_->RelinquishConsumerHold(buffer.first, 1);
-  client->active_buffers.clear();
+  for (const auto& buffer_id : client->buffers_in_use)
+    buffer_id_to_state_map_[buffer_id].DecreaseConsumerCount();
+  client->buffers_in_use.clear();
 
   int session_id = client->session_id;
   controller_clients_.remove_if(
@@ -353,42 +385,16 @@ void VideoCaptureController::ReturnBuffer(
 
   // If this buffer is not held by this client, or this client doesn't exist
   // in controller, do nothing.
-  ControllerClient::ActiveBufferMap::iterator iter;
-  if (!client || (iter = client->active_buffers.find(buffer_id)) ==
-                     client->active_buffers.end()) {
+  if (!client || (client->buffers_in_use.find(buffer_id) ==
+                  client->buffers_in_use.end())) {
     NOTREACHED();
     return;
   }
 
-  // Set the RESOURCE_UTILIZATION to the maximum of those provided by each
-  // consumer (via separate calls to this method that refer to the same
-  // VideoFrame).  The producer of this VideoFrame may check this value, after
-  // all consumer holds are relinquished, to make quality versus performance
-  // trade-off decisions.
-  scoped_refptr<VideoFrame> frame = iter->second;
-  if (std::isfinite(consumer_resource_utilization) &&
-      consumer_resource_utilization >= 0.0) {
-    double resource_utilization = -1.0;
-    if (frame->metadata()->GetDouble(VideoFrameMetadata::RESOURCE_UTILIZATION,
-                                     &resource_utilization)) {
-      frame->metadata()->SetDouble(VideoFrameMetadata::RESOURCE_UTILIZATION,
-                                   std::max(consumer_resource_utilization,
-                                            resource_utilization));
-    } else {
-      frame->metadata()->SetDouble(VideoFrameMetadata::RESOURCE_UTILIZATION,
-                                   consumer_resource_utilization);
-    }
-  }
-
-  client->active_buffers.erase(iter);
-  buffer_pool_->RelinquishConsumerHold(buffer_id, 1);
-
-#if defined(OS_ANDROID)
-  DCHECK(!sync_token.HasData());
-#endif
-  if (sync_token.HasData())
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(&ReturnVideoFrame, frame, sync_token));
+  BufferState& buffer_state = buffer_id_to_state_map_[buffer_id];
+  buffer_state.RecordConsumerUtilization(consumer_resource_utilization);
+  buffer_state.DecreaseConsumerCount();
+  client->buffers_in_use.erase(buffer_id);
 }
 
 const media::VideoCaptureFormat&
@@ -407,7 +413,15 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
   const int buffer_id = buffer->id();
   DCHECK_NE(buffer_id, media::VideoCaptureBufferPool::kInvalidId);
 
-  int count = 0;
+  if (buffer_id_to_state_map_.find(buffer_id) ==

[miu, 2016/12/01 05:25:17]

As mentioned in the header file, you might be able to eliminate the zero-arg
ctor and 3 map look-ups here by using iterators:

  // Insert if not exists.
  const auto it = buffer_id_to_state_map_.insert(
      std::make_pair(buffer_id, BufferState(...))).first;
  BufferState& buffer_state = it->second;

[chfremer, 2016/12/02 01:28:28]

Very nice solution. Thanks.
Unfortunately, in order to make the invariant members of BufferState const, we
now have to make a copy, but I think it is worth it. If only our compiler would
support std::map::emplace...

+         buffer_id_to_state_map_.end()) {
+    BufferState new_buffer_state(buffer_id, frame_receiver_observer_,
+                                 buffer_pool_, frame);
+    buffer_id_to_state_map_[buffer_id] = std::move(new_buffer_state);
+  }
+  BufferState& buffer_state = buffer_id_to_state_map_[buffer_id];
+  DCHECK(buffer_state.HasZeroConsumerHoldCount());
+
   if (state_ == VIDEO_CAPTURE_STATE_STARTED) {
     if (!frame->metadata()->HasKey(VideoFrameMetadata::FRAME_RATE)) {
       frame->metadata()->SetDouble(VideoFrameMetadata::FRAME_RATE,
@@ -442,11 +456,9 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
 
       client->event_handler->OnBufferReady(client->controller_id, buffer_id,
                                            frame);
-      const bool inserted =
-          client->active_buffers.insert(std::make_pair(buffer_id, frame))
-              .second;
+      const bool inserted = client->buffers_in_use.insert(buffer_id).second;
       DCHECK(inserted) << "Unexpected duplicate buffer: " << buffer_id;
-      count++;
+      buffer_state.IncreaseConsumerCount();
     }
   }
 
@@ -466,8 +478,6 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
     UMA_HISTOGRAM_COUNTS("Media.VideoCapture.FrameRate", frame_rate);
     has_received_frames_ = true;
   }
-
-  buffer_pool_->HoldForConsumers(buffer_id, count);
 }
 
 void VideoCaptureController::OnError() {
@@ -498,6 +508,8 @@ void VideoCaptureController::OnBufferDestroyed(int buffer_id_to_drop) {
                                                buffer_id_to_drop);
     }
   }
+
+  buffer_id_to_state_map_.erase(buffer_id_to_drop);
 }
 
 void VideoCaptureController::DoNewBufferOnIOThread(