[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

In interface VideoCaptureDeviceFactory, instances of struct VideoCaptureDeviceDescriptor were
used as keys for accessing capture devices. This CL simplifies this key to a simple string.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL1.9.4

BUG=637439, 584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

yzshen@: PTAL
mcasas@: PTAL

Committed: https://crrev.com/f6995b002a218dff4841cd1793f79d4e8a8991ad
Cr-Commit-Position: refs/heads/master@{#432040}

[chfremer, 2016-11-12 00:08:50]

Description was changed from

==========
Presubmit


Cleanup before review


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Merge branch '1.9.3' into DeviceId


Merge branch '1.9.1' into 1.9.3


Use device_id instead of whole descriptor as key to identify capture devices.


fix presubmit


Merge branch '1.9.1' into 1.9.3


* Fix error in conversion code causing test failures * Remove unused field and
forward declaration


Replace manual conversions with Mojo typemappings

BUG=
==========

to

==========
[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

In interface VideoCaptureDeviceFactory, instances of struct
VideoCaptureDeviceDescriptor were 
used as keys for accessing capture devices. This CL simplifies this key to a
simple string.

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.4

BUG=637439, 584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

yzshen@: PTAL
mcasas@: PTAL
==========

[chfremer, 2016-11-12 00:09:48]

chfremer@chromium.org changed reviewers:
+ mcasas@chromium.org, tsepez@chromium.org, yzshen@chromium.org

[chfremer, 2016-11-12 00:09:48]

yzshen@: PTAL
mcasas@: PTAL
tsepez@: Please RS *.mojom

[chfremer, 2016-11-12 00:09:57]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 00:10:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-11-12 00:27:29]

https://codereview.chromium.org/2494033004/diff/1/services/video_capture/devi...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2494033004/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:137:
descriptors_by_id_.clear();
Why do we need to store descriptors_by_id_ as a class member? It seems we always
refresh it whenever we need it, so we could directly use the result from
device_factory_ instead of storing it. Right?

[Tom Sepez, 2016-11-12 00:56:06]

RS mojom.

[Tom Sepez, 2016-11-12 01:02:23]

On 2016/11/12 00:56:06, Tom Sepez wrote:
> RS mojom.

RS LGTM mojom, that is.

[commit-bot: I haz the power, 2016-11-12 01:29:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-12 01:29:28]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-11-14 17:38:05]

yzshen@: PTAL

https://codereview.chromium.org/2494033004/diff/1/services/video_capture/devi...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2494033004/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:137:
descriptors_by_id_.clear();
On 2016/11/12 00:27:29, yzshen1 wrote:
> Why do we need to store descriptors_by_id_ as a class member? It seems we
always
> refresh it whenever we need it, so we could directly use the result from
> device_factory_ instead of storing it. Right?

Excellent observation. Thank you.
Fixed.

[chfremer, 2016-11-14 17:38:24]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 17:39:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-11-14 17:56:21]

https://codereview.chromium.org/2494033004/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2494033004/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:102: auto
descriptor_iter = std::find_if(
Something to consider: it may make sense to extract the code of finding a
specific descriptor, making it part of the device factory API. It seems a waste
of returning a vector and discard most of them immediately.

WDYT?

[chfremer, 2016-11-14 18:17:43]

https://codereview.chromium.org/2494033004/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2494033004/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:102: auto
descriptor_iter = std::find_if(
On 2016/11/14 17:56:21, yzshen1 wrote:
> Something to consider: it may make sense to extract the code of finding a
> specific descriptor, making it part of the device factory API. It seems a
waste
> of returning a vector and discard most of them immediately.
> 
> WDYT?

I completely agree, but I would like to defer that refactoring to a later time.
VideoCaptureDeviceFactory has many platform-dependent implementations. What we
would need is a layer between those platform-specific implementations and the
API exposed to clients, where we can handle the mapping between device ids and
device descriptors.

I am adding crbug.com/665065 to track this.

[yzshen1, 2016-11-14 18:27:11]

lgtm

[chfremer, 2016-11-14 18:29:01]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 18:30:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mcasas, 2016-11-14 19:02:02]

lgtm with a few suggestions/questions

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:42:
std::vector<media::VideoCaptureDeviceDescriptor> descriptors;
media::VideoCaptureDeviceDescriptors still
exists, right? [1]

[1]
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:60: }
nit: no {}

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:107: if
(descriptor_iter == descriptors.end()) {
l.100-l.107 is the same code as l.50-l.58, is it
worth factoring it out somehow...?

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_device_factory.mojom
(right):

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_device_factory.mojom:30:
GetSupportedFormats(string device_id)
As a side comment for the future, we might like
to use UUIDs [1] here to allow for security by obfuscation,
now it's fine to use |device_id| (although technically
is an internal detail, e.g. /dev/video0 etc).

[1]
https://cs.chromium.org/chromium/src/base/guid.h?q=GenerateGUID&sq=package:ch...

[chfremer, 2016-11-14 19:23:29]

Addressed comments

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:42:
std::vector<media::VideoCaptureDeviceDescriptor> descriptors;
On 2016/11/14 19:02:02, mcasas wrote:
> media::VideoCaptureDeviceDescriptors still
> exists, right? [1]
> 
> [1]
>
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...

