[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..f7ff0aaeb2095183a402bc87fcbfa6378f1a7a95 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -32,6 +32,7 @@
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
 #include "media/capture/video/video_capture_device.h"
+#include "media/capture/video/video_capture_device_client.h"
 #include "media/capture/video/video_capture_device_factory.h"
 
 #if defined(ENABLE_SCREEN_CAPTURE) && !defined(OS_ANDROID)
@@ -130,9 +131,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 a

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

s/consists of a only a controller/consists of only a controller/

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

Done.

+// 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 VideoCaptureDevice::Client as the ConsumerLoadObserver.
 class VideoCaptureManager::DeviceEntry {
  public:
   DeviceEntry(MediaStreamType stream_type,
@@ -418,6 +426,7 @@ 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()->SetFrameReceiverObserver(nullptr);
 
   if (entry->video_capture_device()) {
     // |entry->video_capture_device| can be null if creating the device fails.
@@ -454,6 +463,10 @@ void VideoCaptureManager::HandleQueuedStartRequest() {
   DVLOG(3) << "HandleQueuedStartRequest, Post start to device thread, device = "
            << entry->id << " start id = " << entry->serial_id;
 
+  std::unique_ptr<media::VideoCaptureDeviceClient> device_client =
+      entry->video_capture_controller()->NewDeviceClient();
+  media::VideoCaptureDeviceClient* device_client_ptr = device_client.get();
+
   base::Callback<std::unique_ptr<VideoCaptureDevice>(void)>
       start_capture_function;
 
@@ -470,10 +483,10 @@ void VideoCaptureManager::HandleQueuedStartRequest() {
                                found->descriptor.GetNameAndModel().c_str(),
                                found->descriptor.GetCaptureApiTypeString()));
 
-        start_capture_function = base::Bind(
-            &VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread, this,
-            found->descriptor, request->params(),
-            base::Passed(entry->video_capture_controller()->NewDeviceClient()));
+        start_capture_function =
+            base::Bind(&VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread,
+                       this, found->descriptor, request->params(),
+                       base::Passed(std::move(device_client)));
       } else {
         // Errors from DoStartDeviceCaptureOnDeviceThread go via
         // VideoCaptureDeviceClient::OnError, which needs some thread
@@ -494,17 +507,17 @@ void VideoCaptureManager::HandleQueuedStartRequest() {
       break;
     }
     case MEDIA_TAB_VIDEO_CAPTURE:
-      start_capture_function = base::Bind(
-          &VideoCaptureManager::DoStartTabCaptureOnDeviceThread, this,
-          entry->id, request->params(),
-          base::Passed(entry->video_capture_controller()->NewDeviceClient()));
+      start_capture_function =
+          base::Bind(&VideoCaptureManager::DoStartTabCaptureOnDeviceThread,
+                     this, entry->id, request->params(),
+                     base::Passed(std::move(device_client)));
       break;
 
     case MEDIA_DESKTOP_VIDEO_CAPTURE:
-      start_capture_function = base::Bind(
-          &VideoCaptureManager::DoStartDesktopCaptureOnDeviceThread, this,
-          entry->id, request->params(),
-          base::Passed(entry->video_capture_controller()->NewDeviceClient()));
+      start_capture_function =
+          base::Bind(&VideoCaptureManager::DoStartDesktopCaptureOnDeviceThread,
+                     this, entry->id, request->params(),
+                     base::Passed(std::move(device_client)));
       break;
 
     default: {
@@ -515,18 +528,19 @@ void VideoCaptureManager::HandleQueuedStartRequest() {
   base::PostTaskAndReplyWithResult(
       device_task_runner_.get(), FROM_HERE, start_capture_function,
       base::Bind(&VideoCaptureManager::OnDeviceStarted, this,
-                 request->serial_id()));
+                 request->serial_id(), device_client_ptr));

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

Is this safe (passing the raw pointer)? Could, in some rare timings, the device
client be destroyed before the task runs? I ask because it looks like we're
going over to the device thread to start the capture device, transferring
ownership of the device client to that thread, and then we are hopping back to
the IO thread to call OnDeviceStarted().

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

Yes, but that fact that you had to raise the question indicates that it was too
non-obvious why it worked. 

The good news is that with the simplifications enabled by your other
suggestions, we no longer need to do this. The controller can now talk directly
to the device instead of going through the device_client. 

 }
 
 void VideoCaptureManager::OnDeviceStarted(
     int serial_id,
+    media::VideoCaptureDeviceClient* device_client_ptr,
     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();
@@ -541,6 +555,10 @@ void VideoCaptureManager::OnDeviceStarted(
     DeviceEntry* const entry = GetDeviceEntryBySerialId(serial_id);
     DCHECK(entry);
     DCHECK(!entry->video_capture_device());
+    if (device) {
+      entry->video_capture_controller()->SetFrameReceiverObserver(
+          device_client_ptr);
+    }
     entry->SetVideoCaptureDevice(std::move(device));
 
     if (entry->stream_type == MEDIA_DESKTOP_VIDEO_CAPTURE) {
@@ -569,7 +587,7 @@ std::unique_ptr<media::VideoCaptureDevice>
 VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread(
     const VideoCaptureDeviceDescriptor& descriptor,
     const media::VideoCaptureParams& params,
-    std::unique_ptr<VideoCaptureDevice::Client> device_client) {
+    std::unique_ptr<media::VideoCaptureDeviceClient> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
   DCHECK(IsOnDeviceThread());
 
@@ -582,6 +600,7 @@ VideoCaptureManager::DoStartDeviceCaptureOnDeviceThread(
     return nullptr;
   }
 
+  device_client->SetConsumerLoadObserver(video_capture_device.get());
   video_capture_device->AllocateAndStart(params, std::move(device_client));
   return video_capture_device;
 }
@@ -590,7 +609,7 @@ std::unique_ptr<media::VideoCaptureDevice>
 VideoCaptureManager::DoStartTabCaptureOnDeviceThread(
     const std::string& id,
     const media::VideoCaptureParams& params,
-    std::unique_ptr<VideoCaptureDevice::Client> device_client) {
+    std::unique_ptr<media::VideoCaptureDeviceClient> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
   DCHECK(IsOnDeviceThread());
 
@@ -602,6 +621,7 @@ VideoCaptureManager::DoStartTabCaptureOnDeviceThread(
     return nullptr;
   }
 
+  device_client->SetConsumerLoadObserver(video_capture_device.get());

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

OOC, should we call SetConsumerLoadObserver(nullptr) before StopAndDeallocate()?
Or, do we know that it's impossible for the load observer to have its method
called any time at or after that point? Could there be pending tasks that will
call the observer's method in some other thread's task queue?

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

Again, this is no longer needed with the other simplifications.
I added the following comment to the place where the controller is connected to
the 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_|.

   video_capture_device->AllocateAndStart(params, std::move(device_client));
   return video_capture_device;
 }
@@ -610,7 +630,7 @@ std::unique_ptr<media::VideoCaptureDevice>
 VideoCaptureManager::DoStartDesktopCaptureOnDeviceThread(
     const std::string& id,
     const media::VideoCaptureParams& params,
-    std::unique_ptr<VideoCaptureDevice::Client> device_client) {
+    std::unique_ptr<media::VideoCaptureDeviceClient> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
   DCHECK(IsOnDeviceThread());
 
@@ -648,6 +668,7 @@ VideoCaptureManager::DoStartDesktopCaptureOnDeviceThread(
     return nullptr;
   }
 
+  device_client->SetConsumerLoadObserver(video_capture_device.get());
   video_capture_device->AllocateAndStart(params, std::move(device_client));
   return video_capture_device;
 }