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..7dd1de8d5edb6a54c6be69519748a94d547368bf 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,73 @@
 
 namespace content {
 
+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 ProducerVideoCaptureBufferPool
+    : public base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool> {
+ 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 ~ProducerVideoCaptureBufferPool() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool>;
+};

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

nit : add here a
DISALLOW_IMPLICIT_CONSTRUCTORS(ClassName);

actually, it should be added to all new base classes,
either that macro or DISALLOW_COPY_AND_ASSIGN,
whichever makes sense

[chfremer, 2016/09/12 18:31:22]

Done, but I have a question about it.
Why is this needed? Interface classed cannot be instantiated anyway, because
they consist of purely virtual methods only.

[chfremer, 2016/09/13 00:30:56]

Hmm. I tried both, but neither of the macros seems to be suitable here. Since
they remove the default constructor, derived classes can no longer instantiate,
and we get a compiler error.

+
 // 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 +112,9 @@ namespace content {
 // reallocated at larger size. When reallocation occurs, new buffer IDs will
 // circulate.
 class CONTENT_EXPORT VideoCaptureBufferPool
-    : public base::RefCountedThreadSafe<VideoCaptureBufferPool> {
+    : public ProducerVideoCaptureBufferPool {
  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 +128,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 ProducerVideoCaptureBufferPool 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 +150,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 +216,7 @@ class CONTENT_EXPORT VideoCaptureBufferPool
   };
 
   friend class base::RefCountedThreadSafe<VideoCaptureBufferPool>;
-  virtual ~VideoCaptureBufferPool();
+  ~VideoCaptureBufferPool() override;
 
   int ReserveForProducerInternal(const gfx::Size& dimensions,
                                  media::VideoPixelFormat format,