[Mojo Video Capture] Decouple VCController from VCBufferPool and VCDeviceClient

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>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "content/browser/media/capture/desktop_capture_device_uma_types.h"
 #include "content/browser/media/capture/web_contents_video_capture_device.h"
 #include "content/browser/media/media_internals.h"
 #include "content/browser/renderer_host/media/video_capture_controller.h"
 #include "content/browser/renderer_host/media/video_capture_controller_event_handler.h"
+#include "content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h"
+#include "content/browser/renderer_host/media/video_frame_receiver_on_io_thread.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/desktop_media_id.h"
 #include "content/public/common/media_stream_request.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
+#include "media/capture/video/video_capture_buffer_pool_impl.h"
+#include "media/capture/video/video_capture_buffer_tracker_factory_impl.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)
 #include "content/browser/media/capture/desktop_capture_device.h"
 #if defined(USE_AURA)
 #include "content/browser/media/capture/desktop_capture_device_aura.h"
 #endif
 #endif
 
 #if defined(ENABLE_SCREEN_CAPTURE) && defined(OS_ANDROID)
@@ skipping 61 matching lines @@
   formats->erase(last, formats->end());
   // Mark all formats as I420, since this is what the renderer side will get
   // anyhow: the actual pixel format is decided at the device level.
   // Don't do this for Y16 format as it is handled separatelly.
   for (auto& format : *formats) {
     if (format.pixel_format != media::PIXEL_FORMAT_Y16)
       format.pixel_format = media::PIXEL_FORMAT_I420;
   }
 }
 
-// The maximum number of buffers in the capture pipeline. See
-// VideoCaptureController ctor comments for more details.
+// The maximum number of video frame buffers in-flight at any one time. This
+// value should be based on the logical capacity of the capture pipeline, and
+// not on hardware performance.  For example, tab capture requires more buffers
+// than webcam capture because the pipeline is longer (it includes read-backs
+// pending in the GPU pipeline).
 const int kMaxNumberOfBuffers = 3;
 // TODO(miu): The value for tab capture should be determined programmatically.
 // http://crbug.com/460318
 const int kMaxNumberOfBuffersForTabCapture = 10;
 
 // Used for logging capture events.
 // Elements in this enum should not be deleted or rearranged; the only
 // permitted operation is to add new elements before NUM_VIDEO_CAPTURE_EVENT.
 enum VideoCaptureEvent {
   VIDEO_CAPTURE_START_CAPTURE = 0,
   VIDEO_CAPTURE_STOP_CAPTURE_OK = 1,
   VIDEO_CAPTURE_STOP_CAPTURE_DUE_TO_ERROR = 2,
   VIDEO_CAPTURE_STOP_CAPTURE_OK_NO_FRAMES_PRODUCED_BY_DEVICE = 3,
   VIDEO_CAPTURE_STOP_CAPTURE_OK_NO_FRAMES_PRODUCED_BY_DESKTOP_OR_TAB = 4,
   NUM_VIDEO_CAPTURE_EVENT
 };
 
 void LogVideoCaptureEvent(VideoCaptureEvent event) {
   UMA_HISTOGRAM_ENUMERATION("Media.VideoCaptureManager.Event",
                             event,
                             NUM_VIDEO_CAPTURE_EVENT);
 }
 
 // Counter used for identifying a DeviceRequest to start a capture device.
 static int g_device_start_id = 0;
 
 const media::VideoCaptureSessionId kFakeSessionId = -1;
 
