[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

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 000b0febdb94c274900fd734eaf616fecde209ba..d5272e2f80bda4e119d5eb342f61c18ffd556319 100644
--- a/media/capture/video/video_capture_device.h
+++ b/media/capture/video/video_capture_device.h
@@ -41,7 +41,38 @@ class Location;
 
 namespace media {
 
-class CAPTURE_EXPORT VideoCaptureDevice {
+class CAPTURE_EXPORT FrameReceiverObserver {

[miu, 2016/12/01 05:25:18]

Should this interface be in vcd_client.h instead?

[chfremer, 2016/12/02 01:28:29]

With the simplification, this interface is no longer needed.
Removed.

+ public:
+  virtual ~FrameReceiverObserver() {}
+
+  virtual void OnReceiverReportingUtilization(int buffer_id,
+                                              double utilization) = 0;
+
+  static double no_utilization_recorded() { return -1.0; }

[miu, 2016/12/01 05:25:18]

nit: static constexpr double kNoUtilizationRecorded = -1.0;

[chfremer, 2016/12/02 01:28:28]

Done.

+};
+
+// During processing of a video frame delivered to |receiver|, we may report

[miu, 2016/12/01 05:25:18]

1. Please fix: The code comment refers to "|receiver|", but this is a
class-level comment so there's no receiver variable.

2. Maybe this should be a method comment for OnConsumerReportingUtilization()
instead?

[chfremer, 2016/12/02 01:28:28]

Done.

+// back to the device under how much load the |receiver| is. The Device may
+// use this information to adjust the rate of data it pushes out to the
+// receiver to avoid overloading it.
+// 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.
+class CAPTURE_EXPORT ConsumerLoadObserver {

[miu, 2016/12/01 05:25:18]

History: At one point, we were wondering whether we might include other forms of
feedback with each frame. I only plumbed-in utilization because that was all
that was needed at the time.

So, you might consider naming this interface something like
ConsumerFeedbackObserver instead; and then we might add more methods in the
future (either per-frame, or maybe more aggregate feedback stats).

[chfremer, 2016/12/02 01:28:28]

Done.

+ public:
+  virtual ~ConsumerLoadObserver() {}
+  virtual void OnConsumerReportingUtilization(int frame_id,

[miu, 2016/12/01 05:25:18]

naming nit: s/frame_id/frame_feedback_id/ everywhere (i.e., a little more
descriptive to hint at how this is being used).

[chfremer, 2016/12/02 01:28:28]

Done.

+                                              double utilization){};

[miu, 2016/12/01 05:25:18]

style nit: add space before open brace

[chfremer, 2016/12/02 01:28:29]

Oops. Seems there was an extra ";" which caused the auto format to go crazy.
Fixed. 

+};
+
+class CAPTURE_EXPORT VideoCaptureDevice : public ConsumerLoadObserver {
  public:
 
   // Interface defining the methods that clients of VideoCapture must have. It
@@ -85,7 +116,8 @@ class CAPTURE_EXPORT VideoCaptureDevice {
                                         const VideoCaptureFormat& frame_format,
                                         int clockwise_rotation,
                                         base::TimeTicks reference_time,
-                                        base::TimeDelta timestamp) = 0;
+                                        base::TimeDelta timestamp,
+                                        int frame_id = 0) = 0;

[miu, 2016/12/01 05:25:18]

Please document the new argument.

[chfremer, 2016/12/02 01:28:28]

Done.

 
     // Reserve an output buffer into which contents can be captured directly.
     // The returned Buffer will always be allocated with a memory size suitable
@@ -112,14 +144,14 @@ class CAPTURE_EXPORT VideoCaptureDevice {
     // |timestamp|.
     // TODO(chfremer): Consider removing one of the two in order to simplify the
     // interface.
-    virtual void OnIncomingCapturedBuffer(
-        std::unique_ptr<Buffer> buffer,
-        const VideoCaptureFormat& frame_format,
-        base::TimeTicks reference_time,
-        base::TimeDelta timestamp) = 0;
-    virtual void OnIncomingCapturedVideoFrame(
-        std::unique_ptr<Buffer> buffer,
-        scoped_refptr<VideoFrame> frame) = 0;
+    virtual void OnIncomingCapturedBuffer(std::unique_ptr<Buffer> buffer,
+                                          const VideoCaptureFormat& format,
+                                          base::TimeTicks reference_time,
+                                          base::TimeDelta timestamp,
+                                          int frame_id) = 0;
+    virtual void OnIncomingCapturedVideoFrame(std::unique_ptr<Buffer> buffer,
+                                              scoped_refptr<VideoFrame> frame,
+                                              int frame_id) = 0;
 
     // Attempts to reserve the same Buffer provided in the last call to one of
     // the OnIncomingCapturedXXX() methods. This will fail if the content of the
@@ -144,7 +176,7 @@ class CAPTURE_EXPORT VideoCaptureDevice {
     virtual double GetBufferPoolUtilization() const = 0;
   };
 
-  virtual ~VideoCaptureDevice();
+  ~VideoCaptureDevice() override;
 
   // Prepares the video capturer for use. StopAndDeAllocate() must be called
   // before the object is deleted.