From Video Capture, abolish OnFrameInfo and enable resolution changes

content/browser/renderer_host/media/video_capture_buffer_pool.h

Index: content/browser/renderer_host/media/video_capture_buffer_pool.h
diff --git a/content/browser/renderer_host/media/video_capture_buffer_pool.h b/content/browser/renderer_host/media/video_capture_buffer_pool.h
index 6d9607737dc4a9777e2c47f1c7714c10feff2ace..0d6eedac734532703ff0fd5fd234cdf3089851d8 100644
--- a/content/browser/renderer_host/media/video_capture_buffer_pool.h
+++ b/content/browser/renderer_host/media/video_capture_buffer_pool.h
@@ -5,6 +5,8 @@
 #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_BUFFER_POOL_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_BUFFER_POOL_H_
 
+#include <map>
+
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
@@ -25,43 +27,46 @@ namespace content {
 // A thread-safe class that does the bookkeeping and lifetime management for a
 // pool of shared-memory pixel buffers cycled between an in-process producer
 // (e.g. a VideoCaptureDevice) and a set of out-of-process consumers. The pool
-// is intended to be allocated and orchestrated by a VideoCaptureController, but
-// is designed to outlive the controller if necessary.
+// is intended to be orchestrated by a VideoCaptureController, but is designed
+// to outlive the controller if necessary.
+//
+// Producers get a buffer by calling ReserveForProducer(), and may pass on their
+// ownership to the consumer by calling HoldForConsumers(), or drop the buffer
+// (without further processing) by calling RelinquishProducerReservation().
+// Consumers signal that they are done with the buffer by calling
+// RelinquishConsumerHold().
 //
-// Buffers are identified by an int value called |buffer_id|. Callers may depend
-// on the buffer IDs being dense in the range [0, count()), so long as the
-// Allocate() step succeeded. -1 is never a valid ID, and is returned by some
-// methods to indicate failure. Producers get a buffer by calling
-// ReserveForProducer(), and may pass on their ownership to the consumer by
-// calling HoldForConsumers(), or drop the buffer (without further processing)
-// by calling ReserveForProducer(). Consumers signal that they are done with the
-// buffer by calling RelinquishConsumerHold().
+// Buffers are allocated on demand, but there will never be more than |count|
+// buffers in existence at any time. Buffers are identified by an int value
+// called |buffer_id|. -1 (kInvalidId) is never a valid ID, and is returned by
+// some methods to indicate failure. The active set of buffer ids may change
+// over the lifetime of the buffer pool, as existing buffers are freed and
+// reallocated at larger size. When reallocation occurs, new buffer IDs will
+// circulate.
 class CONTENT_EXPORT VideoCaptureBufferPool
     : public base::RefCountedThreadSafe<VideoCaptureBufferPool> {
  public:
-  VideoCaptureBufferPool(size_t size, int count);
-
-  // One-time initialization to allocate the shared memory buffers. Returns true
-  // on success.
-  bool Allocate();
+  static const int kInvalidId = -1;

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

enum { kInvalidId = -1 };
avoids the need for storage (which you're missing in the .cc file, which is
invalid C++ and which is a fatal under some toolchains)

[ncarter (slow), 2013/10/16 02:08:40]

Added storage. I think I'm not a fan of the enum trick; it seems such constants
do not play nicely with ASSERT_EQ on all platforms.

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

FTR: googletest's conflict is with unnamed enum types, and is triggered by
googletest's template magic that wants to use the unnamed type as a template
parameter (which is disallowed in the language).
Workarounds include:
- Use a non-prettyprinting variant: replace ASSERT_EQ(a, b) with ASSERT_TRUE(a
== b);
- Name the enum: replace "enum { kFoo = 42 };" with "enum MyEnum { kFoo = 42 };"
- don't use enums.

For me the joy of enums-over-static-data outweigh the pain of either of the
first two approaches.  Obvs., this is personal preference and you should do what
you like.

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

Good to know that adding the name fixes things.

+  VideoCaptureBufferPool(int count);

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

explicit

[ncarter (slow), 2013/10/16 02:08:40]

Done.

 
   // One-time (per client/per-buffer) initialization to share a particular
-  // buffer to a process.
+  // buffer to a process. The size of the allocation is returned as
+  // |memory_size|.
   base::SharedMemoryHandle ShareToProcess(int buffer_id,
-                                          base::ProcessHandle process_handle);
-
-  // Get the shared memory handle for a particular buffer index.
-  base::SharedMemoryHandle GetHandle(int buffer_id);
+                                          base::ProcessHandle process_handle,
+                                          size_t* memory_size);
 
-  // Get the mapped buffer memory for a particular buffer index.
-  void* GetMemory(int buffer_id);
-
-  // Locate the index of a buffer (if any) that's not in use by the producer or
-  // consumers, and reserve it. The buffer remains reserved (and writable by the
+  // Locate or allocate a buffer that's not in use by the producer or consumers,
+  // reserve it, and return its id. If no such buffer exists, returns

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

"no such buffer exists" is at odds with "allocate a buffer" on the previous
line.

[ncarter (slow), 2013/10/16 02:08:40]

Done.

+  // kInvalidId. The reserved buffer remains reserved (and writable by the
   // producer) until ownership is transferred either to the consumer via
   // HoldForConsumers(), or back to the pool with
   // RelinquishProducerReservation().
-  int ReserveForProducer();
+  //
+  // On occasion, this call will decide to free an old buffer to make room for a
+  // new allocation at a larger size. If so, the ID of the destroyed buffer is
+  // returned via |buffer_id_to_drop|.
+  int ReserveForProducer(size_t size, int* buffer_id_to_drop);
 
   // Indicate that a buffer held for the producer should be returned back to the
   // pool without passing on to the consumer. This effectively is the opposite
@@ -79,19 +84,20 @@ class CONTENT_EXPORT VideoCaptureBufferPool
   void RelinquishConsumerHold(int buffer_id, int num_clients);
 
   // Detect whether a particular SharedMemoryHandle is exported by a buffer that
-  // belongs to this pool -- that is, whether it was allocated by an earlier
-  // call to ReserveForProducer(). If so, return its buffer_id (a value on the
-  // range [0, count())). If not, return -1, indicating the buffer is not
-  // recognized (it may be a valid frame, but we didn't allocate it).
+  // belongs to this pool -- that is, whether it was reserved by an earlier call
+  // to ReserveForProducer(). If so, return its buffer_id. If not, return
+  // kInvalidId, indicating the buffer is not recognized (it may be a valid
+  // frame, but we didn't allocate it).
   int RecognizeReservedBuffer(base::SharedMemoryHandle maybe_belongs_to_pool);
 
-  // Utility functions to return a buffer wrapped in a useful type.
-  scoped_refptr<media::VideoFrame> ReserveI420VideoFrame(const gfx::Size& size,
-                                                         int rotation);
+  // Return a buffer wrapped in a useful type. If a reallocation occurred, the
+  // ID of the destroyed buffer is returned via |buffer_id_to_drop|.
+  scoped_refptr<media::VideoFrame> ReserveI420VideoFrame(
+      const gfx::Size& size,
+      int rotation,
+      int* buffer_id_to_drop);

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

ಠ_ಠ
When we discussed this I forgot that there were two Reserve* APIs  and thus that
this buffer_id_to_drop would show up in both.
This is _almost_ enough to push me to suggest there should be a callback that
the producer can register to be notified of droppage, but probably not quite
enough.  

Anyway, no action necessary, just thought I'd mention it if you were more on the
fence than I was and were looking for an excuse.

[ncarter (slow), 2013/10/16 02:08:40]

Every approach we discussed is fairly dirty, including this one. I'm going to
stick with it for now, as it's the simplest approach of the ones we've
considered. The future needs of the consumers (like native textures / hw encode)
will probably drive this API to be something else, but I can't quite see how
it's going to go.

 
   int count() const { return count_; }
-  size_t GetMemorySize() const;
-  bool IsAnyBufferHeldForConsumers();
 
  private:
   friend class base::RefCountedThreadSafe<VideoCaptureBufferPool>;
@@ -104,6 +110,9 @@ class CONTENT_EXPORT VideoCaptureBufferPool
     base::SharedMemory shared_memory;
 
     // Rotation in degrees of the buffer.
+    //
+    // TODO(jiayl): Move this out of this class. Clients can track rotation
+    // state themselves by means of a map keyed by buffer_id.

[jiayl, 2013/10/03 00:51:14]

What's the benefit of moving it into the clients? At which level of the pipeline
should it be moved to?

[ncarter (slow), 2013/10/16 02:08:40]

The design doc describes this and gives a rationale. I think it should move into
ColorConvertingCaptureRig. It doesn't belong here because this class ought to be
about allocating buffers, not tracking metadata about their contents.

     int rotation;
 
     // Tracks whether this buffer is currently referenced by the producer.
@@ -113,19 +122,24 @@ class CONTENT_EXPORT VideoCaptureBufferPool
     int consumer_hold_count;
   };
 