+std::unique_ptr<media::VideoCaptureJpegDecoder> CreateGpuJpegDecoder(
+    const media::VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) {
+  return base::MakeUnique<content::VideoCaptureGpuJpegDecoder>(decode_done_cb);
+}
+
 }  // namespace
 
 namespace content {
 
 // 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
+// via CreateDeviceClient() 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, this
 // newly created VideoCaptureDevice instance is connected to the
 // VideoCaptureController via SetConsumerFeedbackObserver().
-class VideoCaptureManager::DeviceEntry {
+struct VideoCaptureManager::DeviceEntry {
  public:
   DeviceEntry(MediaStreamType stream_type,
               const std::string& id,
               std::unique_ptr<VideoCaptureController> controller,
               const media::VideoCaptureParams& params);
   ~DeviceEntry();
+  std::unique_ptr<media::VideoCaptureDevice::Client> CreateDeviceClient();
+  std::unique_ptr<media::FrameBufferPool> CreateFrameBufferPool();
 
   const int serial_id;
   const MediaStreamType stream_type;
   const std::string id;
   const media::VideoCaptureParams parameters;
-
-  VideoCaptureController* video_capture_controller() const;
-  media::VideoCaptureDevice* video_capture_device() const;
-
-  void SetVideoCaptureDevice(std::unique_ptr<VideoCaptureDevice> device);
-  std::unique_ptr<VideoCaptureDevice> ReleaseVideoCaptureDevice();
-
- private:
-  const std::unique_ptr<VideoCaptureController> video_capture_controller_;
-
-  std::unique_ptr<VideoCaptureDevice> video_capture_device_;
-
-  base::ThreadChecker thread_checker_;
+  const std::unique_ptr<VideoCaptureController> video_capture_controller;
+  scoped_refptr<media::VideoCaptureBufferPool> buffer_pool;
+  std::unique_ptr<media::VideoCaptureDevice> video_capture_device;
 };
 
 // Bundles a media::VideoCaptureDeviceDescriptor with corresponding supported
 // video formats.
 struct VideoCaptureManager::DeviceInfo {
   DeviceInfo();
   DeviceInfo(media::VideoCaptureDeviceDescriptor descriptor);
   DeviceInfo(const DeviceInfo& other);
   ~DeviceInfo();
   DeviceInfo& operator=(const DeviceInfo& other);
 
   media::VideoCaptureDeviceDescriptor descriptor;
   media::VideoCaptureFormats supported_formats;
 };
 
+class BufferPoolFrameBufferPool : public media::FrameBufferPool {
+ public:
+  BufferPoolFrameBufferPool(

[miu, 2016/12/13 21:11:10]

'explicit'

[chfremer, 2016/12/13 23:12:40]

Done.

+      scoped_refptr<media::VideoCaptureBufferPool> buffer_pool)
+      : buffer_pool_(std::move(buffer_pool)) {}
+
+  void SetBufferHold(int buffer_id) override {
+    buffer_pool_->HoldForConsumers(buffer_id, 1);
+  }
+
+  void ReleaseBufferHold(int buffer_id) override {
+    buffer_pool_->RelinquishConsumerHold(buffer_id, 1);
+  }
+
+  mojo::ScopedSharedBufferHandle GetHandleForTransit(int buffer_id) override {
+    return buffer_pool_->GetHandleForTransit(buffer_id);
+  }
+
+ private:
+  scoped_refptr<media::VideoCaptureBufferPool> buffer_pool_;
+};
+
 // Class used for queuing request for starting a device.
 class VideoCaptureManager::CaptureDeviceStartRequest {
  public:
   CaptureDeviceStartRequest(int serial_id,
                             media::VideoCaptureSessionId session_id,
                             const media::VideoCaptureParams& params);
   int serial_id() const { return serial_id_; }
   media::VideoCaptureSessionId session_id() const { return session_id_; }
   media::VideoCaptureParams params() const { return params_; }
 
@@ skipping 13 matching lines @@
 
 VideoCaptureManager::DeviceEntry::DeviceEntry(
     MediaStreamType stream_type,
     const std::string& id,
     std::unique_ptr<VideoCaptureController> controller,
     const media::VideoCaptureParams& params)
     : serial_id(g_device_start_id++),
       stream_type(stream_type),
       id(id),
       parameters(params),
-      video_capture_controller_(std::move(controller)) {}
+      video_capture_controller(std::move(controller)) {}
 
 VideoCaptureManager::DeviceEntry::~DeviceEntry() {
-  DCHECK(thread_checker_.CalledOnValidThread());

[miu, 2016/12/13 21:11:10]

Is it okay that any of the objects DeviceEntry owns (such as VCController) could
be destroyed on any thread? If so, maybe comment that this point should be
reached only if there are no external references to the objects on any thread?
If not, you may need to post tasks to other threads to delete them...

[chfremer, 2016/12/13 23:12:39]

That would be not okay, and should never happen. DeviceEntry instances are
privately owned by the VideoCaptureManager instance and are deleted on the IO
thread. Added a 
DCHECK_CURRENTLY_ON(BrowserThread::IO) to make it clear.

   // DCHECK that this DeviceEntry does not still own a
   // media::VideoCaptureDevice. media::VideoCaptureDevice must be deleted on
   // the device thread.
-  DCHECK(video_capture_device_ == nullptr);
+  DCHECK(video_capture_device == nullptr);
 }
 
-void VideoCaptureManager::DeviceEntry::SetVideoCaptureDevice(
-    std::unique_ptr<VideoCaptureDevice> device) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  video_capture_device_.swap(device);
+std::unique_ptr<media::VideoCaptureDevice::Client>
+VideoCaptureManager::DeviceEntry::CreateDeviceClient() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  const int max_buffers = stream_type == MEDIA_TAB_VIDEO_CAPTURE

[miu, 2016/12/13 21:11:10]

Note: At some point, we should probably have the VideoCaptureDevice impl provide
this number (maybe specify it via a new method in VCDClient?), since different
capture pipelines will need something different. For example, there are two
different desktop capture impls: One that probably would work best with the tab
capture # of buffers, and one with the smaller #.

[chfremer, 2016/12/13 23:12:39]

Added this as a point for future discussion to the design doc.
I am not 100% sure the VideoCaptureDevice impl always has all the information
required to make this decision. The ideal buffer size probably depends not just
on the VideoCaptureDevice implementation itself, but also on the consumers, e.g.
how fast and reliable clients are able to consume.

+                              ? kMaxNumberOfBuffersForTabCapture
+                              : kMaxNumberOfBuffers;
+  buffer_pool = new media::VideoCaptureBufferPoolImpl(
+      base::MakeUnique<media::VideoCaptureBufferTrackerFactoryImpl>(),
+      max_buffers);
+
+  return base::MakeUnique<media::VideoCaptureDeviceClient>(
+      base::MakeUnique<VideoFrameReceiverOnIOThread>(
+          video_capture_controller->GetWeakPtrForIOThread()),
+      buffer_pool,
+      base::Bind(
+          &CreateGpuJpegDecoder,
+          base::Bind(&media::VideoFrameReceiver::OnIncomingCapturedVideoFrame,
+                     video_capture_controller->GetWeakPtrForIOThread())));
 }
 
-std::unique_ptr<media::VideoCaptureDevice>
-VideoCaptureManager::DeviceEntry::ReleaseVideoCaptureDevice() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  return std::move(video_capture_device_);
-}
-
-VideoCaptureController*
-VideoCaptureManager::DeviceEntry::video_capture_controller() const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  return video_capture_controller_.get();
-}
-
-media::VideoCaptureDevice*
-VideoCaptureManager::DeviceEntry::video_capture_device() const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  return video_capture_device_.get();
+std::unique_ptr<media::FrameBufferPool>
+VideoCaptureManager::DeviceEntry::CreateFrameBufferPool() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(buffer_pool);
+  return base::MakeUnique<BufferPoolFrameBufferPool>(buffer_pool);
 }
 
 VideoCaptureManager::DeviceInfo::DeviceInfo() = default;
 
 VideoCaptureManager::DeviceInfo::DeviceInfo(
     media::VideoCaptureDeviceDescriptor descriptor)
     : descriptor(descriptor) {}
 
 VideoCaptureManager::DeviceInfo::DeviceInfo(
     const VideoCaptureManager::DeviceInfo& other) = default;
