ImageCapture: separate fillLightMode, redEyeReduction and Torch

ImageCapture: separate fillLightMode, redEyeReduction and Torch


Implementation catch-up to some idl changes (already landed):

- RedEyeReduction capability-reading-wise is changed from being a 
  boolean to an enum ("never", "always", "controllable")

- FillLightMode enum capability-reading-wise is changed to 
  returning an array of supported capabilities (vs. on ToT
  where the _actual_ setting that was returned). 
  It also loses the enum entries "none" and "torch": "torch"
  is separated, and the condition of "no fill light mode
  supported" is indicated via an empty supported mode array.

- MediaTrackCapabilities/ConstraintSet's |torch| is wired all
 the way down to the implementations.


Here and there I reordered the capabilities/settings 
manipulation to follow the idl definitions. 


BUG=700607
TEST=Update LayoutTests, capture_unittests and manual 
testing using e.g. 
https://rawgit.com/Miguelao/demos/master/imagecapture.html

Review-Url: https://codereview.chromium.org/2787933002
Cr-Commit-Position: refs/heads/master@{#461293}
Committed: https://chromium.googlesource.com/chromium/src/+/ca371a65982c6c43fc81dbb997271ed2aab6c9da

[mcasas, 2017-03-31 16:18:07]

Description was changed from

==========
ImageCapture: separate fillLightMode, redEyeReduction and Torch

wip

Adapted redEyeReduction, fillLightMode and torch; updated Fake, V4L2 impls and
LayoutTests

BUG=701849
==========

to

==========
ImageCapture: separate fillLightMode, redEyeReduction and Torch


Implementation catch-up to some idl changes (already landed):

- RedEyeReduction capability-reading-wise is changed from being a 
  boolean to an enum ("never", "always", "controllable")

- FillLightMode enum capability-reading-wise is changed to 
  returning an array of supported capabilities (vs. on ToT
  where the _actual_ setting that was returned). 
  It also loses the enum entries "none" and "torch": "torch"
  is separated, and the condition of "no fill light mode
  supported" is indicated via an empty supported mode array.

- MediaTrackCapabilities/ConstraintSet's |torch| is wired all
 the way down to the implementations.


Here and there I reordered the capabilities/settings 
manipulation to follow the idl definitions. 


BUG=701849
TEST=Update LayoutTests, capture_unittests and manual 
testing using e.g. 
https://rawgit.com/Miguelao/demos/master/imagecapture.html
==========

[mcasas, 2017-03-31 16:18:42]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-03-31 16:18:49]

Patchset #1 (id:20001) has been deleted

[mcasas, 2017-03-31 16:39:06]

Patchset #1 (id:40001) has been deleted

[mcasas, 2017-03-31 17:16:10]

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

[mcasas, 2017-03-31 17:16:11]

reillyg@ PTAL

tsepez@ RS plz mojom changes

[Tom Sepez, 2017-03-31 17:32:54]

mojom LGTM

[mcasas, 2017-03-31 18:04:20]

Description was changed from

==========
ImageCapture: separate fillLightMode, redEyeReduction and Torch


Implementation catch-up to some idl changes (already landed):

- RedEyeReduction capability-reading-wise is changed from being a 
  boolean to an enum ("never", "always", "controllable")

- FillLightMode enum capability-reading-wise is changed to 
  returning an array of supported capabilities (vs. on ToT
  where the _actual_ setting that was returned). 
  It also loses the enum entries "none" and "torch": "torch"
  is separated, and the condition of "no fill light mode
  supported" is indicated via an empty supported mode array.

- MediaTrackCapabilities/ConstraintSet's |torch| is wired all
 the way down to the implementations.


Here and there I reordered the capabilities/settings 
manipulation to follow the idl definitions. 


BUG=701849
TEST=Update LayoutTests, capture_unittests and manual 
testing using e.g. 
https://rawgit.com/Miguelao/demos/master/imagecapture.html
==========

