[Mojo Video Capture] Create abstraction VideoCaptureProvider

content/browser/renderer_host/media/in_process_video_capture_provider.cc

Index: content/browser/renderer_host/media/in_process_video_capture_provider.cc
diff --git a/content/browser/renderer_host/media/in_process_video_capture_provider.cc b/content/browser/renderer_host/media/in_process_video_capture_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bcabaf598c2c6b59051ae767a9011a30a31fef15
--- /dev/null
+++ b/content/browser/renderer_host/media/in_process_video_capture_provider.cc
@@ -0,0 +1,36 @@
+// Copyright 2016 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/in_process_video_capture_provider.h"
+
+#include "content/browser/renderer_host/media/in_process_buildable_video_capture_device.h"
+
+namespace content {
+
+InProcessVideoCaptureProvider::InProcessVideoCaptureProvider(
+    std::unique_ptr<media::VideoCaptureSystem> video_capture_system,
+    scoped_refptr<base::SingleThreadTaskRunner> device_task_runner)
+    : video_capture_system_(std::move(video_capture_system)),
+      device_task_runner_(std::move(device_task_runner)) {}
+
+InProcessVideoCaptureProvider::~InProcessVideoCaptureProvider() = default;

[emircan, 2017/04/10 20:48:12]

This class is owned by VideoCaptureManager. Since it is a scoped_refptr, I
couldn't exactly pinpoint where it is destrcuted and this class is destructed.
Can you write more info on this threading model? 

[chfremer, 2017/04/10 22:24:43]

The threading model of this class has 3 aspects:
1. It is irrelevant which thread it is constructed or destroyed on.
2. |result_callback| must guarantee that the called instance stays alive during
the asynchronous execution of GetDeviceInfosAsync(). 
3. The instance is not destroyed while any of its methods are executing.

I added a comment to the interface VideoCaptureProvider to express aspect 2.
Aspect 3 is a standard requirement for every class, so it should not require
being called out explicitly. I feel that aspect 1 is already expressed by not
adding any thread checks that would restrict things otherwise, so I am not sure
adding a class-level comment for this would add value. WDYT?

I think the reason you wanted to know which thread calls the destructor is to
check safety of the Unretained() use. With aspect 2. clarified, knowing this
should no longer be needed.

+
+void InProcessVideoCaptureProvider::GetDeviceInfosAsync(
+    const base::Callback<void(
+        const std::vector<media::VideoCaptureDeviceInfo>&)>& result_callback) {
+  device_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&media::VideoCaptureSystem::GetDeviceInfosAsync,
+                            base::Unretained(video_capture_system_.get()),

[emircan, 2017/04/10 20:48:12]

Add a comment why it is safe to use Unretained. AFAICT, VideoCaptureManager adds
a reference to itself in |result_callback| and makes sure itself and this class
is alive until this call is completed. However, if there is any other client of
this class, that wouldn't be intuitive. 

My suggestion would be calling like below to guarantee safety.
device_task_runner_->DeleteSoon(FROM_HERE, device_task_runner_.release());

[chfremer, 2017/04/10 22:24:43]

Done.

+                            result_callback));
+}
+
+std::unique_ptr<BuildableVideoCaptureDevice>
+InProcessVideoCaptureProvider::CreateBuildableDevice(
+    const std::string& device_id,
+    MediaStreamType stream_type) {
+  return base::MakeUnique<InProcessBuildableVideoCaptureDevice>(
+      device_task_runner_, video_capture_system_.get());
+}
+
+}  // namespace content