ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings' width and height

ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings' width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Android's "old" and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
Committed: https://crrev.com/5c7ce5d4d50500a169efaf836510703d1fd11b30
Cr-Commit-Position: refs/heads/master@{#407298}

[mcasas, 2016-07-20 01:29:35]

Description was changed from

==========
[WIP] ImageCapture: wire |iso|, |width| and |height|

BUG=518807
==========

to

==========
ImageCapture: wire PhotoCapabilitiess ISO, width, height and PhotoSettings'
width and height

BUG=518807
==========

[mcasas, 2016-07-20 01:29:47]

Patchset #2 (id:20001) has been deleted

[mcasas, 2016-07-20 01:29:52]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-07-20 01:35:42]

Description was changed from

==========
ImageCapture: wire PhotoCapabilitiess ISO, width, height and PhotoSettings'
width and height

BUG=518807
==========

to

==========
ImageCapture: wire PhotoCapabilitiess ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:
- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Anddroid's "old' and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.


BUG=518807
==========

[mcasas, 2016-07-20 01:41:54]

Description was changed from

==========
ImageCapture: wire PhotoCapabilitiess ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:
- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Anddroid's "old' and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.


BUG=518807
==========

to

==========
ImageCapture: wire PhotoCapabilitiess ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Anddroid's "old' and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
==========

[mcasas, 2016-07-20 01:47:24]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-07-20 02:28:15]

Patchset #1 (id:60001) has been deleted

[mcasas, 2016-07-20 02:28:59]

mcasas@chromium.org changed reviewers:
+ dcheng@chromium.org, reillyg@chromium.org

[mcasas, 2016-07-20 02:29:00]

dcheng@: image_capture.mojom and its ramifications plz

reillyg@ PTAL

[mcasas, 2016-07-20 02:30:22]

Description was changed from

==========
ImageCapture: wire PhotoCapabilitiess ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Anddroid's "old' and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
==========

to

==========
ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Anddroid's "old' and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
==========

[mcasas, 2016-07-20 05:18:01]

Patchset #1 (id:80001) has been deleted

[Reilly Grant (use Gerrit), 2016-07-20 14:54:21]

Description was changed from

==========
ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Anddroid's "old' and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
==========

to

==========
ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Android's "old" and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
==========

[Reilly Grant (use Gerrit), 2016-07-20 15:03:09]

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
File
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java
(right):

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:385:
+ ((height > 0) ? Math.abs(size.height - height) : 0);
This seems an odd way to find the closest supported size. If you really want to
find the closest I would compare area instead of perimeter length but I think
you probably actually want to find either the smallest supported size that the
requested rectangle fits within or the largest supported size that fits within
the requested rectangle.

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
File media/capture/video/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
media/capture/video/android/video_capture_device_android.cc:202: // |width| and
/or|height| are kept for the next TakePhoto()s.
and/or |height|

[mcasas, 2016-07-20 23:35:25]

PTAL

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
File
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java
(right):

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:385:
+ ((height > 0) ? Math.abs(size.height - height) : 0);
On 2016/07/20 15:03:09, Reilly Grant wrote:
> This seems an odd way to find the closest supported size. If you really want
to
> find the closest I would compare area instead of perimeter length but I think
> you probably actually want to find either the smallest supported size that the
> requested rectangle fits within or the largest supported size that fits within
> the requested rectangle.

I followed the pattern in VCCamera(2).java [1,2],
and of other VCDevice implementations [3]. I 
think individual comparisons might be better to avoid
selecting by mistake e.g. portrait modes when
landscapes are desired and viceversa. Although 
it is true that there is no warranty that the selected
resolution might be smaller than the requested,
which might be... wrong... maybe...?

I can make a bug and try to clean up all the 
(android) locations where this method is used,
WDYT?

[1]
https://cs.chromium.org/chromium/src/media/capture/video/android/java/src/org...
[2]
https://cs.chromium.org/chromium/src/media/capture/video/android/java/src/org...
[3]
https://cs.chromium.org/chromium/src/media/capture/video/win/capability_list_...

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
File media/capture/video/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/2164473002/diff/90001/media/capture/video/and...
media/capture/video/android/video_capture_device_android.cc:202: // |width| and
/or|height| are kept for the next TakePhoto()s.
On 2016/07/20 15:03:09, Reilly Grant wrote:
> and/or |height|

