Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_buffer_pool.h

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_BUFFER_POOL_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_BUFFER_POOL_H_
 
 #include <stddef.h>
 
 #include <map>
 
 #include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
 #include "base/process/process.h"
 #include "base/synchronization/lock.h"
 #include "build/build_config.h"
 #include "content/common/content_export.h"
 #include "media/base/video_capture_types.h"
 #include "media/base/video_frame.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 
 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

[miu, 2016/09/12 20:16:46]

The naming feels reversed to me: Shouldn't the interface be named
VideoCaptureBufferPool, and the concrete implementation be named
ProducerVideoCaptureBufferPool?

[chfremer, 2016/09/12 22:23:33]

I understand why it feels reversed. 
The reason I chose the names like this is as follows:
Instances of VideoCaptureBufferPool seem to have more than one type of client.
One such client is the class media::VideoCaptureDeviceClient. This client calls
only a subset of the public methods of VideoCaptureBufferPool. These are the
methods I extracted into this interface. Based on the method names, I believed
that this client, from the perspective of the pool, is called a "Producer" (but
I may be off with this).

Without having it analyzed too much, I am assuming that there is at least a
second type of client that calls the remaining public methods, and I am assuming
that, from the perspective of the pool, is called a "Consumer".

To properly model this, we should have the concrete class VideoCaptureBufferPool
expose two separate interfaces (one for each type of client). To distinguish
them, my best idea for naming is to call one "ProducerVideoCaptureBufferPool"
and the other "ConsumerVideoCaptureBufferPool", even though I see that these
names are not great at explaining themselves.

I have extracted the methods for the "Producer" client, but not yet the ones for
the "Consumer" client (since it was not yet needed). But maybe the change would
be clearer if I would do that second interface in this CL as well?

Please let me know if you have any good suggestions for this situation.

[chfremer, 2016/09/12 23:57:10]

Had an offline discussion with mcasas@ and emircan@. We agreed on doing the
following. Use an interface "VideoCaptureBufferPool" that contains _all_ public
methods (as opposed to just the ones that are called by
VideoCaptureDeviceClient). Then rename the implementation to
"VideoCaptureBufferPoolImpl". 

The downside of this solution is that it does not separate the interfaces
required by different client, which make them depend on more than they need. But
it makes it possible to conform to the simple scheme of having one interface and
one implementation.

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

One more thing maybe as a link to the "literature" explaining my initial
motivation to split the interfaces. This is the I in SOLID.

https://en.wikipedia.org/wiki/Interface_segregation_principle

[miu, 2016/09/13 00:33:29]

OIC now. Hmm...any other names I can think of sound stranger than this. Perhaps,
later in the refactoring process (a future change), it'll become more obvious
what to call this? ;-) ProducerVCBufferPool is fine for now.
 
Yep.
It's fine to leave this for a later change. BTW--You will have conflicts if the
ConsumerVCBufferPool also implements base::RefCountedThreadSafe<CVCBPool>, since
then VideoCaptureBufferPool would be inheriting two different
base::RefCountedThreadSafe<> types, each of which thinks it can "delete this".

Thus, if you really don't need the ref-counting, because object ownership and
lifetime is well-defined across threads, by all means *do* try to remove it.
This is likely easier to do in earlier changes (per my experience w/
refactoring, anyways).