@@ skipping 101 matching lines @@
   if (session_it == sessions_.end()) {
     NOTREACHED();
     return;
   }
 
   DeviceEntry* const existing_device =
       GetDeviceEntryByTypeAndId(session_it->second.type, session_it->second.id);
   if (existing_device) {
     // Remove any client that is still using the session. This is safe to call
     // even if there are no clients using the session.
-    existing_device->video_capture_controller()
-        ->StopSession(capture_session_id);
+    existing_device->video_capture_controller->StopSession(capture_session_id);
 
     // StopSession() may have removed the last client, so we might need to
     // close the device.
     DestroyDeviceEntryIfNoClients(existing_device);
   }
 
   // Notify listeners asynchronously, and forget the session.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&VideoCaptureManager::OnClosed, this,
                             session_it->second.type, capture_session_id));
@@ skipping 27 matching lines @@
     if (request->serial_id() == entry->serial_id) {
       request->set_abort_start();
       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(
+  entry->video_capture_controller->OnLog(
       base::StringPrintf("Stopping device: id: %s", entry->id.c_str()));
-  entry->video_capture_controller()->SetConsumerFeedbackObserver(nullptr);
+  entry->video_capture_controller->SetConsumerFeedbackObserver(nullptr);
+  entry->video_capture_controller->SetFrameBufferPool(nullptr);
 
   // |entry->video_capture_device| can be null if creating the device has
   // failed.
-  if (entry->video_capture_device()) {
+  if (entry->video_capture_device) {
     device_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
-                   base::Passed(entry->ReleaseVideoCaptureDevice())));
+                   base::Passed(&entry->video_capture_device)));
   }
 }
 
 void VideoCaptureManager::HandleQueuedStartRequest() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Remove all start requests that have been aborted.
   while (device_start_queue_.begin() != device_start_queue_.end() &&
       device_start_queue_.begin()->abort_start()) {
     device_start_queue_.pop_front();
   }
   DeviceStartQueue::iterator request = device_start_queue_.begin();
   if (request == device_start_queue_.end())
     return;
 
   const int serial_id = request->serial_id();
   DeviceEntry* const entry = GetDeviceEntryBySerialId(serial_id);
   DCHECK(entry);
 
   DVLOG(3) << "HandleQueuedStartRequest, Post start to device thread, device = "
            << entry->id << " start id = " << entry->serial_id;
 