Done.

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:60: }
On 2016/11/14 19:02:02, mcasas wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:107: if
(descriptor_iter == descriptors.end()) {
On 2016/11/14 19:02:02, mcasas wrote:
> l.100-l.107 is the same code as l.50-l.58, is it
> worth factoring it out somehow...?

It is! Thanks.
Done.

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_device_factory.mojom
(right):

https://codereview.chromium.org/2494033004/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_device_factory.mojom:30:
GetSupportedFormats(string device_id)
On 2016/11/14 19:02:02, mcasas wrote:
> As a side comment for the future, we might like
> to use UUIDs [1] here to allow for security by obfuscation,
> now it's fine to use |device_id| (although technically
> is an internal detail, e.g. /dev/video0 etc).
> 
> [1]
>
https://cs.chromium.org/chromium/src/base/guid.h?q=GenerateGUID&sq=package:ch...

Acknowledged.

[chfremer, 2016-11-14 19:23:43]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 19:25:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 20:45:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 20:45:29]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-11-14 20:49:39]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-11-14 20:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
mcasas@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2494033004/#ps50001
(title: "mcasas comments")

[commit-bot: I haz the power, 2016-11-14 20:50:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 22:38:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 22:38:55]

Failed to apply patch for services/video_capture/mock_device_test.cc:
While running git apply --index -p1;
  error: patch failed: services/video_capture/mock_device_test.cc:9
  error: services/video_capture/mock_device_test.cc: patch does not apply

Patch:       services/video_capture/mock_device_test.cc
Index: services/video_capture/mock_device_test.cc
diff --git a/services/video_capture/mock_device_test.cc
b/services/video_capture/mock_device_test.cc
index
a6789af88636f7f1b1bffe11c3941799dfd6ed57..d7e523a0ecbe1932ffc632e774a81d2e0d8d867c
100644
--- a/services/video_capture/mock_device_test.cc
+++ b/services/video_capture/mock_device_test.cc
@@ -9,8 +9,7 @@ namespace video_capture {
 MockDeviceTest::MockDeviceTest()
     : service_manager::test::ServiceTest("exe:video_capture_unittests") {}
 
-MockDeviceTest::~MockDeviceTest() =
-    default;
+MockDeviceTest::~MockDeviceTest() = default;
 
 void MockDeviceTest::SetUp() {
   ServiceTest::SetUp();
@@ -27,7 +26,7 @@ void MockDeviceTest::SetUp() {
 
   // Obtain the mock device from the factory
   factory_->CreateDeviceProxy(
-      mock_descriptor, mojo::GetProxy(&device_proxy_),
+      mock_descriptor.device_id, mojo::GetProxy(&device_proxy_),
       base::Bind([](mojom::DeviceAccessResultCode result_code) {}));
 
   requested_settings_.format.frame_size = gfx::Size(800, 600);

[chfremer, 2016-11-14 22:58:47]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-11-14 22:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
mcasas@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2494033004/#ps70001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-14 22:59:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 01:01:43]

Description was changed from

==========
[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

In interface VideoCaptureDeviceFactory, instances of struct
VideoCaptureDeviceDescriptor were 
used as keys for accessing capture devices. This CL simplifies this key to a
simple string.

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.4

BUG=637439, 584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

yzshen@: PTAL
mcasas@: PTAL
==========

to

==========
[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

In interface VideoCaptureDeviceFactory, instances of struct
VideoCaptureDeviceDescriptor were 
used as keys for accessing capture devices. This CL simplifies this key to a
simple string.

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.4

BUG=637439, 584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

yzshen@: PTAL
mcasas@: PTAL
==========

[commit-bot: I haz the power, 2016-11-15 01:01:44]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-11-15 01:15:02]

Description was changed from

==========
[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

In interface VideoCaptureDeviceFactory, instances of struct
VideoCaptureDeviceDescriptor were 
used as keys for accessing capture devices. This CL simplifies this key to a
simple string.

This CL is part of the Mojo Video Capture work. For the bigger picture, 
see [1] CL1.9.4

BUG=637439, 584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

yzshen@: PTAL
mcasas@: PTAL
==========

to

==========
[Mojo Video Capture] Use string keys instead of VideoCaptureDeviceDescriptor

In interface VideoCaptureDeviceFactory, instances of struct
VideoCaptureDeviceDescriptor were
used as keys for accessing capture devices. This CL simplifies this key to a
simple string.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL1.9.4

BUG=637439, 584797
TEST=video_capture_unittests, content_unittests, capture_unittests

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

yzshen@: PTAL
mcasas@: PTAL

Committed: https://crrev.com/f6995b002a218dff4841cd1793f79d4e8a8991ad
Cr-Commit-Position: refs/heads/master@{#432040}
==========

[commit-bot: I haz the power, 2016-11-15 01:15:04]

Patchset 5 (id:??) landed as
https://crrev.com/f6995b002a218dff4841cd1793f79d4e8a8991ad
Cr-Commit-Position: refs/heads/master@{#432040}