+    : public base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool> {

[miu, 2016/09/12 20:16:46]

I looked in the design doc, but I could not find an answer to this question:
What are the lifetime semantics of the buffer pool? Before your changes, there
was a single buffer pool that is shared by all video capture devices, and it
lived in the browser process (memory shared to render processes as needed).

Are any of these properties changing in this change? How about in future
changes?

[chfremer, 2016/09/12 22:23:33]

My understanding of how things currently work is different. To me it seems that
there is one buffer pool per device. This CL does not change anything about
that. All it does it extract some interfaces to break dependencies.

I am planning to keep things the way they are now, i.e. one buffer pool per
device, but instead of "Chromium IPC shared memory" I will switch to "Mojo
shared buffers" down the line.

If it is helpful, I could go and add a some diagrams and explanations to the
design doc describing how things are working currently, and how I think they
will work when the transition to Mojo is complete.

[miu, 2016/09/13 00:33:29]

My mistake. I must've been remembering the design from a long time ago.
SGTM. You will need the same fine-grained control over the shared memory to
support things like the "resurrect" feature; and hopefully porting the code to
use the Mojo shmem will be trivial (IIUC).

+ 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;

[miu, 2016/09/12 20:16:46]

Why was 'const' removed from the method signature?

[chfremer, 2016/09/12 22:23:33]

This may be a topic worth discussing with a larger audience.

The rationale behind removing const qualifiers from methods that are extracted
to a pure interface is as follows.

When working with pure interfaces in C++ (i.e. classes with all purely virtual
methods), we can have concrete classes expose any number of different interfaces
simultaneously. The const qualifier on methods is basically a short-hand
notation for exposing exactly two interfaces, one const and one non-const. Using
pure interfaces, we can do the same thing by exposing two interfaces and naming
them, for example,
"ProducerVideoCaptureBufferPool" and "ConstProducerVideoCaptureBufferPool". 

Now there are some observations to make:
1.) We have one client that wants to consume the methods in
ProducerVideoCaptureBufferPool. We don't have any clients that would instead
want to just consume GetBufferPoolUtilization(). Therefore we don't need to
expose two different interfaces. 
2.) It is of no concern to clients how an implementation realizes an exposed
interfaces. Clients have no need to know whether an implementation is given
access to "this" or "const this". Therefore, the const qualifier should not be
part of the exposed interface.
3.) It is true that by removing the const from the method, the compiler no
longer enforces that the implementation can only interface with "const this".
That is acceptable, because it does not effectively prevent program state from
being modified anyway.

In conclusion: When working with pure interfaces, the const qualifier for
methods is not useful.

Please let me know if this explanation makes sense.

[miu, 2016/09/13 00:33:29]

I do have counter-arguments on this topic, but while they would be strong
arguments in many situations, they are not very strong in this particular
situation here. So, in short, practical considerations trump theoretical ones;
which is why I'm fine if you want to remove the const.

But my original question was: Why? I was curious whether some assumption
changed, because of the refactoring, that required the method to be non-const.

[chfremer, 2016/09/14 16:26:01]

Hehe, the answer to the "why?" was supposed to be that I tried to apply
something I believed to be a general good practice. There is no change in
semantics or behavior that would require the method to be non-const.

I would love to learn more about the counter-arguments and also if there are any
existing guidelines on how to handle this in Chromium. I feel there should be an
agreed upon practice for Chromium so that we can apply it consistently, but I
found nothing in the coding style docs. 

I don't think it makes sense if I am the only dev on the project that avoids the
use of const in interfaces while everyone else wants to see it be used. Is this
something that has been discussed before on the chromium-dev list? If not, I
could start a discussion thread there to see if this practice would get any
buy-in. Would that make sense?

[miu, 2016/09/16 21:10:25]

First of all, let me say: THANK YOU so much for caring about interface design,
naming, and code structure! :) I really wish more developers paid attention to
details like this because making poor decisions in these areas costs us *all*
dearly in terms of future code changes/maintenance.

As for general guidance, the Chromium C++ style guide starts with the Google C++
style guide, which discusses the use of const:
https://google.github.io/styleguide/cppguide.html#Use_of_const

Granted, in the case here, the discussion in the style guide could be
interpreted as either "make this const" or "it doesn't make sense to make this
const."  Here's how I look at this particular case:

1. Your argument (IIUC): The client of a "pure interface" shouldn't care whether
the implementation of the interface actually mutates state or not, so the
interface should not restrict that.

2. My support for your argument: The more "distant" an interface is from its
implementation and/or client code modules, the more I would tend to agree. The
reason here is that the cost of tighter-coupling of the client code and the
implementation--via the interface--is high. Meaning, making any interface
changes (such as removing the const), would take a lot more effort (e.g., more
code churn, more code reviewers, OWNERS stamps, etc.). What I mean by "distant"
interfaces: Those defined in src/content/public, third-party library includes,
etc.

3. My counter-arguments sum up to: Start restrictively, then relax; as a matter
of safety (and other "pros" listed in the style guide). As of today, we know all
implementations of this "getter" will not mutate state. Also, changing this
interface later (to remove the const) is very low cost because basically it
involves changing a small number of files, with code review from a small number
of developers around you. One other note: If there ever was a reason the
implementation needed to have the const removed, a reviewer would be presented
with a code change that shows a clear reason that a "getter" operation needs to
mutate state. The developer may actually chose to re-name the method when making
such a change.
If you start one, I'm sure you're gonna get 100 different answers/opinions. ;-)