+  std::unique_ptr<media::VideoCaptureDevice::Client> device_client =
+      entry->CreateDeviceClient();
+  std::unique_ptr<media::FrameBufferPool> frame_buffer_pool =
+      entry->CreateFrameBufferPool();
+
   base::Callback<std::unique_ptr<VideoCaptureDevice>(void)>
       start_capture_function;
 
   switch (entry->stream_type) {
     case MEDIA_DEVICE_VIDEO_CAPTURE: {
       // We look up the device id from the renderer in our local enumeration
       // since the renderer does not have all the information that might be
       // held in the browser-side VideoCaptureDevice::Name structure.
       const DeviceInfo* found = GetDeviceInfoById(entry->id);
       if (found) {
-        entry->video_capture_controller()->OnLog(
+        entry->video_capture_controller->OnLog(
             base::StringPrintf("Starting device: id: %s, name: %s, api: %s",
                                found->descriptor.device_id.c_str(),
                                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
         // dancing to get errors processed on the IO thread. But since
         // we're on that thread, we call VideoCaptureController
         // methods directly.
         const std::string log_message = base::StringPrintf(
             "Error on %s:%d: device %s unknown. Maybe recently disconnected?",
             __FILE__, __LINE__, entry->id.c_str());
         DLOG(ERROR) << log_message;
-        entry->video_capture_controller()->OnLog(log_message);
-        entry->video_capture_controller()->OnError();
+        entry->video_capture_controller->OnLog(log_message);
+        entry->video_capture_controller->OnError();
         // Drop the failed start request.
         device_start_queue_.pop_front();
 
         return;
       }
       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()));
+          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()));
+          entry->id, request->params(), base::Passed(std::move(device_client)));
       break;
 
     default: {
       NOTIMPLEMENTED();
       return;
     }
   }
+  // Passing raw pointer |device_client_ptr| is safe, because it will

[miu, 2016/12/13 21:11:10]

Did you mean "device_task_runner_" instead of "device_client_ptr" here?

[chfremer, 2016/12/13 23:12:39]

Nope. Sorry, this comment was just a leftover from a previous version of this CL
and I missed deleting it during merging. 

Deleted.

