VideoCaptureBufferPool: Refactor to allow support of non-ShMem backed buffers

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 f583a94adaa638396431ecda7017aa787d99aaa2..f3d6a936f155a57610c9d0b1cc35f27f2990c1e3 100644
--- a/content/browser/renderer_host/media/video_capture_buffer_pool.cc
+++ b/content/browser/renderer_host/media/video_capture_buffer_pool.cc
@@ -4,18 +4,77 @@
 
 #include "content/browser/renderer_host/media/video_capture_buffer_pool.h"
 
-#include "base/bind.h"
-#include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
-#include "media/base/video_frame.h"
-#include "media/base/video_util.h"
+
+using media::VideoFrame;
 
 namespace content {
 
 const int VideoCaptureBufferPool::kInvalidId = -1;
 
+VideoFrame::Format VideoPixelFormatToVideoFrameFormat(

[miu, 2015/04/08 22:26:14]

I seem to recall seeing this enum-mapping function in other places (maybe in
video_capture_device_client.cc?).  So, consider laying the foundation to prevent
future code duplication by making this an exported static function declared in
media/base/video_capture_types.h:

  MEDIA_EXPORT VideoFrame::Format VideoPixelFormatToVideoFrameFormat(...);

And then put this function in media/base/video_capture_types.cc.

[mcasas, 2015/04/09 02:03:22]

Perhaps you might be counting transforms
between FourCC or any other OS-enum and 
VideoPixelFormat/VideoFrame::Format, of which
there are plenty...

I'd rather not put it in video_capture_types.h
because that will link it with video_frame.h
and now they are separated entities, as I 
think they should be...

+    media::VideoPixelFormat pixel_format) {
+  static struct {
+    media::VideoPixelFormat pixel_format;
+    VideoFrame::Format frame_format;
+  } const kVideoFrameFormatToVideoPixelFormat[] = {
+      {media::PIXEL_FORMAT_I420, VideoFrame::I420},
+      {media::PIXEL_FORMAT_ARGB, VideoFrame::ARGB},
+      {media::PIXEL_FORMAT_TEXTURE, VideoFrame::NATIVE_TEXTURE},
+    };
+
+  for (const auto& format_pair : kVideoFrameFormatToVideoPixelFormat) {

[miu, 2015/04/08 22:26:14]

Instead of iterating a static array, consider using a switch/case statement so
the compiler will ensure we have all the mappings defined.

[mcasas, 2015/04/09 02:03:22]

Note that not all cases are contemplated here,
only those that we support allocation for, IOW
we don't want to reply NV21 if input format is
such one.

+    if (format_pair.pixel_format == pixel_format)
+      return format_pair.frame_format;
+  }
+  LOG(ERROR) << "Unsupported VideoPixelFormat "
+             << media::VideoCaptureFormat::PixelFormatToString(pixel_format);
+  return VideoFrame::UNKNOWN;
+}
+
+// Tracker specifics for SharedMemory.
+class VideoCaptureBufferPool::SharedMemTracker final : public Tracker {
+ public:
+  SharedMemTracker();
+
+  bool Init(VideoFrame::Format format, const gfx::Size& dimensions) override;
+  void* storage() override { return shared_memory_.memory(); }
+  size_t requested_size() override { return shared_memory_.requested_size(); }
+  size_t mapped_size() override { return shared_memory_.mapped_size(); }
+
+  bool ShareToProcess(base::ProcessHandle process_handle,
+                      base::SharedMemoryHandle* new_handle) override {
+    return shared_memory_.ShareToProcess(process_handle, new_handle);
+  }
+
+ private:
+  // The memory created to be shared with renderer processes.
+  base::SharedMemory shared_memory_;
+};
+
+VideoCaptureBufferPool::SharedMemTracker::SharedMemTracker()
+    : Tracker() {}
+
+bool VideoCaptureBufferPool::SharedMemTracker::Init(
+    VideoFrame::Format format,
+    const gfx::Size& dimensions) {
+  // Input |dimensions| can be 0x0 for trackers that do not require memory
+  // backing. The allocated size is calculated using VideoFrame methods since
+  // this will be the abstraction used to wrap the underlying data.
+  return shared_memory_.CreateAndMapAnonymous(
+      VideoFrame::AllocationSize(format, dimensions));
+}
+
+//static
+scoped_ptr<VideoCaptureBufferPool::Tracker>
+VideoCaptureBufferPool::Tracker::CreateTracker() {
+  return make_scoped_ptr(new SharedMemTracker());
+}
+
+VideoCaptureBufferPool::Tracker::~Tracker() {}
+
 VideoCaptureBufferPool::VideoCaptureBufferPool(int count)
     : count_(count),
       next_buffer_id_(0) {
@@ -23,7 +82,7 @@ VideoCaptureBufferPool::VideoCaptureBufferPool(int count)
 }
 
 VideoCaptureBufferPool::~VideoCaptureBufferPool() {
-  STLDeleteValues(&buffers_);
+  STLDeleteValues(&trackers_);
 }
 
 base::SharedMemoryHandle VideoCaptureBufferPool::ShareToProcess(
@@ -32,140 +91,148 @@ base::SharedMemoryHandle VideoCaptureBufferPool::ShareToProcess(
     size_t* memory_size) {
   base::AutoLock lock(lock_);
 
-  Buffer* buffer = GetBuffer(buffer_id);
-  if (!buffer) {
+  Tracker* tracker = GetTracker(buffer_id);
+  if (!tracker) {
     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;
+  if (tracker->ShareToProcess(process_handle, &remote_handle)) {
+    *memory_size = tracker->mapped_size();
+    return remote_handle;
+  }
+  DPLOG(ERROR) << "Error mapping Shared Memory.";
+  return base::SharedMemoryHandle();
 }
 
 bool VideoCaptureBufferPool::GetBufferInfo(int buffer_id,
-                                           void** memory,
+                                           void** storage,
                                            size_t* size) {
   base::AutoLock lock(lock_);
 
-  Buffer* buffer = GetBuffer(buffer_id);
-  if (!buffer) {
+  Tracker* tracker = GetTracker(buffer_id);
+  if (!tracker) {
     NOTREACHED() << "Invalid buffer_id.";
     return false;
   }
 
-  DCHECK(buffer->held_by_producer);
-  *memory = buffer->shared_memory.memory();
-  *size = buffer->shared_memory.mapped_size();
+  DCHECK(tracker->held_by_producer());
+  *storage = tracker->storage();
+  *size = tracker->mapped_size();
   return true;
 }
 
-int VideoCaptureBufferPool::ReserveForProducer(size_t size,
+int VideoCaptureBufferPool::ReserveForProducer(media::VideoPixelFormat format,
+                                               const gfx::Size& dimensions,
                                                int* buffer_id_to_drop) {
   base::AutoLock lock(lock_);
-  return ReserveForProducerInternal(size, buffer_id_to_drop);
+  return ReserveForProducerInternal(format, dimensions, buffer_id_to_drop);
 }
 
 void VideoCaptureBufferPool::RelinquishProducerReservation(int buffer_id) {
   base::AutoLock lock(lock_);
-  Buffer* buffer = GetBuffer(buffer_id);
-  if (!buffer) {
+  Tracker* tracker = GetTracker(buffer_id);
+  if (!tracker) {
     NOTREACHED() << "Invalid buffer_id.";
     return;
   }
-  DCHECK(buffer->held_by_producer);
-  buffer->held_by_producer = false;
+  DCHECK(tracker->held_by_producer());
+  tracker->set_held_by_producer(false);
 }
 
 void VideoCaptureBufferPool::HoldForConsumers(
     int buffer_id,
     int num_clients) {
   base::AutoLock lock(lock_);
-  Buffer* buffer = GetBuffer(buffer_id);
-  if (!buffer) {
+  Tracker* tracker = GetTracker(buffer_id);
+  if (!tracker) {
     NOTREACHED() << "Invalid buffer_id.";
     return;
   }
-  DCHECK(buffer->held_by_producer);
-  DCHECK(!buffer->consumer_hold_count);
+  DCHECK(tracker->held_by_producer());
+  DCHECK(!tracker->consumer_hold_count());
 
-  buffer->consumer_hold_count = num_clients;
-  // Note: |held_by_producer| will stay true until
+  tracker->set_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 tracker, e.g. a media::VideoFrame).
 }
 
 void VideoCaptureBufferPool::RelinquishConsumerHold(int buffer_id,
                                                     int num_clients) {
   base::AutoLock lock(lock_);
-  Buffer* buffer = GetBuffer(buffer_id);
-  if (!buffer) {
+  Tracker* tracker = GetTracker(buffer_id);
+  if (!tracker) {
     NOTREACHED() << "Invalid buffer_id.";
     return;
   }
-  DCHECK_GE(buffer->consumer_hold_count, num_clients);
+  DCHECK_GE(tracker->consumer_hold_count(), num_clients);
 
-  buffer->consumer_hold_count -= num_clients;
+  tracker->set_consumer_hold_count(tracker->consumer_hold_count() -
+                                   num_clients);
 }
 
-VideoCaptureBufferPool::Buffer::Buffer()
-    : held_by_producer(false), consumer_hold_count(0) {}
-
-int VideoCaptureBufferPool::ReserveForProducerInternal(size_t size,
-                                                       int* buffer_id_to_drop) {
+int VideoCaptureBufferPool::ReserveForProducerInternal(
+    media::VideoPixelFormat format,
+    const gfx::Size& dimensions,
+    int* buffer_id_to_drop) {
+  DCHECK(format == media::PIXEL_FORMAT_I420 ||
+         format == media::PIXEL_FORMAT_ARGB ||
+         format == media::PIXEL_FORMAT_TEXTURE);
   lock_.AssertAcquired();
+  const media::VideoFrame::Format frame_format =
+      VideoPixelFormatToVideoFrameFormat(format);
+  const size_t size_in_bytes =
+      VideoFrame::AllocationSize(frame_format, dimensions);
 
-  // Look for a buffer that's allocated, big enough, and not in use. Track the
-  // largest one that's not big enough, in case we have to reallocate a buffer.
+  // Look for a tracker that's allocated, big enough, and not in use. Track the
+  // largest one that's not big enough, in case we have to reallocate a tracker.
   *buffer_id_to_drop = kInvalidId;
   size_t realloc_size = 0;
-  BufferMap::iterator realloc = buffers_.end();
-  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;
+  TrackerMap::iterator tracker_to_drop = trackers_.end();
+  for (TrackerMap::iterator it = trackers_.begin(); it != trackers_.end();
+       ++it) {
+    Tracker* const tracker = it->second;
+    if (!tracker->consumer_hold_count() && !tracker->held_by_producer()) {
+      if (tracker->requested_size() >= size_in_bytes) {
+        // Existing tracker is big enough. Reuse it.
+        tracker->set_held_by_producer(true);
         return it->first;
       }
-      if (buffer->shared_memory.requested_size() > realloc_size) {
-        realloc_size = buffer->shared_memory.requested_size();
-        realloc = it;
+      if (tracker->requested_size() > realloc_size) {
+        realloc_size = tracker->requested_size();
+        tracker_to_drop = it;
       }
     }
   }
 
-  // Preferentially grow the pool by creating a new buffer. If we're at maximum
+  // Preferably grow the pool by creating a new tracker. If we're at maximum
   // size, then reallocate by deleting an existing one instead.
-  if (buffers_.size() == static_cast<size_t>(count_)) {
-    if (realloc == buffers_.end()) {
-      // We're out of space, and can't find an unused buffer to reallocate.
+  if (trackers_.size() == static_cast<size_t>(count_)) {
+    if (tracker_to_drop == trackers_.end()) {
+      // We're out of space, and can't find an unused tracker to reallocate.
       return kInvalidId;
     }
-    *buffer_id_to_drop = realloc->first;
-    delete realloc->second;
-    buffers_.erase(realloc);
+    *buffer_id_to_drop = tracker_to_drop->first;
+    delete tracker_to_drop->second;
+    trackers_.erase(tracker_to_drop);
   }
 
-  // Create the new buffer.
-  int buffer_id = next_buffer_id_++;
-  scoped_ptr<Buffer> buffer(new Buffer());
-  if (size) {
-    // |size| can be 0 for buffers that do not require memory backing.
-    if (!buffer->shared_memory.CreateAndMapAnonymous(size))
-      return kInvalidId;
-  }
-  buffer->held_by_producer = true;
-  buffers_[buffer_id] = buffer.release();
+  // Create the new tracker.
+  const int buffer_id = next_buffer_id_++;
+  scoped_ptr<Tracker> tracker = Tracker::CreateTracker();
+  if (!tracker->Init(frame_format, dimensions))
+    return kInvalidId;
+  tracker->set_held_by_producer(true);
+  trackers_[buffer_id] = tracker.release();
+
   return buffer_id;
 }
 
-VideoCaptureBufferPool::Buffer* VideoCaptureBufferPool::GetBuffer(
+VideoCaptureBufferPool::Tracker* VideoCaptureBufferPool::GetTracker(
     int buffer_id) {
-  BufferMap::iterator it = buffers_.find(buffer_id);
-  if (it == buffers_.end())
-    return NULL;
-  return it->second;
+  TrackerMap::const_iterator it = trackers_.find(buffer_id);
+  return (it == trackers_.end()) ? NULL : it->second;
 }
 
 }  // namespace content