Break tight coupling of VideoCaptureDeviceClient to renderer_host

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 ba92d4d4f867a60938c4fa1498cefe12bd8d66aa..6b4c2a97ec61d57818544a8d8fe94b915ae69abf 100644
--- a/content/browser/renderer_host/media/video_capture_buffer_pool.h
+++ b/content/browser/renderer_host/media/video_capture_buffer_pool.h
@@ -24,6 +24,75 @@
 
 namespace content {
 
+// Abstraction of a pool's buffer data buffer and size for clients.
+// TODO(emircan): See https://crbug.com/521059, refactor this class.
+class VideoCaptureBufferPoolBufferHandle {
+ public:
+  virtual ~VideoCaptureBufferPoolBufferHandle() {}
+  virtual gfx::Size dimensions() const = 0;
+  virtual size_t mapped_size() const = 0;
+  virtual void* data(int plane) = 0;
+  virtual ClientBuffer AsClientBuffer(int plane) = 0;
+#if defined(OS_POSIX) && !defined(OS_MACOSX)
+  virtual base::FileDescriptor AsPlatformFile() = 0;
+#endif
+};
+
+class CONTENT_EXPORT VideoCaptureBufferPoolInterface

[mcasas, 2016/09/07 22:43:40]

Using Base as suffix is more commonplace than
Interface (49 versus 14, [1,2]). 



[1] find content/ chrome media | grep _base.h | wc -l
[2] find content/ chrome media | grep _interface.h | wc -l

[chfremer, 2016/09/08 17:28:51]

I agree that VideoCaptureBufferPoolInterface is probably not the best name for
this interface.
I don't think calling it "VideoCaptureBufferPoolBase" would be appropriate,
because it is not a base class in the sense that it inherits functionality to
child classes.

What it is, is the subset of method signatures from class VideoCaptureBufferPool
that class VideoCaptureDeviceClient depends on (i.e. want to invoke). In terms
of the buffer pool, clients interacting with that set of methods are probably
considered "producers", so maybe a good name would be
"ProducerVideoCaptureBufferPool".

Please let me know what you think about it.

[mcasas, 2016/09/09 22:28:01]

I see, in that case I'd say the usual naming convention
is VideoCaptureBufferPool and VideoCaptureBufferPoolImpl,
for base and derived class, resp.

+    : public base::RefCountedThreadSafe<VideoCaptureBufferPoolInterface> {
+ public:
+  static constexpr int kInvalidId = -1;
+
+  // Try and obtain a BufferHandle for |buffer_id|.
+  virtual std::unique_ptr<VideoCaptureBufferPoolBufferHandle> GetBufferHandle(
+      int buffer_id) = 0;
+
+  // Reserve or allocate a buffer to support a packed frame of |dimensions| of
+  // pixel |format| and return its id. This will fail (returning kInvalidId) if
+  // the pool already is at its |count| limit of the number of allocations, and
+  // all allocated buffers are in use by the producer and/or consumers.
+  //
+  // If successful, 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().
+  //
+  // 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|.
+  virtual int ReserveForProducer(const gfx::Size& dimensions,
+                                 media::VideoPixelFormat format,
+                                 media::VideoPixelStorage storage,
+                                 int* buffer_id_to_drop) = 0;
+
+  // 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
+  // of ReserveForProducer().
+  virtual void RelinquishProducerReservation(int buffer_id) = 0;
+
+  // Attempt to reserve the same buffer that was relinquished in the last call
+  // to RelinquishProducerReservation(). If the buffer is not still being
+  // consumed, and has not yet been re-used since being consumed, and the
+  // specified |dimensions|, |format|, and |storage| agree with its last
+  // reservation, this will succeed. Otherwise, |kInvalidId| will be returned.
+  //
+  // A producer may assume the content of the buffer has been preserved and may
+  // also make modifications.
+  virtual int ResurrectLastForProducer(const gfx::Size& dimensions,
+                                       media::VideoPixelFormat format,
+                                       media::VideoPixelStorage storage) = 0;
+
+  // Returns a snapshot of the current number of buffers in-use divided by the
+  // maximum |count_|.
+  virtual double GetBufferPoolUtilization() = 0;
+
+ protected:
+  virtual ~VideoCaptureBufferPoolInterface() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<VideoCaptureBufferPoolInterface>;
+};
+
 // A thread-safe class that does the bookkeeping and lifetime management for a
 // pool of pixel buffers cycled between an in-process producer (e.g. a
 // VideoCaptureDevice) and a set of out-of-process consumers. The pool is
@@ -45,23 +114,9 @@ namespace content {
 // reallocated at larger size. When reallocation occurs, new buffer IDs will
 // circulate.
 class CONTENT_EXPORT VideoCaptureBufferPool
-    : public base::RefCountedThreadSafe<VideoCaptureBufferPool> {
+    : public VideoCaptureBufferPoolInterface {
  public:
-  static const int kInvalidId;
-
-  // Abstraction of a pool's buffer data buffer and size for clients.
-  // TODO(emircan): See https://crbug.com/521059, refactor this class.
-  class BufferHandle {
-   public:
-    virtual ~BufferHandle() {}
-    virtual gfx::Size dimensions() const = 0;
-    virtual size_t mapped_size() const = 0;
-    virtual void* data(int plane) = 0;
-    virtual ClientBuffer AsClientBuffer(int plane) = 0;
-#if defined(OS_POSIX) && !defined(OS_MACOSX)
-    virtual base::FileDescriptor AsPlatformFile() = 0;
-#endif
-  };
+  using BufferHandle = VideoCaptureBufferPoolBufferHandle;
 
   explicit VideoCaptureBufferPool(int count);
 
@@ -75,31 +130,17 @@ class CONTENT_EXPORT VideoCaptureBufferPool
                        base::ProcessHandle process_handle,
                        gfx::GpuMemoryBufferHandle* new_handle);
 
-  // Try and obtain a BufferHandle for |buffer_id|.
-  std::unique_ptr<BufferHandle> GetBufferHandle(int buffer_id);
-
-  // Reserve or allocate a buffer to support a packed frame of |dimensions| of
-  // pixel |format| and return its id. This will fail (returning kInvalidId) if
-  // the pool already is at its |count| limit of the number of allocations, and
-  // all allocated buffers are in use by the producer and/or consumers.
-  //
-  // If successful, 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().
-  //
-  // 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|.
+  // Implementation of VideoCaptureBufferPoolInterface interface:
+  std::unique_ptr<BufferHandle> GetBufferHandle(int buffer_id) override;
   int ReserveForProducer(const gfx::Size& dimensions,
                          media::VideoPixelFormat format,
                          media::VideoPixelStorage storage,
-                         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
-  // of ReserveForProducer().
-  void RelinquishProducerReservation(int buffer_id);
+                         int* buffer_id_to_drop) override;
+  void RelinquishProducerReservation(int buffer_id) override;
+  int ResurrectLastForProducer(const gfx::Size& dimensions,
+                               media::VideoPixelFormat format,
+                               media::VideoPixelStorage storage) override;
+  double GetBufferPoolUtilization() override;
 
   // Transfer a buffer from producer to consumer ownership.
   // |buffer_id| must be a buffer index previously returned by
@@ -111,22 +152,6 @@ class CONTENT_EXPORT VideoCaptureBufferPool
   // done, a buffer is returned to the pool for reuse.
   void RelinquishConsumerHold(int buffer_id, int num_clients);
 
-  // Attempt to reserve the same buffer that was relinquished in the last call
-  // to RelinquishProducerReservation(). If the buffer is not still being
-  // consumed, and has not yet been re-used since being consumed, and the
-  // specified |dimensions|, |format|, and |storage| agree with its last
-  // reservation, this will succeed. Otherwise, |kInvalidId| will be returned.
-  //
-  // A producer may assume the content of the buffer has been preserved and may
-  // also make modifications.
-  int ResurrectLastForProducer(const gfx::Size& dimensions,
-                               media::VideoPixelFormat format,
-                               media::VideoPixelStorage storage);
-
-  // Returns a snapshot of the current number of buffers in-use divided by the
-  // maximum |count_|.
-  double GetBufferPoolUtilization() const;
-
  private:
   class GpuMemoryBufferTracker;
   class SharedMemTracker;
@@ -193,7 +218,7 @@ class CONTENT_EXPORT VideoCaptureBufferPool
   };
 
   friend class base::RefCountedThreadSafe<VideoCaptureBufferPool>;
-  virtual ~VideoCaptureBufferPool();
+  ~VideoCaptureBufferPool() override;
 
   int ReserveForProducerInternal(const gfx::Size& dimensions,
                                  media::VideoPixelFormat format,