+  // be owned by the device returned by |start_capture_function|.
   base::PostTaskAndReplyWithResult(
       device_task_runner_.get(), FROM_HERE, start_capture_function,
       base::Bind(&VideoCaptureManager::OnDeviceStarted, this,
-                 request->serial_id()));
+                 request->serial_id(), base::Passed(&frame_buffer_pool)));
 }
 
 void VideoCaptureManager::OnDeviceStarted(
     int serial_id,
+    std::unique_ptr<media::FrameBufferPool> frame_buffer_pool,
     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()) {
     // 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<VideoFrameConsumerFeedbackObserverOnTaskRunner>(
-            device.get(), device_task_runner_));
-    entry->SetVideoCaptureDevice(std::move(device));
+    DCHECK(!entry->video_capture_device);
+    if (device) {

[miu, 2016/12/13 21:11:10]

Can |device| ever be null? Is this new if-statement necessary?

(If it can now be null sometimes, does that mean the code earlier in this method
should null-check it too?)

[chfremer, 2016/12/13 23:12:40]

Thanks for catching this.
Yes, |device| can be null (see l.574).
The code earlier in this method does check it, see l.583 where it is checked as
|device_ptr|.

Actually, the newly introduced if-statement should have already been present in
the previous CL to prevent that we call SetConsumerFeedbackObserver with a
VideoFrameConsumerFeedbackObserverOnTaskRunner forwarding to a nullptr.

+      entry->video_capture_controller->SetFrameBufferPool(
+          std::move(frame_buffer_pool));
+      // 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<VideoFrameConsumerFeedbackObserverOnTaskRunner>(
+              device.get(), device_task_runner_));
+    }
+    entry->video_capture_device = 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();
     while (it != photo_request_queue_.end()) {
       auto request = it++;
       DeviceEntry* maybe_entry = GetDeviceEntryBySessionId(request->first);
-      if (maybe_entry && maybe_entry->video_capture_device()) {
-        request->second.Run(maybe_entry->video_capture_device());
+      if (maybe_entry && maybe_entry->video_capture_device) {
+        request->second.Run(maybe_entry->video_capture_device.get());
         photo_request_queue_.erase(request);
       }
     }
   }
 
   device_start_queue_.pop_front();
   HandleQueuedStartRequest();
 }
 
 std::unique_ptr<media::VideoCaptureDevice>
@@ skipping 92 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DVLOG(1) << __func__ << ", session_id = " << session_id << ", request: "
            << media::VideoCaptureFormat::ToString(params.requested_format);
 
   DeviceEntry* entry = GetOrCreateDeviceEntry(session_id, params);
   if (!entry) {
     done_cb.Run(base::WeakPtr<VideoCaptureController>());
     return;
   }
 
-  DCHECK(entry->video_capture_controller());
+  DCHECK(entry->video_capture_controller);
 
   LogVideoCaptureEvent(VIDEO_CAPTURE_START_CAPTURE);
 
   // First client starts the device.
-  if (!entry->video_capture_controller()->HasActiveClient() &&
-      !entry->video_capture_controller()->HasPausedClient()) {
+  if (!entry->video_capture_controller->HasActiveClient() &&
+      !entry->video_capture_controller->HasPausedClient()) {
     DVLOG(1) << "VideoCaptureManager starting device (type = "
              << entry->stream_type << ", id = " << entry->id << ")";
     QueueStartDevice(session_id, entry, params);
   }
   // Run the callback first, as AddClient() may trigger OnFrameInfo().
-  done_cb.Run(entry->video_capture_controller()->GetWeakPtrForIOThread());
-  entry->video_capture_controller()->AddClient(
-      client_id, client_handler, session_id, params);
+  done_cb.Run(entry->video_capture_controller->GetWeakPtrForIOThread());
+  entry->video_capture_controller->AddClient(client_id, client_handler,
+                                             session_id, params);
 }
 
 void VideoCaptureManager::StopCaptureForClient(
     VideoCaptureController* controller,
     VideoCaptureControllerID client_id,
     VideoCaptureControllerEventHandler* client_handler,
     bool aborted_due_to_error) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(controller);
   DCHECK(client_handler);