Done.

[Reilly Grant (use Gerrit), 2016-07-21 21:10:10]

lgtm

[mcasas, 2016-07-21 23:08:32]

mcasas@chromium.org changed reviewers:
+ foolip@chromium.org

[mcasas, 2016-07-21 23:08:33]

foolip@ PTAL at WebKit files' changes.

[foolip, 2016-07-22 01:23:56]

No third_party/WebKit/Source/modules/imagecapture/OWNERS? Maybe emircan@?

I can do rubberstamping of trivial things, but this code needs someone who feels
ownership for it I think for the juicy bits.

[foolip, 2016-07-22 01:33:20]

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.idl
(right):

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.idl:16:
readonly attribute MediaSettingsRange iso;
Filed https://github.com/w3c/mediacapture-image/issues/29, if it's not possible
to copy-paste the spec's IDL then TODOs and spec issues are in order :)

[foolip, 2016-07-22 01:42:34]

lgtm

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/imagecapture/setoptions.html (right):

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imagecapture/setoptions.html:35:
assert_equals(true, theMock.options().has_zoom, 'has_zoom must be true');
assert_true instead?

[mcasas, 2016-07-22 02:00:04]

dcheng@ ping

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/imagecapture/setoptions.html (right):

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imagecapture/setoptions.html:35:
assert_equals(true, theMock.options().has_zoom, 'has_zoom must be true');
On 2016/07/22 01:42:34, foolip wrote:
> assert_true instead?

Done.

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.idl
(right):

https://codereview.chromium.org/2164473002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.idl:16:
readonly attribute MediaSettingsRange iso;
On 2016/07/22 01:33:20, foolip wrote:
> Filed https://github.com/w3c/mediacapture-image/issues/29, if it's not
possible
> to copy-paste the spec's IDL then TODOs and spec issues are in order :)

Done and added another TODO+Issue.

[dcheng, 2016-07-22 04:45:29]

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
File media/capture/video/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
media/capture/video/android/video_capture_device_android.cc:205:
next_photo_resolution_.set_width(settings->width);
What happens if settings->width overflows int?

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:208:
MediaSettingsRange* width = MediaSettingsRange::create(capabilities->width->max,
capabilities->width->min, capabilities->width->current);
Any chance of typemapping this soon? =)

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp
(right):

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp:15:
MediaSettingsRange* PhotoCapabilities::iso() const { return m_iso; }
FWIW, simple setters and getters are usually inlined into the header in Blink.

[mcasas, 2016-07-22 16:46:32]

dcheng@ PTAL

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
File media/capture/video/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
media/capture/video/android/video_capture_device_android.cc:205:
next_photo_resolution_.set_width(settings->width);
On 2016/07/22 04:45:29, dcheng wrote:
> What happens if settings->width overflows int?

gfx::Size::set_{width,height}() clamps negative numbers
to 0 [1] which will essentially result in thoem being ignored.


[1]
https://cs.chromium.org/chromium/src/ui/gfx/geometry/size.h?q=gfx::size&sq=pa...

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:208:
MediaSettingsRange* width = MediaSettingsRange::create(capabilities->width->max,
capabilities->width->min, capabilities->width->current);
On 2016/07/22 04:45:29, dcheng wrote:
> Any chance of typemapping this soon? =)

Totally! I've been dealing with other mojo-related
cleanups -- i.e. https://crrev.com/2166713002/ (closed)
and https://crrev.com/2167313002 (under review) --
but will get to this one soon..

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp
(right):

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp:15:
MediaSettingsRange* PhotoCapabilities::iso() const { return m_iso; }
On 2016/07/22 04:45:29, dcheng wrote:
> FWIW, simple setters and getters are usually inlined into the header in Blink.

I think peter@ mentioned a while ago that I should
try and keep all accesors/mutators together, and,
given the focusMode(), they're all pushed here. WDYT?

[dcheng, 2016-07-22 18:07:24]

lgtm with nits addressed

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
File media/capture/video/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
media/capture/video/android/video_capture_device_android.cc:205:
next_photo_resolution_.set_width(settings->width);
On 2016/07/22 16:46:32, mcasas wrote:
> On 2016/07/22 04:45:29, dcheng wrote:
> > What happens if settings->width overflows int?
> 
> gfx::Size::set_{width,height}() clamps negative numbers
> to 0 [1] which will essentially result in thoem being ignored.
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/ui/gfx/geometry/size.h?q=gfx::size&sq=pa...

