[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 BufferAccessProvider {
+ public:
+  virtual ~BufferAccessProvider() {}
+  virtual mojo::ScopedSharedBufferHandle GetHandleForTransit() = 0;
+  virtual std::unique_ptr<VideoCaptureBufferHandle> GetReadWriteAccess() = 0;
+};
+
 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 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() {}
 
     // 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 |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;
 
     // 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_