@@ skipping 44 matching lines @@
   DCHECK(controller);
   DCHECK(client_handler);
   DeviceEntry* entry = GetDeviceEntryByController(controller);
   if (!entry)
     NOTREACHED() << "Got Null entry while pausing capture";
 
   const bool had_active_client = controller->HasActiveClient();
   controller->PauseClient(client_id, client_handler);
   if (!had_active_client || controller->HasActiveClient())
     return;
-  if (media::VideoCaptureDevice* device = entry->video_capture_device()) {
+  if (media::VideoCaptureDevice* device = entry->video_capture_device.get()) {
     device_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&VideoCaptureDevice::MaybeSuspend,
                    // Unretained is safe to use here because |device| would be
                    // null if it was scheduled for shutdown and destruction, and
                    // because this task is guaranteed to run before the task
                    // that destroys the |device|.
                    base::Unretained(device)));
   }
 }
 
 void VideoCaptureManager::ResumeCaptureForClient(
     media::VideoCaptureSessionId session_id,
     const media::VideoCaptureParams& params,
     VideoCaptureController* controller,
     VideoCaptureControllerID client_id,
     VideoCaptureControllerEventHandler* client_handler) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(controller);
   DCHECK(client_handler);
 
   DeviceEntry* entry = GetDeviceEntryByController(controller);
   if (!entry)
     NOTREACHED() << "Got Null entry while resuming capture";
 
   const bool had_active_client = controller->HasActiveClient();
   controller->ResumeClient(client_id, client_handler);
   if (had_active_client || !controller->HasActiveClient())
     return;
