From Video Capture, abolish OnFrameInfo and enable resolution changes

content/browser/renderer_host/media/video_capture_buffer_pool.cc

Index: content/browser/renderer_host/media/video_capture_buffer_pool.cc
diff --git a/content/browser/renderer_host/media/video_capture_buffer_pool.cc b/content/browser/renderer_host/media/video_capture_buffer_pool.cc
index 3e30834feb2ca8a17788ddd94cf3b0ade2147eb9..ed35e0f74c02e47031ef7aeae113f678aec16b83 100644
--- a/content/browser/renderer_host/media/video_capture_buffer_pool.cc
+++ b/content/browser/renderer_host/media/video_capture_buffer_pool.cc
@@ -7,71 +7,53 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "media/base/video_frame.h"
 #include "media/base/video_util.h"
 
 namespace content {
 
-VideoCaptureBufferPool::VideoCaptureBufferPool(size_t size, int count)
-    : size_(size),
-      count_(count) {
-}
+const int VideoCaptureBufferPool::kInvalidId = -1;
 
-VideoCaptureBufferPool::~VideoCaptureBufferPool() {
+VideoCaptureBufferPool::VideoCaptureBufferPool(int count)
+    : count_(count),
+      next_buffer_id_(0) {
 }
 
-bool VideoCaptureBufferPool::Allocate() {
-  base::AutoLock lock(lock_);
-  DCHECK(!IsAllocated());
-  buffers_.resize(count_);
-  for (int buffer_id = 0; buffer_id < count(); ++buffer_id) {
-    Buffer* buffer = new Buffer();
-    buffers_[buffer_id] = buffer;
-    if (!buffer->shared_memory.CreateAndMapAnonymous(GetMemorySize()))
-      return false;
-  }
-  return true;
+VideoCaptureBufferPool::~VideoCaptureBufferPool() {
+  STLDeleteValues(&buffers_);
 }
 
 base::SharedMemoryHandle VideoCaptureBufferPool::ShareToProcess(
     int buffer_id,
-    base::ProcessHandle process_handle) {
+    base::ProcessHandle process_handle,
+    size_t* memory_size) {
   base::AutoLock lock(lock_);
-  DCHECK(IsAllocated());
-  DCHECK(buffer_id >= 0);
-  DCHECK(buffer_id < count_);
-  Buffer* buffer = buffers_[buffer_id];
+
+  Buffer* buffer = GetBuffer(buffer_id);
+  if (!buffer) {
+    NOTREACHED() << "Invalid buffer_id.";
+    return base::SharedMemory::NULLHandle();
+  }
   base::SharedMemoryHandle remote_handle;
   buffer->shared_memory.ShareToProcess(process_handle, &remote_handle);
+  *memory_size = buffer->shared_memory.requested_size();
   return remote_handle;
 }
 
-base::SharedMemoryHandle VideoCaptureBufferPool::GetHandle(int buffer_id) {
+int VideoCaptureBufferPool::ReserveForProducer(size_t size,
+                                               int* buffer_id_to_drop) {
   base::AutoLock lock(lock_);
-  DCHECK(IsAllocated());
-  DCHECK(buffer_id >= 0);
-  DCHECK(buffer_id < count_);
-  return buffers_[buffer_id]->shared_memory.handle();
-}
-
-void* VideoCaptureBufferPool::GetMemory(int buffer_id) {
-  base::AutoLock lock(lock_);
-  DCHECK(IsAllocated());
-  DCHECK(buffer_id >= 0);
-  DCHECK(buffer_id < count_);
-  return buffers_[buffer_id]->shared_memory.memory();
-}
-
-int VideoCaptureBufferPool::ReserveForProducer() {
-  base::AutoLock lock(lock_);
-  return ReserveForProducerInternal();
+  return ReserveForProducerInternal(size, buffer_id_to_drop);
 }
 
 void VideoCaptureBufferPool::RelinquishProducerReservation(int buffer_id) {
   base::AutoLock lock(lock_);
-  DCHECK(buffer_id >= 0);
-  DCHECK(buffer_id < count());
-  Buffer* buffer = buffers_[buffer_id];
+  Buffer* buffer = GetBuffer(buffer_id);
+  if (!buffer) {
+    NOTREACHED() << "Invalid buffer_id.";
+    return;
+  }
   DCHECK(buffer->held_by_producer);
   buffer->held_by_producer = false;
 }
@@ -80,65 +62,56 @@ void VideoCaptureBufferPool::HoldForConsumers(
     int buffer_id,
     int num_clients) {
   base::AutoLock lock(lock_);
-  DCHECK(buffer_id >= 0);
-  DCHECK(buffer_id < count());
-  DCHECK(IsAllocated());
-  Buffer* buffer = buffers_[buffer_id];
+  Buffer* buffer = GetBuffer(buffer_id);
+  if (!buffer) {
+    NOTREACHED() << "Invalid buffer_id.";
+    return;
+  }
   DCHECK(buffer->held_by_producer);
   DCHECK(!buffer->consumer_hold_count);
 
   buffer->consumer_hold_count = num_clients;
   // Note: |held_by_producer| will stay true until
   // RelinquishProducerReservation() (usually called by destructor of the object
-  // wrapping this buffer, e.g. a media::VideoFrame
+  // wrapping this buffer, e.g. a media::VideoFrame).
 }
 
 void VideoCaptureBufferPool::RelinquishConsumerHold(int buffer_id,
                                                     int num_clients) {
   base::AutoLock lock(lock_);
-  DCHECK(buffer_id >= 0);
-  DCHECK(buffer_id < count());
-  DCHECK_GT(num_clients, 0);
-  DCHECK(IsAllocated());
-  Buffer* buffer = buffers_[buffer_id];
+  Buffer* buffer = GetBuffer(buffer_id);
+  if (!buffer) {
+    NOTREACHED() << "Invalid buffer_id.";
+    return;
+  }
   DCHECK_GE(buffer->consumer_hold_count, num_clients);
 
   buffer->consumer_hold_count -= num_clients;
 }
 
-// State query functions.
-size_t VideoCaptureBufferPool::GetMemorySize() const {
-  // No need to take |lock_| currently.
-  return size_;
-}
-
 int VideoCaptureBufferPool::RecognizeReservedBuffer(
     base::SharedMemoryHandle maybe_belongs_to_pool) {
   base::AutoLock lock(lock_);
-  for (int buffer_id = 0; buffer_id < count(); ++buffer_id) {
-    Buffer* buffer = buffers_[buffer_id];
-    if (buffer->shared_memory.handle() == maybe_belongs_to_pool) {
-      DCHECK(buffer->held_by_producer);
-      return buffer_id;
+  for (BufferMap::iterator it = buffers_.begin(); it != buffers_.end(); it++) {
+    if (it->second->shared_memory.handle() == maybe_belongs_to_pool) {
+      DCHECK(it->second->held_by_producer);
+      return it->first;
     }
   }
-  return -1;  // Buffer is not from our pool.
+  return kInvalidId;  // Buffer is not from our pool.
 }
 
 scoped_refptr<media::VideoFrame> VideoCaptureBufferPool::ReserveI420VideoFrame(
     const gfx::Size& size,
-    int rotation) {
-  if (GetMemorySize() !=
-      media::VideoFrame::AllocationSize(media::VideoFrame::I420, size)) {
-    DCHECK_EQ(GetMemorySize(),
-              media::VideoFrame::AllocationSize(media::VideoFrame::I420, size));
-    return NULL;
-  }
-
+    int rotation,
+    int* buffer_id_to_drop) {
   base::AutoLock lock(lock_);
 
-  int buffer_id = ReserveForProducerInternal();
-  if (buffer_id < 0)
+  size_t frame_bytes =
+      media::VideoFrame::AllocationSize(media::VideoFrame::I420, size);
+
+  int buffer_id = ReserveForProducerInternal(frame_bytes, buffer_id_to_drop);
+  if (buffer_id == kInvalidId)
     return NULL;
 
   base::Closure disposal_handler = base::Bind(
@@ -146,7 +119,7 @@ scoped_refptr<media::VideoFrame> VideoCaptureBufferPool::ReserveI420VideoFrame(
       this,
       buffer_id);
 
-  Buffer* buffer = buffers_[buffer_id];
+  Buffer* buffer = GetBuffer(buffer_id);
   // Wrap the buffer in a VideoFrame container.
   scoped_refptr<media::VideoFrame> frame =
       media::VideoFrame::WrapExternalSharedMemory(
@@ -155,13 +128,13 @@ scoped_refptr<media::VideoFrame> VideoCaptureBufferPool::ReserveI420VideoFrame(
           gfx::Rect(size),
           size,
           static_cast<uint8*>(buffer->shared_memory.memory()),
-          GetMemorySize(),
+          frame_bytes,
           buffer->shared_memory.handle(),
           base::TimeDelta(),
           disposal_handler);
 
   if (buffer->rotation != rotation) {
-    // TODO(nick): Generalize the |rotation| mechanism.
+    // TODO(jiayl): Generalize the |rotation| mechanism.
     media::FillYUV(frame.get(), 0, 128, 128);
     buffer->rotation = rotation;
   }
@@ -169,45 +142,62 @@ scoped_refptr<media::VideoFrame> VideoCaptureBufferPool::ReserveI420VideoFrame(
   return frame;
 }
 
-bool VideoCaptureBufferPool::IsAnyBufferHeldForConsumers() {
-  base::AutoLock lock(lock_);
-  for (int buffer_id = 0; buffer_id < count(); ++buffer_id) {
-    Buffer* buffer = buffers_[buffer_id];
-    if (buffer->consumer_hold_count > 0)
-      return true;
-  }
-  return false;
-}
-
 VideoCaptureBufferPool::Buffer::Buffer()
     : rotation(0),
       held_by_producer(false),
       consumer_hold_count(0) {}
 
-int VideoCaptureBufferPool::ReserveForProducerInternal() {
+int VideoCaptureBufferPool::ReserveForProducerInternal(size_t size,
+                                                       int* buffer_id_to_drop) {
   lock_.AssertAcquired();
-  DCHECK(IsAllocated());
 
-  int buffer_id = -1;
-  for (int candidate_id = 0; candidate_id < count(); ++candidate_id) {
-    Buffer* candidate = buffers_[candidate_id];
-    if (!candidate->consumer_hold_count && !candidate->held_by_producer) {
-      buffer_id = candidate_id;
+  // Look for a buffer that's allocated, big enough, but not in use.

[Ami GONE FROM CHROMIUM, 2013/10/17 20:31:45]

linguistic nit: s/but/and/

[ncarter (slow), 2013/10/22 01:06:21]

Done.

+  *buffer_id_to_drop = kInvalidId;
+  for (BufferMap::iterator it = buffers_.begin(); it != buffers_.end(); it++) {
+    Buffer* buffer = it->second;
+    if (!buffer->consumer_hold_count && !buffer->held_by_producer) {
+      if (buffer->shared_memory.requested_size() >= size) {
+        // Existing buffer is big enough. Reuse it.
+        buffer->held_by_producer = true;
+        return it->first;
+      }
+    }
+  }
+
+  // Look for a buffer that's not in use, that we can reallocate.

[Ami GONE FROM CHROMIUM, 2013/10/17 20:31:45]

Is it important to only do this paragraph if buffers_.size() < count_ ?

[ncarter (slow), 2013/10/22 01:06:21]

if (buffers_.size() == count_) then it's required that we do this (otherwise
there would be no reallocation). This paragraph will cause buffers_.size() to go
down, allowing the third part to trigger. If buffers_.size() < count, then the
free is not strictly required, but I see no benefit to not doing it in that
case.

+  for (BufferMap::iterator it = buffers_.begin(); it != buffers_.end(); it++) {
+    Buffer* buffer = it->second;
+    if (!buffer->consumer_hold_count && !buffer->held_by_producer) {
+      // Existing buffer is too small. Free it so we can allocate a new one
+      // after the loop.
+      *buffer_id_to_drop = it->first;
+      buffers_.erase(it);
+      delete buffer;
       break;
     }
   }
-  if (buffer_id == -1)
-    return -1;
 
-  Buffer* buffer = buffers_[buffer_id];
-  CHECK_GE(buffer->shared_memory.requested_size(), size_);
-  buffer->held_by_producer = true;
-  return buffer_id;
+  // If possible, grow the pool by creating a new buffer.
+  if (static_cast<int>(buffers_.size()) < count_) {

[Ami GONE FROM CHROMIUM, 2013/10/17 20:31:45]

optional: If count_ was of type size_t then the ctor could do
count_(checked_numeric_cast<size_t>(count))
and no static_cast<int> here.  

[ncarter (slow), 2013/10/22 01:06:21]

count is 3. I am just not worried?

+    int buffer_id = next_buffer_id_++;
+    Buffer* buffer = new Buffer();
+    if (!buffer->shared_memory.CreateAndMapAnonymous(size))
+      return kInvalidId;
+    buffer->held_by_producer = true;
+    buffers_[buffer_id] = buffer;
+    return buffer_id;
+  }
+
+  // The pool is at its size limit, and all buffers are in use.
+  return kInvalidId;
 }
 
-bool VideoCaptureBufferPool::IsAllocated() const {
-  lock_.AssertAcquired();
-  return !buffers_.empty();
+VideoCaptureBufferPool::Buffer* VideoCaptureBufferPool::GetBuffer(
+    int buffer_id) {
+  BufferMap::iterator it = buffers_.find(buffer_id);
+  if (it == buffers_.end())
+    return NULL;
+  return it->second;
 }
 
 }  // namespace content