FWIW, I drank this cool-aid long ago: http://gamesfromwithin.com/the-const-nazi
There have been several cases where I cleaned-up code as part of my project
efforts, added more consts, and actually found hidden bugs as a result.

I've also been told by others I can sometimes be too excessive in my use of
const. This is mainly influenced by my past experience with certain functional
programming languages, where the focus was on instantiating immutable values and
transforming them, rather than imperatively mutating variables.

[chfremer, 2016/09/17 00:09:21]

Thanks so much for these kind words and for spending the effort of presenting
such a clear and detailed response.
Agreed. I reviewed the guidelines and they are really quite vague on this topic.
Correct. And I think I would even go a bit further and say that a pure interface
should not dictate whether or not implementations are allowed to mutate state.
One example would be a Mock implementation of a getter method that
(legitimately) wants to increase a call counter.
Makes sense. The "distant" probably can mean the same as "widely used", which in
turn makes changes expensive.
"Start restrictively, then relax" sounds like a good principle to follow. A
downside of this could be that the change may already have become quite
expensive at the time where we decide to relax a restriction. But I realize that
facing the realities of a large project like this one, the advantages of
following this principle may outweigh the disadvantages by a wide margin.
To me it feels like the usefulness of const scales with the complexity of the
"units" we author. Idealistically, I would like to believe that, in theory, we
could make our units (such as methods, classes, and interfaces) small enough
such that const loses its usefulness. Using the example of a "getter" method, if
it is small enough, it should become completely obvious to both authors and
readers of the implementation that it does not mutate state. As a result of this
consideration, I feel that a need for relying on mechanisms such as const is a
warning sign that our "units" are too large and complex.

I realize that in an open-source project of this scale we are probably not going
to get to a point where const loses its usefulness. Considering that, I will go
with embracing const for now :-).

+
+ protected:
+  virtual ~ProducerVideoCaptureBufferPool() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool>;
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ProducerVideoCaptureBufferPool);
+};
+
 // 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
 // intended to be orchestrated by a VideoCaptureDevice::Client, but is designed
 // to outlive the controller if necessary. The pixel buffers may be backed by a
 // SharedMemory, but this is not compulsory.
 //
 // 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 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 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);
 
   // One-time (per client/per-buffer) initialization to share a particular
   // buffer to a process. The shared handle is returned as |new_handle|.
   bool ShareToProcess(int buffer_id,
                       base::ProcessHandle process_handle,
                       base::SharedMemoryHandle* new_handle);
   bool ShareToProcess2(int buffer_id,
                        int plane,
                        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
   // ReserveForProducer(), and not already passed to HoldForConsumers().
   void HoldForConsumers(int buffer_id, int num_clients);
 
   // Indicate that one or more consumers are done with a particular buffer. This
   // effectively is the opposite of HoldForConsumers(). Once the consumers are
   // 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;
   // Generic class to keep track of the state of a given mappable resource. Each
   // Tracker carries indication of pixel format and storage type.
   class Tracker {
    public:
     static std::unique_ptr<Tracker> CreateTracker(
         media::VideoPixelStorage storage);
 
@@ skipping 46 matching lines @@
     media::VideoPixelStorage storage_type_;
 
     // Indicates whether this Tracker is currently referenced by the producer.
     bool held_by_producer_;
 
     // Number of consumer processes which hold this Tracker.
     int consumer_hold_count_;
   };
 
   friend class base::RefCountedThreadSafe<VideoCaptureBufferPool>;
-  virtual ~VideoCaptureBufferPool();
+  ~VideoCaptureBufferPool() override;
 
   int ReserveForProducerInternal(const gfx::Size& dimensions,
                                  media::VideoPixelFormat format,
                                  media::VideoPixelStorage storage,
                                  int* tracker_id_to_drop);
 
   Tracker* GetTracker(int buffer_id);
 
   // The max number of buffers that the pool is allowed to have at any moment.
   const int count_;
@@ skipping 11 matching lines @@
   // The buffers, indexed by the first parameter, a buffer id.
   using TrackerMap = std::map<int, Tracker*>;
   TrackerMap trackers_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoCaptureBufferPool);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_BUFFER_POOL_H_