Right, it just ... seems a bit odd.

I suppose we can try to figure out the proper way to address it if we ever
figure out how to turn on warnings for narrowing conversions.

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp
(right):

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp:15:
MediaSettingsRange* PhotoCapabilities::iso() const { return m_iso; }
On 2016/07/22 16:46:32, mcasas wrote:
> On 2016/07/22 04:45:29, dcheng wrote:
> > FWIW, simple setters and getters are usually inlined into the header in
Blink.
> 
> I think peter@ mentioned a while ago that I should
> try and keep all accesors/mutators together, and,
> given the focusMode(), they're all pushed here. WDYT?

The inlining isn't for organization; it's for codegen.

It's not a hard rule, but it is the convention throughout basically the rest of
the codebase. I don't see a reason not to follow it here.

[mcasas, 2016-07-22 21:28:28]

Patchset #4 (id:150001) has been deleted

[mcasas, 2016-07-22 22:07:40]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-07-22 22:07:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, foolip@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2164473002/#ps170001
(title: "dcheng@ comments")

[commit-bot: I haz the power, 2016-07-22 22:08:15]

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

[mcasas, 2016-07-22 23:16:27]

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
File media/capture/video/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/2164473002/diff/130001/media/capture/video/an...
media/capture/video/android/video_capture_device_android.cc:205:
next_photo_resolution_.set_width(settings->width);
On 2016/07/22 18:07:23, dcheng wrote:
> On 2016/07/22 16:46:32, mcasas wrote:
> > On 2016/07/22 04:45:29, dcheng wrote:
> > > What happens if settings->width overflows int?
> > 
> > gfx::Size::set_{width,height}() clamps negative numbers
> > to 0 [1] which will essentially result in thoem being ignored.
> > 
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/geometry/size.h?q=gfx::size&sq=pa...
> 
> Right, it just ... seems a bit odd.
> 
> I suppose we can try to figure out the proper way to address it if we ever
> figure out how to turn on warnings for narrowing conversions.

Using base::saturated_cast<int>() now.

(Note that having huge numbers here is not a
problem, the platform will just fall back to the
largest available resolution).

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp
(right):

https://codereview.chromium.org/2164473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp:15:
MediaSettingsRange* PhotoCapabilities::iso() const { return m_iso; }
On 2016/07/22 18:07:23, dcheng wrote:
> On 2016/07/22 16:46:32, mcasas wrote:
> > On 2016/07/22 04:45:29, dcheng wrote:
> > > FWIW, simple setters and getters are usually inlined into the header in
> Blink.
> > 
> > I think peter@ mentioned a while ago that I should
> > try and keep all accesors/mutators together, and,
> > given the focusMode(), they're all pushed here. WDYT?
> 
> The inlining isn't for organization; it's for codegen.
> 
> It's not a hard rule, but it is the convention throughout basically the rest
of
> the codebase. I don't see a reason not to follow it here.

Done.

[commit-bot: I haz the power, 2016-07-22 23:33:44]

Description was changed from

==========
ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Android's "old" and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807
==========

to

==========
ImageCapture: wire PhotoCapabilities' ISO, width, height and PhotoSettings'
width and height

This CL continues implementing ImageCapture:

- adds |iso|, |imageHeight| and |imageWidth| to PhotoCapabilities
 (each is a MediaSettingsRange).
- adds |imageHeight| and |imageWidth| to PhotoSettings.
- extends image_capture.mojom to support all the previous new
 fields
- wires Android's "old" and "new" Camera API to provide the
 capabilities and listen to the Settings (seems like the
 "old" does not reliably support ISO).
- extends the FakeVideoCaptureDeviceTest and the LayoutTests
 for these new features.

Note that setOptions() is not synchronous with takePhoto(),
so VideoCaptureDeviceAndroid holds on to the latest, if any,
requested capture resolution.

BUG=518807

Committed: https://crrev.com/5c7ce5d4d50500a169efaf836510703d1fd11b30
Cr-Commit-Position: refs/heads/master@{#407298}
==========

[commit-bot: I haz the power, 2016-07-22 23:33:45]

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

[commit-bot: I haz the power, 2016-07-22 23:34:01]

Failed to apply the patch.