-  if (media::VideoCaptureDevice* device = entry->video_capture_device()) {
+  if (media::VideoCaptureDevice* device = entry->video_capture_device.get()) {
     device_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&VideoCaptureDevice::Resume,
                    // Unretained is safe to use here because |device| would be
                    // null if it was scheduled for shutdown and destruction, and
                    // because this task is guaranteed to run before the task
                    // that destroys the |device|.
                    base::Unretained(device)));
   }
 }
 
 void VideoCaptureManager::RequestRefreshFrameForClient(
     VideoCaptureController* controller) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (DeviceEntry* entry = GetDeviceEntryByController(controller)) {
-    if (media::VideoCaptureDevice* device = entry->video_capture_device()) {
+    if (media::VideoCaptureDevice* device = entry->video_capture_device.get()) {
       device_task_runner_->PostTask(
           FROM_HERE,
           base::Bind(&VideoCaptureDevice::RequestRefreshFrame,
                      // Unretained is safe to use here because |device| would be
                      // null if it was scheduled for shutdown and destruction,
                      // and because this task is guaranteed to run before the
                      // task that destroys the |device|.
                      base::Unretained(device)));
     }
   }
@@ skipping 27 matching lines @@
   if (it == sessions_.end())
     return false;
   DVLOG(1) << "GetDeviceFormatsInUse for device: " << it->second.name;
 
   // Return the currently in-use format(s) of the device, if it's started.
   DeviceEntry* device_in_use =
       GetDeviceEntryByTypeAndId(it->second.type, it->second.id);
   if (device_in_use) {
     // Currently only one format-in-use is supported at the VCC level.
     formats_in_use->push_back(
-        device_in_use->video_capture_controller()->GetVideoCaptureFormat());
+        device_in_use->video_capture_controller->GetVideoCaptureFormat());
   }
   return true;
 }
 
 void VideoCaptureManager::SetDesktopCaptureWindowId(
     media::VideoCaptureSessionId session_id,
     gfx::NativeViewId window_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   VLOG(2) << "SetDesktopCaptureWindowId called for session " << session_id;
 
   notification_window_ids_[session_id] = window_id;
   MaybePostDesktopCaptureWindowId(session_id);
 }
 
 void VideoCaptureManager::MaybePostDesktopCaptureWindowId(
     media::VideoCaptureSessionId session_id) {
   SessionMap::iterator session_it = sessions_.find(session_id);
   if (session_it == sessions_.end())
     return;
 
   DeviceEntry* const existing_device =
       GetDeviceEntryByTypeAndId(session_it->second.type, session_it->second.id);
   if (!existing_device) {
     DVLOG(2) << "Failed to find an existing screen capture device.";
     return;
   }
 
-  if (!existing_device->video_capture_device()) {
+  if (!existing_device->video_capture_device) {
     DVLOG(2) << "Screen capture device not yet started.";
     return;
   }
 
   DCHECK_EQ(MEDIA_DESKTOP_VIDEO_CAPTURE, existing_device->stream_type);
   DesktopMediaID id = DesktopMediaID::Parse(existing_device->id);
   if (id.is_null())
     return;
 
   auto window_id_it = notification_window_ids_.find(session_id);
   if (window_id_it == notification_window_ids_.end()) {
     DVLOG(2) << "Notification window id not set for screen capture.";
     return;
   }
 
   // Post |existing_device->video_capture_device| to the VideoCaptureDevice to
   // the device_task_runner_. This is safe since the device is destroyed on the
   // device_task_runner_.
   device_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&VideoCaptureManager::SetDesktopCaptureWindowIdOnDeviceThread,
-                 this,
-                 existing_device->video_capture_device(),
+                 this, existing_device->video_capture_device.get(),
                  window_id_it->second));
 
   notification_window_ids_.erase(window_id_it);
 }
 
 void VideoCaptureManager::GetPhotoCapabilities(
     int session_id,
     VideoCaptureDevice::GetPhotoCapabilitiesCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   const DeviceEntry* entry = GetDeviceEntryBySessionId(session_id);
   if (!entry)
     return;
-  VideoCaptureDevice* device = entry->video_capture_device();
+  VideoCaptureDevice* device = entry->video_capture_device.get();
   if (device) {
     VideoCaptureManager::DoGetPhotoCapabilities(std::move(callback), device);
     return;
   }
   // |entry| is known but |device| is nullptr, queue up a request for later.
   photo_request_queue_.emplace_back(
       session_id, base::Bind(&VideoCaptureManager::DoGetPhotoCapabilities, this,
                              base::Passed(&callback)));
 }
 
 void VideoCaptureManager::SetPhotoOptions(
     int session_id,
     media::mojom::PhotoSettingsPtr settings,
     VideoCaptureDevice::SetPhotoOptionsCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   const DeviceEntry* entry = GetDeviceEntryBySessionId(session_id);
   if (!entry)
     return;
-  VideoCaptureDevice* device = entry->video_capture_device();
+  VideoCaptureDevice* device = entry->video_capture_device.get();
   if (device) {
     VideoCaptureManager::DoSetPhotoOptions(std::move(callback),
                                            std::move(settings), device);
     return;
   }
   // |entry| is known but |device| is nullptr, queue up a request for later.
   photo_request_queue_.emplace_back(
       session_id, base::Bind(&VideoCaptureManager::DoSetPhotoOptions, this,
                              base::Passed(&callback), base::Passed(&settings)));
 }
 
 void VideoCaptureManager::TakePhoto(
     int session_id,
     VideoCaptureDevice::TakePhotoCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   const DeviceEntry* entry = GetDeviceEntryBySessionId(session_id);
   if (!entry)
     return;
-  VideoCaptureDevice* device = entry->video_capture_device();
+  VideoCaptureDevice* device = entry->video_capture_device.get();
   if (device) {
     VideoCaptureManager::DoTakePhoto(std::move(callback), device);
     return;
   }
   // |entry| is known but |device| is nullptr, queue up a request for later.
   photo_request_queue_.emplace_back(
       session_id, base::Bind(&VideoCaptureManager::DoTakePhoto, this,
                              base::Passed(&callback)));
 }
 
@@ skipping 87 matching lines @@
     ConsolidateCaptureFormats(&device_info.supported_formats);
     new_devices_info_cache.push_back(device_info);
   }
 
   on_devices_enumerated_callback.Run(new_devices_info_cache);
 }
 
 void VideoCaptureManager::DestroyDeviceEntryIfNoClients(DeviceEntry* entry) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Removal of the last client stops the device.
