[Mojo Video Capture] Retire buffers when Android Chromium goes to the background

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 9902d94dad4e4b80de214b7b342299a6720d697c..49bb11db87604e4c97d69cc8ae9060a88832e681 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -110,6 +110,9 @@ VideoCaptureController::BufferState::~BufferState() = default;
 VideoCaptureController::BufferState::BufferState(
     const VideoCaptureController::BufferState& other) = default;
 
+VideoCaptureController::BufferState& VideoCaptureController::BufferState::
+operator=(const BufferState& other) = default;
+
 void VideoCaptureController::BufferState::RecordConsumerUtilization(
     double utilization) {
   if (std::isfinite(utilization) && utilization >= 0.0) {
@@ -178,8 +181,8 @@ void VideoCaptureController::SetFrameBufferPool(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   frame_buffer_pool_ = std::move(frame_buffer_pool);
   // Update existing BufferState entries.
-  for (auto& entry : buffer_id_to_state_map_)
-    entry.second.SetFrameBufferPool(frame_buffer_pool_.get());
+  for (auto& entry : buffer_states_)
+    entry.SetFrameBufferPool(frame_buffer_pool_.get());
 }
 
 void VideoCaptureController::SetConsumerFeedbackObserver(
@@ -188,8 +191,8 @@ void VideoCaptureController::SetConsumerFeedbackObserver(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   consumer_feedback_observer_ = std::move(consumer_feedback_observer);
   // Update existing BufferState entries.
-  for (auto& entry : buffer_id_to_state_map_)
-    entry.second.SetConsumerFeedbackObserver(consumer_feedback_observer_.get());
+  for (auto& entry : buffer_states_)
+    entry.SetConsumerFeedbackObserver(consumer_feedback_observer_.get());
 }
 
 void VideoCaptureController::AddClient(
@@ -251,9 +254,11 @@ int VideoCaptureController::RemoveClient(
   if (!client)
     return kInvalidMediaCaptureSessionId;
 
-  // Take back all buffers held by the |client|.
-  for (const auto& buffer_id : client->buffers_in_use)
-    buffer_id_to_state_map_.at(buffer_id).DecreaseConsumerCount();
+  for (const auto& buffer_id : client->buffers_in_use) {
+    OnClientFinishedConsumingBuffer(
+        client, buffer_id,
+        media::VideoFrameConsumerFeedbackObserver::kNoUtilizationRecorded);
+  }
   client->buffers_in_use.clear();
 
   int session_id = client->session_id;
@@ -356,11 +361,10 @@ void VideoCaptureController::ReturnBuffer(
     NOTREACHED();
     return;
   }
-
-  BufferState& buffer_state = buffer_id_to_state_map_.at(buffer_id);
-  buffer_state.RecordConsumerUtilization(consumer_resource_utilization);
-  buffer_state.DecreaseConsumerCount();
   client->buffers_in_use.erase(buffers_in_use_entry_iter);
+
+  OnClientFinishedConsumingBuffer(client, buffer_id,
+                                  consumer_resource_utilization);
 }
 
 const media::VideoCaptureFormat&
@@ -375,17 +379,18 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   const int buffer_id = buffer.id();
   DCHECK_NE(buffer_id, media::VideoCaptureBufferPool::kInvalidId);
-
-  // Insert if not exists.
-  const auto it =
-      buffer_id_to_state_map_
-          .insert(std::make_pair(
-              buffer_id, BufferState(buffer_id, buffer.frame_feedback_id(),
-                                     consumer_feedback_observer_.get(),
-                                     frame_buffer_pool_.get())))
-          .first;
-  BufferState& buffer_state = it->second;
-  DCHECK(buffer_state.HasZeroConsumerHoldCount());
+  auto buffer_state_iter =
+      std::find_if(buffer_states_.begin(), buffer_states_.end(),
+                   [buffer_id](const BufferState& entry) {
+                     return entry.buffer_id() == buffer_id;
+                   });
+  if (buffer_state_iter == buffer_states_.end()) {
+    buffer_states_.emplace_back(buffer_id, buffer.frame_feedback_id(),
+                                consumer_feedback_observer_.get(),
+                                frame_buffer_pool_.get());
+    buffer_state_iter = buffer_states_.end() - 1;
+  }
+  DCHECK(buffer_state_iter->HasZeroConsumerHoldCount());
 
   if (state_ == VIDEO_CAPTURE_STATE_STARTED) {
     if (!frame->metadata()->HasKey(VideoFrameMetadata::FRAME_RATE)) {
@@ -440,7 +445,7 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
         client->buffers_in_use.push_back(buffer_id);
       else
         DCHECK(false) << "Unexpected duplicate buffer: " << buffer_id;
-      buffer_state.IncreaseConsumerCount();
+      buffer_state_iter->IncreaseConsumerCount();
     }
   }
 
@@ -468,7 +473,7 @@ void VideoCaptureController::OnError() {
 
   for (const auto& client : controller_clients_) {
     if (client->session_closed)
-       continue;
+      continue;
     client->event_handler->OnError(client->controller_id);
   }
 }
@@ -478,24 +483,24 @@ void VideoCaptureController::OnLog(const std::string& message) {
   MediaStreamManager::SendMessageToNativeLog("Video capture: " + message);
 }
 
-void VideoCaptureController::OnBufferDestroyed(int buffer_id_to_drop) {
+void VideoCaptureController::OnBufferRetired(int buffer_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  for (const auto& client : controller_clients_) {
-    if (client->session_closed)
-      continue;
-
-    auto known_buffers_entry_iter =
-        std::find(std::begin(client->known_buffers),
-                  std::end(client->known_buffers), buffer_id_to_drop);
-    if (known_buffers_entry_iter != std::end(client->known_buffers)) {
-      client->known_buffers.erase(known_buffers_entry_iter);
-      client->event_handler->OnBufferDestroyed(client->controller_id,
-                                               buffer_id_to_drop);
-    }
+  auto buffer_state_iter =
+      std::find_if(buffer_states_.begin(), buffer_states_.end(),
+                   [buffer_id](const BufferState& entry) {
+                     return entry.buffer_id() == buffer_id;
+                   });
+  DCHECK(buffer_state_iter != buffer_states_.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 BufferState entry until then,
+  // because it contains the consumer hold.
+  if (buffer_state_iter->HasZeroConsumerHoldCount()) {
+    ReleaseBufferState(buffer_state_iter);
+  } else {
+    buffer_state_iter->set_is_retired();
   }

[mcasas, 2017/01/20 22:41:32]

nit: no {} for one-line bodies, even for two one-line bodies.

[chfremer, 2017/01/25 23:45:20]

Done.

-
-  buffer_id_to_state_map_.erase(buffer_id_to_drop);
 }
 
 VideoCaptureController::ControllerClient* VideoCaptureController::FindClient(
@@ -519,4 +524,41 @@ VideoCaptureController::ControllerClient* VideoCaptureController::FindClient(
   return nullptr;
 }
 
+void VideoCaptureController::OnClientFinishedConsumingBuffer(
+    ControllerClient* client,
+    int buffer_id,
+    double consumer_resource_utilization) {
+  auto buffer_state_iter =
+      std::find_if(buffer_states_.begin(), buffer_states_.end(),
+                   [buffer_id](const BufferState& entry) {
+                     return entry.buffer_id() == buffer_id;
+                   });
+  DCHECK(buffer_state_iter != buffer_states_.end());

[mcasas, 2017/01/20 22:41:32]

These lines are == to l.489-494 (and also a bit like l.382-286)
plz factor them out somehow.

[chfremer, 2017/01/25 23:45:20]

Done.

+
+  buffer_state_iter->RecordConsumerUtilization(consumer_resource_utilization);
+  buffer_state_iter->DecreaseConsumerCount();
+  if (buffer_state_iter->HasZeroConsumerHoldCount() &&
+      buffer_state_iter->is_retired()) {
+    ReleaseBufferState(buffer_state_iter);
+  }
+}
+
+void VideoCaptureController::ReleaseBufferState(
+    const std::vector<BufferState>::iterator& buffer_state_iter) {
+  for (const auto& client : controller_clients_) {
+    if (client->session_closed)
+      continue;
+
+    auto known_buffers_entry_iter = std::find(std::begin(client->known_buffers),

[mcasas, 2017/01/20 22:41:32]

I wouldn't use the suffix _iter because it looks like
encoding the variable type in the name, which we 
don't like in Chromium; instead, just call it by what
it is, e.g.
s/known_buffers_entry_iter/known_buffer/

Here and elsewhere in this file.

[chfremer, 2017/01/25 23:45:20]

Hmm. I specifically chose to add the "_iter" suffix in order to call it by what
it is (or more precisely, what API a reader should expect it to expose). The
suffix in the name is needed to avoid confusing it with a reference (or copy) of
an actual entry of the container. Without the suffix, readers have to navigate
to the declaration in order to find out what it is, which is an unnecessary
burden on the reader.

[mcasas, 2017/01/31 01:39:39]

I disagree, variable names should have a semantical
meaning, not a syntactical, and the fact that a variable
is an iterator or a class or a reference should be taken
care by the definition of the operators etc.  That said, 
it seems like _iter in variables is somehow popular in
media/ and content/
(grep -rn "_iter =" media content | wc -l  ==> 125)
so I guess it's not too bad for temporary variables
(but not for constness etc).

+                                              std::end(client->known_buffers),
+                                              buffer_state_iter->buffer_id());
+    if (known_buffers_entry_iter != std::end(client->known_buffers)) {
+      client->known_buffers.erase(known_buffers_entry_iter);
+      client->event_handler->OnBufferDestroyed(client->controller_id,
+                                               buffer_state_iter->buffer_id());
+    }
+  }
+  buffer_states_.erase(buffer_state_iter);
+}

[mcasas, 2017/01/20 22:41:32]

More testing of all this dancing of buffers...?

[chfremer, 2017/01/25 23:45:20]

Yes. Will do.

+
 }  // namespace content