to

==========
ImageCapture: separate fillLightMode, redEyeReduction and Torch


Implementation catch-up to some idl changes (already landed):

- RedEyeReduction capability-reading-wise is changed from being a 
  boolean to an enum ("never", "always", "controllable")

- FillLightMode enum capability-reading-wise is changed to 
  returning an array of supported capabilities (vs. on ToT
  where the _actual_ setting that was returned). 
  It also loses the enum entries "none" and "torch": "torch"
  is separated, and the condition of "no fill light mode
  supported" is indicated via an empty supported mode array.

- MediaTrackCapabilities/ConstraintSet's |torch| is wired all
 the way down to the implementations.


Here and there I reordered the capabilities/settings 
manipulation to follow the idl definitions. 


BUG=700607
TEST=Update LayoutTests, capture_unittests and manual 
testing using e.g. 
https://rawgit.com/Miguelao/demos/master/imagecapture.html
==========

[Reilly Grant (use Gerrit), 2017-03-31 20:13:09]

lgtm with nits

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

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:563:
builder.setFillLightModes(modesAsIntArray);
builder.setFillLightModes(modes.toArray(new int[modes.size()]));

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
File
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java:804:
builder.setRedEyeReduction(true);
Can you add a comment explaining why these are now unconditionally set to true?
Previously the torch capability was set based on
CameraMetadata.FLASH_MODE_TORCH.

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
File media/capture/video/android/photo_capabilities.cc (right):

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/photo_capabilities.cc:164: env, jni_modes.obj(),
reinterpret_cast<std::vector<int>*>(&modes));
base::android::ScopedJavaLocalRef<jintArray> jni_modes =
    Java_PhotoCapabilities_getFillLightModes(env, object_);
if (jni_modes.obj()) {
  base::android::JavaIntArrayToIntVector(
      env, jni_modes.obj(), reinterpret_cast<std::vector<int>*>(&modes));
}
return modes;

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

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/video_capture_device_android.cc:79: return
mojom::FillLightMode::OFF;
NOTREACHED();
return mojom::FillLightMode::OFF;

https://codereview.chromium.org/2787933002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2787933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:50: return
MeteringMode::NONE;
if (blinkMode == "none")
  return MeteringMode::NONE;
NOTREACHED();
return MeteringMode::NONE;

[mcasas, 2017-03-31 20:57:30]

Patchset #3 (id:90001) has been deleted

[mcasas, 2017-03-31 20:58:19]

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

[commit-bot: I haz the power, 2017-03-31 20:58:59]

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

[mcasas, 2017-03-31 21:14:47]

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

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera.java:563:
builder.setFillLightModes(modesAsIntArray);
On 2017/03/31 20:13:09, Reilly Grant wrote:
> builder.setFillLightModes(modes.toArray(new int[modes.size()]));

I wish! toArray() would only on ArrayList<Integer>,
there is no direct correspondence between Integer
and int in java-land, and we need int[] for passing
through jni. We could keep an ArrayList<Integer> in
PhotoCapabilities.java, but then we'd need a 
translation like this one over there.

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
File
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java:804:
builder.setRedEyeReduction(true);
On 2017/03/31 20:13:09, Reilly Grant wrote:
> Can you add a comment explaining why these are now unconditionally set to
true?
> Previously the torch capability was set based on
> CameraMetadata.FLASH_MODE_TORCH.

Previously we were retrieving the status of the control,
confusingly enough, whereas now we retrieve the capabilities.
Also, there seems to be no way using Camera2 to retrieve
the capacity for red eye reduction nor for torch mode;
but at this code point we know there's a flash
unit, so it's relatively safe to assume that they
are available. Added a comment explaining this.

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
File media/capture/video/android/photo_capabilities.cc (right):

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/photo_capabilities.cc:164: env, jni_modes.obj(),
reinterpret_cast<std::vector<int>*>(&modes));
On 2017/03/31 20:13:09, Reilly Grant wrote:
> base::android::ScopedJavaLocalRef<jintArray> jni_modes =
>     Java_PhotoCapabilities_getFillLightModes(env, object_);
> if (jni_modes.obj()) {
>   base::android::JavaIntArrayToIntVector(
>       env, jni_modes.obj(), reinterpret_cast<std::vector<int>*>(&modes));
> }
> return modes;

