[Mojo Video Capture] Replace RESOURCE_UTILIZATION with interface ReceiverLoadObserver

content/browser/renderer_host/media/video_capture_manager.cc

 // 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.
 
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 33 matching lines @@
 #if defined(ENABLE_SCREEN_CAPTURE) && defined(OS_ANDROID)
 #include "content/browser/media/capture/screen_capture_device_android.h"
 #endif
 
 #if defined(OS_MACOSX)
 #include "media/base/mac/avfoundation_glue.h"
 #endif
 
 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
 // by the ConsolidateCaptureFormats() method.
 bool IsCaptureFormatSmaller(const media::VideoCaptureFormat& format1,
                             const media::VideoCaptureFormat& format2) {
   DCHECK(format1.frame_size.GetCheckedArea().IsValid());
   DCHECK(format2.frame_size.GetCheckedArea().IsValid());
   if (format1.frame_size.GetCheckedArea().ValueOrDefault(0) ==
       format2.frame_size.GetCheckedArea().ValueOrDefault(0)) {
@@ skipping 59 matching lines @@
 
 // Counter used for identifying a DeviceRequest to start a capture device.
 static int g_device_start_id = 0;
 
 const media::VideoCaptureSessionId kFakeSessionId = -1;
 
 }  // namespace
 
 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,
               const std::string& id,
               std::unique_ptr<VideoCaptureController> controller,
               const media::VideoCaptureParams& params);
   ~DeviceEntry();
 
   const int serial_id;
   const MediaStreamType stream_type;
@@ skipping 265 matching lines @@
       DVLOG(3) << "DoStopDevice, aborting start request for device "
                << entry->id << " serial_id = " << entry->serial_id;
       return;
     }
   }
 
   DVLOG(3) << "DoStopDevice. Send stop request for device = " << entry->id
            << " 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,
                    base::Passed(entry->ReleaseVideoCaptureDevice())));
   }
 }
 
 void VideoCaptureManager::HandleQueuedStartRequest() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Remove all start requests that have been aborted.
@@ skipping 83 matching lines @@
       base::Bind(&VideoCaptureManager::OnDeviceStarted, this,
                  request->serial_id()));
 }
 
 void VideoCaptureManager::OnDeviceStarted(
     int serial_id,
     std::unique_ptr<VideoCaptureDevice> device) {
   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();
     base::Closure closure =
         base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
                    base::Passed(&device));
     if (device_ptr && !device_task_runner_->PostTask(FROM_HERE, closure)) {
       // PostTask failed. The device must be stopped anyway.
       device_ptr->StopAndDeAllocate();
     }
   } else {
     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) {
       const media::VideoCaptureSessionId session_id =
           device_start_queue_.front().session_id();
       DCHECK(session_id != kFakeSessionId);
       MaybePostDesktopCaptureWindowId(session_id);
     }
 
     auto it = photo_request_queue_.begin();
@@ skipping 732 matching lines @@
     if (!device_in_queue) {
       // Session ID is only valid for Screen capture. So we can fake it to
       // resume video capture devices here.
       QueueStartDevice(kFakeSessionId, entry.get(), entry->parameters);
     }
   }
 }
 #endif  // defined(OS_ANDROID)
 
 }  // namespace content