[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..955ed81a3e960b9329f43727382b62325e007c98 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,20 @@ class CAPTURE_EXPORT VideoFrameConsumerFeedbackObserver {
   static constexpr double kNoUtilizationRecorded = -1.0;
 };
 
+class CAPTURE_EXPORT BufferHandleProvider {
+ public:
+  virtual ~BufferHandleProvider() {}
+  virtual mojo::ScopedSharedBufferHandle GetHandleForInterProcessTransit() = 0;
+  virtual std::unique_ptr<VideoCaptureBufferHandle>
+  GetHandleForInProcessAccess() = 0;
+};
+
+// Destructor-only interface for encapsulating pure ownership.
+class CAPTURE_EXPORT Ownership {

[miu, 2016/12/30 01:03:22]

I think I've figured out why we've all been flip-flopping on this
"destructor-only interface." I think your current patch set shows clearly why it
is needed and desired, however the naming is incredibly abstract, so it always
"feels weird" to see it.

Thus, I suggest more-descriptive naming here. Something that says why this is
needed, but also rationalizes why it's hiding something internal that external
code need not know about. Here are some suggestions I've come up with, but feel
free to roll your own:

  ScopedPermission
  ScopedReservation
  PermissionLock

(BTW--The "ScopedXXX" is very consistent with the naming around similar usage
patterns throughout Chromium.)

One other thing to consider, but this is totally your call: I would suggest
making these classes (i.e., BufferHandleProvider and "whatever Ownership is
called next") inner classes of the VCD::Client::Buffer struct. Also, if
BufferHandleProvider is an inner class of Buffer, you could just name it
HandleProvider.

[chfremer, 2017/01/03 23:15:38]

Done.

+ public:
+  virtual ~Ownership() {}
+};
+
 class CAPTURE_EXPORT VideoCaptureDevice
     : public VideoFrameConsumerFeedbackObserver {
  public:
@@ -87,21 +101,29 @@ 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 a buffer handle as well as
+    // read-write permission to its contents.
+    struct CAPTURE_EXPORT Buffer {
      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<BufferHandleProvider> handle_provider,
+             std::unique_ptr<Ownership> buffer_read_write_permission);
+      ~Buffer();
+      Buffer(Buffer&& other);
+      Buffer& operator=(Buffer&& other);
+
+      bool is_valid() const { return handle_provider != nullptr; }
+      int id() const { return id_; }
+      int frame_feedback_id() const { return frame_feedback_id_; }
+
+      std::unique_ptr<BufferHandleProvider> handle_provider;
+      std::unique_ptr<Ownership> buffer_read_write_permission;
+
+     private:
+      int id_;
+      int frame_feedback_id_;
     };
 
     virtual ~Client() {}
@@ -139,20 +161,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 |buffer_read_write_permission|.
+    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 +181,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 +193,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.