Done.

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

https://codereview.chromium.org/2787933002/diff/80001/media/capture/video/and...
media/capture/video/android/video_capture_device_android.cc:79: return
mojom::FillLightMode::OFF;
On 2017/03/31 20:13:09, Reilly Grant wrote:
> NOTREACHED();
> return mojom::FillLightMode::OFF;

Done.

https://codereview.chromium.org/2787933002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp (right):

https://codereview.chromium.org/2787933002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp:50: return
MeteringMode::NONE;
On 2017/03/31 20:13:09, Reilly Grant wrote:
> if (blinkMode == "none")
>   return MeteringMode::NONE;
> NOTREACHED();
> return MeteringMode::NONE;

Right, done.

[commit-bot: I haz the power, 2017-03-31 22:37:27]

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

[commit-bot: I haz the power, 2017-03-31 22:37:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mcasas, 2017-03-31 22:58:05]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2017-03-31 22:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2787933002/#ps130001
(title: " ")

[commit-bot: I haz the power, 2017-03-31 22:59:02]

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, 2017-04-01 00:57:42]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1491001085007550,
"parent_rev": "b24a207b8d1e25b9cfa4ead338809d3decb6a08c", "commit_rev":
"ca371a65982c6c43fc81dbb997271ed2aab6c9da"}

[commit-bot: I haz the power, 2017-04-01 00:58:32]

Description was changed from

==========
ImageCapture: separate fillLightMode, redEyeReduction and Torch


Implementation catch-up to some idl changes (already landed):

- RedEyeReduction capability-reading-wise is changed from being a 
  boolean to an enum ("never", "always", "controllable")

- FillLightMode enum capability-reading-wise is changed to 
  returning an array of supported capabilities (vs. on ToT
  where the _actual_ setting that was returned). 
  It also loses the enum entries "none" and "torch": "torch"
  is separated, and the condition of "no fill light mode
  supported" is indicated via an empty supported mode array.

- MediaTrackCapabilities/ConstraintSet's |torch| is wired all
 the way down to the implementations.


Here and there I reordered the capabilities/settings 
manipulation to follow the idl definitions. 


BUG=700607
TEST=Update LayoutTests, capture_unittests and manual 
testing using e.g. 
https://rawgit.com/Miguelao/demos/master/imagecapture.html
==========

to

==========
ImageCapture: separate fillLightMode, redEyeReduction and Torch


Implementation catch-up to some idl changes (already landed):

- RedEyeReduction capability-reading-wise is changed from being a 
  boolean to an enum ("never", "always", "controllable")

- FillLightMode enum capability-reading-wise is changed to 
  returning an array of supported capabilities (vs. on ToT
  where the _actual_ setting that was returned). 
  It also loses the enum entries "none" and "torch": "torch"
  is separated, and the condition of "no fill light mode
  supported" is indicated via an empty supported mode array.

- MediaTrackCapabilities/ConstraintSet's |torch| is wired all
 the way down to the implementations.


Here and there I reordered the capabilities/settings 
manipulation to follow the idl definitions. 


BUG=700607
TEST=Update LayoutTests, capture_unittests and manual 
testing using e.g. 
https://rawgit.com/Miguelao/demos/master/imagecapture.html

Review-Url: https://codereview.chromium.org/2787933002
Cr-Commit-Position: refs/heads/master@{#461293}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca371a65982c6c43fc81dbb99727...
==========

[commit-bot: I haz the power, 2017-04-01 00:58:33]

Committed patchset #4 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/ca371a65982c6c43fc81dbb99727...