+  typedef std::map<int, Buffer*> BufferMap;

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

IDMap<Buffer, IDMapOwnPointer> to avoid having to remember to delete Buffers?

[ncarter (slow), 2013/10/16 02:08:40]

Thanks for this suggestion (I hadn't seen or used IDMap before). I tried it,
however, and it seems IDMap has a built-in thread checker that's incompatible
with how this map is used across threads. So, not done.

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

Oooh, interesting.

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

Done.

+
   virtual ~VideoCaptureBufferPool();
 
-  int ReserveForProducerInternal();
+  int ReserveForProducerInternal(size_t size, int* buffer_id_to_drop);
 
-  bool IsAllocated() const;
+  Buffer* GetBuffer(int buffer_id);
 
-  // Protects |buffers_| and contents thereof.
+  // Protects the mutable members of this class.
   base::Lock lock_;
 
-  // The buffers, indexed by |buffer_id|. Element 0 is always NULL.
-  ScopedVector<Buffer> buffers_;
+  // The ID of the next buffer.
+  int next_buffer_id_;
+
+  // The buffers, indexed by |buffer_id|.
+  BufferMap buffers_;
 
-  const size_t size_;
+  // The max number of buffers that the pool is allowed to have at any moment.

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

Why not put this as the first member, which would allow lock_ to be annotated as
protecting all the members below it?

[ncarter (slow), 2013/10/16 02:08:40]

Done.

   const int count_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoCaptureBufferPool);