[Mojo Video Capture] Introduce abstraction VideoCaptureSystem

media/capture/video/video_capture_system.cc

Index: media/capture/video/video_capture_system.cc
diff --git a/media/capture/video/video_capture_system.cc b/media/capture/video/video_capture_system.cc
new file mode 100644
index 0000000000000000000000000000000000000000..45419b6d741ba19f40427abc11b703b1f412bde5
--- /dev/null
+++ b/media/capture/video/video_capture_system.cc
@@ -0,0 +1,128 @@
+// Copyright 2014 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 "media/capture/video/video_capture_system.h"
+
+#include "media/base/bind_to_current_loop.h"
+
+namespace {
+
+// 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)) {
+    return format1.frame_rate > format2.frame_rate;
+  }
+  return format1.frame_size.GetCheckedArea().ValueOrDefault(0) <
+         format2.frame_size.GetCheckedArea().ValueOrDefault(0);
+}
+
+bool IsCaptureFormatSizeEqual(const media::VideoCaptureFormat& format1,
+                              const media::VideoCaptureFormat& format2) {
+  DCHECK(format1.frame_size.GetCheckedArea().IsValid());
+  DCHECK(format2.frame_size.GetCheckedArea().IsValid());
+  return format1.frame_size.GetCheckedArea().ValueOrDefault(0) ==
+         format2.frame_size.GetCheckedArea().ValueOrDefault(0);
+}
+
+// This function receives a list of capture formats, removes duplicated
+// resolutions while keeping the highest frame rate for each, and forcing I420
+// pixel format.
+void ConsolidateCaptureFormats(media::VideoCaptureFormats* formats) {
+  if (formats->empty())
+    return;
+  std::sort(formats->begin(), formats->end(), IsCaptureFormatSmaller);
+  // Due to the ordering imposed, the largest frame_rate is kept while removing
+  // duplicated resolutions.
+  media::VideoCaptureFormats::iterator last =
+      std::unique(formats->begin(), formats->end(), IsCaptureFormatSizeEqual);
+  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;
+  }
+}
+
+}  // anonymous namespace
+
+namespace media {
+
+VideoCaptureSystem::VideoCaptureSystem(
+    std::unique_ptr<VideoCaptureDeviceFactory> factory)
+    : factory_(std::move(factory)) {}
+
+VideoCaptureSystem::~VideoCaptureSystem() = default;
+
+void VideoCaptureSystem::GetDeviceInfosAsync(
+    const base::Callback<void(const std::vector<VideoCaptureDeviceInfo>&)>&
+        result_callback) {
+  factory_->EnumerateDeviceDescriptors(media::BindToCurrentLoop(
+      base::Bind(&VideoCaptureSystem::OnDescriptorsReceived,
+                 base::Unretained(this), result_callback)));
+}
+
+// Creates a VideoCaptureDevice object. Returns NULL if something goes wrong.
+std::unique_ptr<VideoCaptureDevice> VideoCaptureSystem::CreateDevice(
+    const std::string device_id) {
+  media::VideoCaptureDeviceDescriptor descriptor;
+  if (LookupDescriptorFromId(device_id, &descriptor) == false)
+    return nullptr;
+  return factory_->CreateDevice(descriptor);
+}
+
+void VideoCaptureSystem::OnDescriptorsReceived(
+    const base::Callback<void(const std::vector<VideoCaptureDeviceInfo>&)>&
+        result_callback,
+    std::unique_ptr<VideoCaptureDeviceDescriptors> descriptors) {
+  // For devices for which we already have an entry in |devices_info_cache_|,
+  // we do not want to query the |factory_| for supported formats again. We
+  // simply copy them from |devices_info_cache_|.

[miu, 2017/03/22 22:51:23]

nit: Add: "If |descriptors| does not contain a description for an entry in
|devices_info_cache_|, it will be effectively removed from |devices_info_cache_|
later on."

[chfremer, 2017/03/23 00:34:13]

With your suggested improvement from below, I think we no longer need this.

+  std::vector<VideoCaptureDeviceInfo> new_devices_info_cache;
+  for (const auto& device_info : devices_info_cache_) {

[miu, 2017/03/22 22:51:23]

On a second review of this code, it seems this whole method could be simplified
if the nesting of the loops were reversed (and then LookupDescriptorFromId()
could be re-scoped to a LookupDeviceInfo() that returns a
VideoCaptureDeviceInfo*). Meaning:

  std::vector<VideoCaptureDeviceInfo> new_devices_info_cache;
  new_devices_info_cache.reserve(descriptors->size());
  for (const auto& descriptor : *descriptors) {
    if (auto* cached_info = LookupDeviceInfo(descriptor->device_id)) {
      new_devices_info_cache.push_back(*cached_info);
    } else {
      // Query for supported formats in order to create the entry.
      VideoCaptureDeviceInfo device_info(descriptor);
      ...GetSupportedFromats(), Consolidate() here...
      new_devices_info_cache.push_back(device_info);
    }
  }

  devices_info_cache_.swap(new_devices_info_cache);
  result_callback.Run(devices_info_cache_);

[chfremer, 2017/03/23 00:34:13]

Thanks. That makes things much easier to read.
Done.

+    for (auto it = descriptors->begin(); it != descriptors->end(); ++it) {
+      if (device_info.descriptor.device_id == it->device_id) {
+        new_devices_info_cache.push_back(device_info);
+        descriptors->erase(it);
+        break;
+      }
+    }
+  }
+
+  // For the devices for which we did not have entries in |devices_info_cache_|,
+  // we need to query for supported formats in order to create the entries.
+  for (const auto& it : *descriptors) {
+    VideoCaptureDeviceInfo device_info(it);
+    factory_->GetSupportedFormats(it, &device_info.supported_formats);
+    ConsolidateCaptureFormats(&device_info.supported_formats);
+    new_devices_info_cache.push_back(device_info);
+  }
+
+  devices_info_cache_ = new_devices_info_cache;

[miu, 2017/03/22 22:51:23]

nit: devices_info_cache_.swap(new_devices_info_cache);

[chfremer, 2017/03/23 00:34:13]

Done.

+  result_callback.Run(new_devices_info_cache);

[miu, 2017/03/22 22:51:23]

s/new_devices_info_cache/devices_info_cache_/ (because of swap() call; see
previous comment)

[chfremer, 2017/03/23 00:34:13]

Done.

+}
+
+bool VideoCaptureSystem::LookupDescriptorFromId(

[miu, 2017/03/22 22:51:23]

Along the lines of improvements made on prior CLs, can we change this to:

  const media::VideoCaptureDeviceDescriptor*
VideoCaptureSystem::LookupDescriptor(const std::string& device_id) const {
    auto iter = std::find_if(...);
    return iter == devices_info_cache_.end() ? nullptr : &(iter->descriptor);
  }

[chfremer, 2017/03/23 00:34:13]

Done.

+    const std::string& device_id,
+    media::VideoCaptureDeviceDescriptor* descriptor) {
+  auto iter = std::find_if(
+      devices_info_cache_.begin(), devices_info_cache_.end(),
+      [&device_id](const media::VideoCaptureDeviceInfo& device_info) {
+        return device_info.descriptor.device_id == device_id;
+      });
+  if (iter == devices_info_cache_.end())
+    return false;
+  *descriptor = iter->descriptor;
+  return true;
+}
+
+}  // namespace media