[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

content/browser/renderer_host/media/video_capture_manager.cc

Index: content/browser/renderer_host/media/video_capture_manager.cc
diff --git a/content/browser/renderer_host/media/video_capture_manager.cc b/content/browser/renderer_host/media/video_capture_manager.cc
index 8ce5a592c86f08c53164c1cfe74b518f0231e207..c9c13cca84f355593ca92f388e8fe22f7b0bd529 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -51,6 +51,28 @@
 
 namespace {
 
+class ConsumerFeedbackObserverOnTaskRunner

[mcasas, 2016/12/03 01:35:48]

I would s/ConsumerFeedbackObserverOnTaskRunner/ConsumerFeedbackObserverImpl/
since working on a Task Runner should be an impl detail.

[chfremer, 2016/12/05 18:16:59]

I am not sure I follow why it would be a good idea to hide this information from
the name of the class. The only code seeing the name of the class is the factory
logic that instantiates it, which is the following code snippet:

    entry->video_capture_controller()->SetConsumerFeedbackObserver(
        base::MakeUnique<ConsumerFeedbackObserverOnTaskRunner>(
            device.get(), device_task_runner_));

For the factory logic it is very much desirable to have the class name
communicate what the instance is going to do, because otherwise, the reader
would have to go and hunt down the actual implementation to read and infer what
it does.

For the clients who operate the instance, we use the abstraction
"ConsumerFeedbackObserver" which hides away all implementation details including
the name and construction details of the chosen implementation.

+    : public media::ConsumerFeedbackObserver {
+ public:
+  ConsumerFeedbackObserverOnTaskRunner(
+      media::ConsumerFeedbackObserver* observer,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : observer_(observer), task_runner_(std::move(task_runner)) {}
+
+  void OnConsumerReportingUtilization(int frame_feedback_id,
+                                      double utilization) override {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &media::ConsumerFeedbackObserver::OnConsumerReportingUtilization,
+            base::Unretained(observer_), frame_feedback_id, utilization));
+  }
+
+ private:
+  media::ConsumerFeedbackObserver* observer_;

[mcasas, 2016/12/03 01:35:48]

media::ConsumerFeedbackObserver* const

[chfremer, 2016/12/05 18:16:59]

Done.

+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

[mcasas, 2016/12/03 01:35:48]

const 

[chfremer, 2016/12/05 18:17:00]

Done.

+};
+
 // Compares two VideoCaptureFormat by checking smallest frame_size area, then
 // by _largest_ frame_rate. Used to order a VideoCaptureFormats vector so that
 // the first entry for a given resolution has the largest frame rate, as needed
@@ -130,9 +152,16 @@ const media::VideoCaptureSessionId kFakeSessionId = -1;
 
 namespace content {
 
-// This class owns a pair VideoCaptureDevice - VideoCaptureController.
-// VideoCaptureManager owns all such pairs and is responsible for deleting the
-// instances when they are not used any longer.
+// Instances of this struct go through 3 different phases during their lifetime.
+// Phase 1: When first created (in GetOrCreateDeviceEntry()), this consists of
+// only a controller. Clients can already connect to the controller, but there
+// is no device present.
+// Phase 2: When a request to "start" the entry comes in (via
+// HandleQueuedStartRequest()), a VideoCaptureDevice::Client is created
+// via video_capture_controller()->NewDeviceClient() and is used to schedule the
+// creation and start of a VideoCaptureDevice on the Device Thread.
+// Phase 3: As soon as the creation of the VideoCaptureDevice is complete, it
+// is connected to the VideoCaptureController as the ConsumerFeedbackObserver.

[mcasas, 2016/12/03 01:35:48]

This reads like: VCD is connected to the VCC as a ConsumerFeedbackObserver?
because the "it" is unclear in l.164.

Shouldn't it say that a new ConsumerFeedbackObserverOnTaskRunner
is created and ownership passed to the VCC?

[chfremer, 2016/12/05 18:17:00]

That is correct. This is the message the comment is trying to convey.
Agreed. See latest patch set for my attempt to improve this.
 
I feel the threading and ownership details of the connection should be omitted
in this high-level description.

 class VideoCaptureManager::DeviceEntry {
  public:
   DeviceEntry(MediaStreamType stream_type,
@@ -418,9 +447,11 @@ void VideoCaptureManager::DoStopDevice(DeviceEntry* entry) {
            << " serial_id = " << entry->serial_id << ".";
   entry->video_capture_controller()->OnLog(
       base::StringPrintf("Stopping device: id: %s", entry->id.c_str()));
+  entry->video_capture_controller()->SetConsumerFeedbackObserver(nullptr);
 
+  // |entry->video_capture_device| can be null if creating the device has
+  // failed.
   if (entry->video_capture_device()) {
-    // |entry->video_capture_device| can be null if creating the device fails.
     device_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
@@ -524,9 +555,9 @@ void VideoCaptureManager::OnDeviceStarted(
   DVLOG(3) << __func__;
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_EQ(serial_id, device_start_queue_.begin()->serial_id());
+  // |device| can be null if creation failed in
+  //  DoStartDeviceCaptureOnDeviceThread.
   if (device_start_queue_.front().abort_start()) {
-    // |device| can be null if creation failed in
-    // DoStartDeviceCaptureOnDeviceThread.
     // The device is no longer wanted. Stop the device again.
     DVLOG(3) << "OnDeviceStarted but start request have been aborted.";
     media::VideoCaptureDevice* device_ptr = device.get();
@@ -541,6 +572,13 @@ void VideoCaptureManager::OnDeviceStarted(
     DeviceEntry* const entry = GetDeviceEntryBySerialId(serial_id);
     DCHECK(entry);
     DCHECK(!entry->video_capture_device());
+    // Passing raw pointer |device.get()| to the controller is safe,
+    // because we transfer ownership of it to |entry|. We are calling
+    // SetConsumerFeedbackObserver(nullptr) before releasing
+    // |entry->video_capture_device_| on the |device_task_runner_|.
+    entry->video_capture_controller()->SetConsumerFeedbackObserver(
+        base::MakeUnique<ConsumerFeedbackObserverOnTaskRunner>(
+            device.get(), device_task_runner_));
     entry->SetVideoCaptureDevice(std::move(device));
 
     if (entry->stream_type == MEDIA_DESKTOP_VIDEO_CAPTURE) {