-  if (!entry->video_capture_controller()->HasActiveClient() &&
-      !entry->video_capture_controller()->HasPausedClient()) {
+  if (!entry->video_capture_controller->HasActiveClient() &&
+      !entry->video_capture_controller->HasPausedClient()) {
     DVLOG(1) << "VideoCaptureManager stopping device (type = "
              << entry->stream_type << ", id = " << entry->id << ")";
 
     // The DeviceEntry is removed from |devices_| immediately. The controller is
     // deleted immediately, and the device is freed asynchronously. After this
     // point, subsequent requests to open this same device ID will create a new
     // DeviceEntry, VideoCaptureController, and VideoCaptureDevice.
     DoStopDevice(entry);
     // TODO(mcasas): use a helper function https://crbug.com/624854.
     DeviceEntries::iterator device_it =
@@ skipping 29 matching lines @@
   return nullptr;
 }
 
 VideoCaptureManager::DeviceEntry*
 VideoCaptureManager::GetDeviceEntryByController(
     const VideoCaptureController* controller) const {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Look up |controller| in |devices_|.
   for (const std::unique_ptr<DeviceEntry>& device : devices_) {
-    if (device->video_capture_controller() == controller)
+    if (device->video_capture_controller.get() == controller)
       return device.get();
   }
   return nullptr;
 }
 
 VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetDeviceEntryBySerialId(
     int serial_id) const {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   for (const std::unique_ptr<DeviceEntry>& device : devices_) {
@@ skipping 24 matching lines @@
 
   // Check if another session has already opened this device. If so, just
   // use that opened device.
   DeviceEntry* const existing_device =
       GetDeviceEntryByTypeAndId(device_info.type, device_info.id);
   if (existing_device) {
     DCHECK_EQ(device_info.type, existing_device->stream_type);
     return existing_device;
   }
 
-  const int max_buffers = device_info.type == MEDIA_TAB_VIDEO_CAPTURE ?
-      kMaxNumberOfBuffersForTabCapture : kMaxNumberOfBuffers;
-  std::unique_ptr<VideoCaptureController> video_capture_controller(
-      new VideoCaptureController(max_buffers));
-  DeviceEntry* new_device =
+  auto video_capture_controller = base::MakeUnique<VideoCaptureController>();

[miu, 2016/12/13 21:11:10]

nit: Instead of creating separate locals and then just std::move'ing their
values, IMHO, it's simpler to:

  devices_.emplace_back(new DeviceEntry(...,
      base::MakeUnique<VideoCaptureController>(), params));
  return devices_.back();

Note: You might even simplify further and put the construction of the
VCController in the DeviceEntry ctor instead.

Note 2: And then, in the DeviceEntry class, maybe use composition instead of a
std::unique_ptr for the VCController member?

[chfremer, 2016/12/13 23:12:39]

Done.

+  auto new_device_entry =
       new DeviceEntry(device_info.type, device_info.id,
                       std::move(video_capture_controller), params);
-  devices_.emplace_back(new_device);
-  return new_device;
+  devices_.emplace_back(new_device_entry);
+  return new_device_entry;
 }
 
 void VideoCaptureManager::SetDesktopCaptureWindowIdOnDeviceThread(
     media::VideoCaptureDevice* device,
     gfx::NativeViewId window_id) {
   DCHECK(IsOnDeviceThread());
 #if defined(ENABLE_SCREEN_CAPTURE) && !defined(OS_ANDROID)
   DesktopCaptureDevice* desktop_device =
       static_cast<DesktopCaptureDevice*>(device);
   desktop_device->SetNotificationWindowId(window_id);
@@ skipping 65 matching lines @@
   }
 }
 
 void VideoCaptureManager::ResumeDevices() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   for (auto& entry : devices_) {
     // Do not resume Content Video Capture devices, e.g. Tab or Screen capture.
     // Do not try to restart already running devices.
     if (entry->stream_type != MEDIA_DEVICE_VIDEO_CAPTURE ||
-        entry->video_capture_device())
+        entry->video_capture_device)
       continue;
 
     // Check if the device is already in the start queue.
     bool device_in_queue = false;
     for (auto& request : device_start_queue_) {
       if (request.serial_id() == entry->serial_id) {
         device_in_queue = true;
         break;
       }
     }
 
     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