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

media/capture/video/video_capture_device.h

 // Copyright (c) 2012 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.
 //
 // VideoCaptureDevice is the abstract base class for realizing video capture
 // device support in Chromium. It provides the interface for OS dependent
 // implementations.
 // The class is created and functions are invoked on a thread owned by
 // VideoCaptureManager. Capturing is done on other threads, depending on the OS
 // specific implementation.
@@ skipping 12 matching lines @@
 #include "base/files/file.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "media/base/video_frame.h"
 #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"
 #include "ui/gfx/gpu_memory_buffer.h"
 
 namespace tracked_objects {
 class Location;
 }  // namespace tracked_objects
 
 namespace media {
 
 class CAPTURE_EXPORT FrameBufferPool {
  public:
   virtual ~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 {
  public:
   virtual ~VideoFrameConsumerFeedbackObserver() {}
 
   // During processing of a video frame, consumers may report back their
   // utilization level to the source device. The device may use this information
   // to adjust the rate of data it pushes out. Values are interpreted as
   // follows:
   // Less than 0.0 is meaningless and should be ignored.  1.0 indicates a
   // maximum sustainable utilization.  Greater than 1.0 indicates the consumer
   // is likely to stall or drop frames if the data volume is not reduced.
   //
   // Example: In a system that encodes and transmits video frames over the
   // network, this value can be used to indicate whether sufficient CPU
   // is available for encoding and/or sufficient bandwidth is available for
   // transmission over the network.  The maximum of the two utilization
   // measurements would be used as feedback.
   //
   // The parameter |frame_feedback_id| must match a |frame_feedback_id|
   // previously sent out by the VideoCaptureDevice we are giving feedback about.
   // It is used to indicate which particular frame the reported utilization
   // corresponds to.
   virtual void OnUtilizationReport(int frame_feedback_id, double utilization) {}
 
   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:
 
   // Interface defining the methods that clients of VideoCapture must have. It
   // is actually two-in-one: clients may implement OnIncomingCapturedData() or
   // ReserveOutputBuffer() + OnIncomingCapturedVideoFrame(), or all of them.
   // 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() {}
 
     // Captured a new video frame, data for which is pointed to by |data|.
     //
     // The format of the frame is described by |frame_format|, and is assumed to
     // be tightly packed. This method will try to reserve an output buffer and
     // copy from |data| into the output buffer. If no output buffer is
     // available, the frame will be silently dropped. |reference_time| is
@@ skipping 17 matching lines @@
                                         int frame_feedback_id = 0) = 0;
 
     // Reserve an output buffer into which contents can be captured directly.
     // The returned Buffer will always be allocated with a memory size suitable
     // for holding a packed video frame with pixels of |format| format, of
     // |dimensions| frame dimensions. It is permissible for |dimensions| to be
     // zero; in which case the returned Buffer does not guarantee memory
     // 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;
 
     // 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,
         gfx::Rect visible_rect,
         const VideoFrameMetadata& additional_metadata) = 0;
 
     // Attempts to reserve the same Buffer provided in the last call to one of
     // the OnIncomingCapturedBufferXXX() methods. This will fail if the content
     // 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.
     virtual void OnError(const tracked_objects::Location& from_here,
                          const std::string& reason) = 0;
 
     // VideoCaptureDevice requests the |message| to be logged.
     virtual void OnLog(const std::string& message) {}
 
     // Returns the current buffer pool utilization, in the range 0.0 (no buffers
@@ skipping 87 matching lines @@
 
  private:
   // Gets the power line frequency from the current system time zone if this is
   // defined, otherwise returns 0.
   PowerLineFrequency GetPowerLineFrequencyForLocation() const;
 };
 
 }  // namespace media
 
 #endif  // MEDIA_CAPTURE_VIDEO_VIDEO_CAPTURE_DEVICE_H_