[Mojo Video Capture] Simplify media::VideoCaptureDevice::Client:Buffer to a struct

media/capture/video/video_capture_device.h

Index: media/capture/video/video_capture_device.h
diff --git a/media/capture/video/video_capture_device.h b/media/capture/video/video_capture_device.h
index 0ac1c3b94b25083d33e3013f4c738d7660979192..3613c7bef649422114425cb0a193494c19136c24 100644
--- a/media/capture/video/video_capture_device.h
+++ b/media/capture/video/video_capture_device.h
@@ -30,6 +30,7 @@
 #include "media/capture/capture_export.h"
 #include "media/capture/mojo/image_capture.mojom.h"
 #include "media/capture/video/scoped_result_callback.h"
+#include "media/capture/video/video_capture_buffer_handle.h"
 #include "media/capture/video/video_capture_device_descriptor.h"
 #include "media/capture/video_capture_types.h"
 #include "mojo/public/cpp/bindings/array.h"
@@ -47,7 +48,6 @@ class CAPTURE_EXPORT FrameBufferPool {
 
   virtual void SetBufferHold(int buffer_id) = 0;
   virtual void ReleaseBufferHold(int buffer_id) = 0;
-  virtual mojo::ScopedSharedBufferHandle GetHandleForTransit(int buffer_id) = 0;
 };
 
 class CAPTURE_EXPORT VideoFrameConsumerFeedbackObserver {
@@ -77,6 +77,13 @@ class CAPTURE_EXPORT VideoFrameConsumerFeedbackObserver {
   static constexpr double kNoUtilizationRecorded = -1.0;
 };
 
+class CAPTURE_EXPORT BufferAccessProvider {
+ public:
+  virtual ~BufferAccessProvider() {}
+  virtual mojo::ScopedSharedBufferHandle GetHandleForTransit() = 0;
+  virtual std::unique_ptr<VideoCaptureBufferHandle> GetReadWriteAccess() = 0;
+};
+
 class CAPTURE_EXPORT VideoCaptureDevice
     : public VideoFrameConsumerFeedbackObserver {
  public:
@@ -87,21 +94,21 @@ class CAPTURE_EXPORT VideoCaptureDevice
   // All clients must implement OnError().
   class CAPTURE_EXPORT Client {
    public:
-    // Memory buffer returned by Client::ReserveOutputBuffer().
-    class CAPTURE_EXPORT Buffer {
+    // Move-only struct representing access to and reservation of a buffer.
+    struct CAPTURE_EXPORT Buffer {

[miu, 2016/12/20 22:25:37]

Putting the prior two comments together, it then seems that we can rename
VideoCaptureBufferHandle to just "VideoCaptureBuffer" to better reflect that it
is the container of the data.

[miu, 2016/12/20 22:25:37]

naming: I'd suggest renaming this to BufferPtr because:

1. It carries pointer/ID information only, not the actual payload.
2. Is a move-only type that represents ownership.
3. Conventionally similar to objects of similar behavior (e.g., Mojo FooPtrs).

[miu, 2016/12/20 22:25:37]

Random thinking out-loud: IIUC the usage cases correctly, it seems that this
class is really just a smart pointer type with a couple of ID fields as metadata
to be carried alongside and eventually passed-back to VCD::Client. Thus, I'm
wondering if it would simplify a little to write it this way:

  class CAPTURE_EXPORT BufferPtr : public std::unique_ptr<BufferAccessProvider>
{
   public:
    ...ctors/dtor as before...

    int id() const { return id_; }
    int frame_feedback_id() const { return frame_feedback_id_; }

   private:
    int id_, frame_feedback_id_;
  };

Then, code that uses this might look like:

  BufferPtr buffer = client->ReserveOutputBuffer(...);
  if (buffer) {
    MyRoutineToPopulateVideoFrameBuffer(buffer->GetReadWriteAccess());
    client->OnIncomingCapturedBuffer(std::move(buffer), ...);
  } else {
    LOG(ERROR) << "Failed to reserve output buffer for next video frame.";
  }

[chfremer, 2016/12/22 19:01:20]

I like this idea, and I would go with it if I didn't already know where things
are going in the follow-up CL, see [1]. In there, I am proposing to separate the
buffer reservation aspect from the |access_provider| into a separate object
|reservation|. This allows expressing the (pre-)sharing the buffers separately
from the usage reservations more clearly. 

Because of that, this struct is supposed to be nothing more than a plain old set
of members that can be accessed individually as needed. It only needs to exist
to allow multiple return values from things like ReserveOutputBuffer().

[1] https://codereview.chromium.org/2583603002/

[chfremer, 2016/12/22 19:01:20]

Hmm, the way I see it, VideoCaptureBufferHandle is an abstraction to something
that provides read/write access to a block of memory. The ...Handle suffix seems
appropriate for this. Note that implementations of VideoCaptureBufferHandle may
or may not be directly owning the block of memory. In our case, the
implementation is SharedMemoryBufferHandle, which does not own the memory. It
just points to a SharedMemoryBufferTracker, which, in turn, is owned by the
buffer pool.

[chfremer, 2016/12/22 19:01:20]

I agree this would make sense if we were to follow your suggestion to make it
inherit std::unique_ptr<BufferAccessProvider>. Please see my other response (and
maybe the subsequent CL [1]) as for why I recommend we don't do this. And when
treating this type as a plain set of members, we probably also don't want to go
with the ...Ptr suffix.

[1] https://codereview.chromium.org/2583603002/

[miu, 2016/12/27 23:38:48]

You have a better grasp on the big picture than I do; but, my understanding was
that there was an advantage in making the reservations closely tied to the
access of the buffers: Read-write access to a buffer implied a "producer
reservation" exists around it, and read-only access to a buffer implied a
"consumer reservation" exists. If these things become separated, wouldn't that
code structure make it easier to have unintended bugs where reservations can
begin and end while data access is taking place? I may be misunderstanding the
end game here. ;-)

[chfremer, 2016/12/28 01:06:44]

You are making a good point, which makes me realize that I have not really been
clear on the design myself. The way I see it now is as follows. There are 3
"levels" of access to the shared buffers:
1. "Handle Access", i.e. access to a handle (either a pointer to mapped memory
for in-process consumers or an abstract handle that can be shared with
out-of-process consumers). Access to such a handle does imply neither read nor
write access to the contents of the buffer. "Handle Access" is provided
separately before any frame data are delivered.
2. "Read Access" is given to consumers when frame data is ready for consumption.
3. "ReadWrite Access" is only needed by the producer.

So far, these access levels were not clearly separated, which resulted in the
sharing and delivery mechanism for the frames being very hard to grasp based on
the interfaces. Currently, every call to
VideoFrameReceiver::OnIncomingCapturedVideoFrame() gets ReadWrite Access plus an
in-process handle plus access to a shareable out-of-process handle. The end game
here is to make things clear by giving only the type of access that is needed
when it is needed.

I have to admit, my current version of CL1.9.11 is still not quite there yet. I
will rework the Interfaces in there to make things clearer.

For this CL (1.9.10), this does not require any additional changes.
Am I correct in assuming that with this explanation you are okay with this
struct for now then? If not, please reply back to this thread. Otherwise, let us
continue the discussion on this in CL1.9.11.

      public:
-      virtual ~Buffer() = 0;
-      virtual int id() const = 0;
-      virtual int frame_feedback_id() const = 0;
-      virtual gfx::Size dimensions() const = 0;
-      virtual size_t mapped_size() const = 0;
-      virtual void* data(int plane) = 0;
-      void* data() { return data(0); }
-#if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS))
-      virtual base::FileDescriptor AsPlatformFile() = 0;
-#endif
-      virtual bool IsBackedByVideoFrame() const = 0;
-      virtual scoped_refptr<VideoFrame> GetVideoFrame() = 0;
+      Buffer();
+      Buffer(int buffer_id,
+             int frame_feedback_id,
+             std::unique_ptr<BufferAccessProvider> access_provider);
+      ~Buffer();
+      Buffer(Buffer&& other);
+      Buffer& operator=(Buffer&& other);
+
+      int id;

[miu, 2016/12/20 22:25:37]

It seems weird to expose these data members publicly. Do you expect client code
to need to change these?

IMHO, it feels like this should go back to its prior "class" form without
virtualizing the methods.

[chfremer, 2016/12/22 19:01:20]

Done. Maybe my idea of "plain old set of members" was a bit too "plain". We want
|id| and |frame_feedback_id| to be read-only, but cannot make them const if we
want the type to be movable. As per your example, I will make them private and
provide get-only accessors.

|access_provider| will still stay a public field, so I am not sure if we should
call it a struct or a class then.

[miu, 2016/12/27 23:38:48]

Seems fine, since the intent is to allow public modification. IMHO it would be
overkill to write getXXX()/setXXX()/takeXXX() methods around it ("less code that
does more" philosophy).

+      int frame_feedback_id;
+      std::unique_ptr<BufferAccessProvider> access_provider;
+      bool is_valid() { return access_provider != nullptr; }
     };
 
     virtual ~Client() {}
@@ -139,20 +146,19 @@ class CAPTURE_EXPORT VideoCaptureDevice
     // backing, but functions as a reservation for external input for the
     // purposes of buffer throttling.
     //
-    // The output buffer stays reserved and mapped for use until the Buffer
-    // object is destroyed or returned.
-    virtual std::unique_ptr<Buffer> ReserveOutputBuffer(
-        const gfx::Size& dimensions,
-        VideoPixelFormat format,
-        VideoPixelStorage storage,
-        int frame_feedback_id) = 0;
+    // The buffer stays reserved for use by the caller as long as it
+    // holds on to the contained |access_provider|.
+    virtual Buffer ReserveOutputBuffer(const gfx::Size& dimensions,
+                                       VideoPixelFormat format,
+                                       VideoPixelStorage storage,
+                                       int frame_feedback_id) = 0;
 
     // Provides VCD::Client with a populated Buffer containing the content of
     // the next video frame. The |buffer| must originate from an earlier call to
     // ReserveOutputBuffer().
     // See OnIncomingCapturedData for details of |reference_time| and
     // |timestamp|.
-    virtual void OnIncomingCapturedBuffer(std::unique_ptr<Buffer> buffer,
+    virtual void OnIncomingCapturedBuffer(Buffer buffer,
                                           const VideoCaptureFormat& format,
                                           base::TimeTicks reference_time,
                                           base::TimeDelta timestamp) = 0;
@@ -160,7 +166,7 @@ class CAPTURE_EXPORT VideoCaptureDevice
     // Extended version of OnIncomingCapturedBuffer() allowing clients to
     // pass a custom |visible_rect| and |additional_metadata|.
     virtual void OnIncomingCapturedBufferExt(
-        std::unique_ptr<Buffer> buffer,
+        Buffer buffer,
         const VideoCaptureFormat& format,
         base::TimeTicks reference_time,
         base::TimeDelta timestamp,
@@ -172,11 +178,10 @@ class CAPTURE_EXPORT VideoCaptureDevice
     // of the Buffer has not been preserved, or if the |dimensions|, |format|,
     // or |storage| disagree with how it was reserved via ReserveOutputBuffer().
     // When this operation fails, nullptr will be returned.
-    virtual std::unique_ptr<Buffer> ResurrectLastOutputBuffer(
-        const gfx::Size& dimensions,
-        VideoPixelFormat format,
-        VideoPixelStorage storage,
-        int new_frame_feedback_id) = 0;
+    virtual Buffer ResurrectLastOutputBuffer(const gfx::Size& dimensions,
+                                             VideoPixelFormat format,
+                                             VideoPixelStorage storage,
+                                             int new_frame_feedback_id) = 0;
 
     // An error has occurred that cannot be handled and VideoCaptureDevice must
     // be StopAndDeAllocate()-ed. |reason| is a text description of the error.