Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 700607 Migrate existing ImageCapture implementation to use Constrainable Pattern
Starred by 2 users Project Member Reported by mcasas@chromium.org, Mar 11 Back to list
Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature

Blocked on:
issue 543997

Blocking:
issue 518807



Sign in to add a comment
The use of Constrainable Pattern API in the Spec landed in [1].


[1] https://github.com/w3c/mediacapture-image/pull/150

 
Project Member Comment 1 by bugdroid1@chromium.org, Mar 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e973fb670fcbdd7dc263f037d6d79fa31abb6553

commit e973fb670fcbdd7dc263f037d6d79fa31abb6553
Author: mcasas <mcasas@chromium.org>
Date: Mon Mar 13 20:44:28 2017

Image Capture: MediaTrackSupportedConstraints extension

Additions to MediaTrackSupportedConstraints and a LayoutTest

BUG=700607

Review-Url: https://codereview.chromium.org/2741803007
Cr-Commit-Position: refs/heads/master@{#456473}

[add] https://crrev.com/e973fb670fcbdd7dc263f037d6d79fa31abb6553/third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-MediaTrackSupportedConstraints.html
[modify] https://crrev.com/e973fb670fcbdd7dc263f037d6d79fa31abb6553/third_party/WebKit/Source/modules/mediastream/MediaTrackSupportedConstraints.idl

Project Member Comment 2 by bugdroid1@chromium.org, Mar 15
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30a67395e8cfccfd5954b9244440a7906d065563

commit 30a67395e8cfccfd5954b9244440a7906d065563
Author: mcasas <mcasas@chromium.org>
Date: Wed Mar 15 18:50:11 2017

Image Capture: MediaStreamTrack::getCapabilities()

This CL adds MediaTrackCapabilities.idl and
MediaStreamTrack::getCapabilities(). The latter function is wired
to grab the capabilities from image capture, since these are
the only ones available ATM.  These capabilities should only
be collected for Video tracks from devices (i.e. not for
tab/window/cast/remote etc).

MediaStreamTrack.getCapabilities() is synchronous, whereas
image_capture mojom::getPhotoCapabities() is not -- the former
probably doesn't make a lot of sense Spec-wise but, regardless,
I don't think is a good idea to make the Web thread wait for
these capabilities to be collected, so what this CL does is
to trigger such collection upon MediaStreamTrack ctor, without
waiting for this process to be finished. (IOW I tried alternatives
and were much worse).

Added LayoutTests for starters.

This bug is part of a transition from
 ImageCapture.getPhotoCapabilities() to MediaStreamTrack.getCapabilities()
but, since Image Capture is already in Origin Trials, I'm
planning on landing the new infra in parallel, then send a PSA
and switch over (that's why there's a few TODOs) in the code.

BUG=700607

Incidentally, it improves the state of
external/wpt/mediacapture-streams/MediaStreamTrack-init.https.html
by allowing the test-for-getCapabilities() to PASS.

Review-Url: https://codereview.chromium.org/2747573002
Cr-Commit-Position: refs/heads/master@{#457150}

[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/LayoutTests/external/wpt/mediacapture-streams/MediaStreamTrack-init.https-expected.txt
[add] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getCapabilities.html
[add] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-getCapabilities.html
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/imagecapture/MediaSettingsRange.h
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/mediastream/DEPS
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.h
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl
[add] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/mediastream/MediaTrackCapabilities.idl
[modify] https://crrev.com/30a67395e8cfccfd5954b9244440a7906d065563/third_party/WebKit/Source/modules/modules_idl_files.gni

https://codereview.chromium.org/2753513007/ was landed against this
bug as well -- it didn't show up here :?
Cc: haraken@chromium.org reil...@chromium.org
Before it gets lost, I'd like to bring here haraken@s comment in [1]
about the constructions like [2]

  caps->setIso(MediaSettingsRange::create(std::move(capabilities->iso)));

|caps| is a PhotoCapabilities (idl interface [3]) that has a
bunch of MediaSettingsRanges (another idl interface [4]) inside,
so the creation of a garbage-collected MSRange to be plugged
in a garbage-collected PhotoCapabilities is needed and cannot
be avoided either by mojo-mapping MediaSettingsRange to a similar
move-only mojo:: struct called Range [5] or by mojo-mapping the
latter to another struct inside MediaSettingsRange.  (I've been 
trying to do that as well).

I'm afraid the legit concern of "we're creating too many
garbage collected objects" in the PhotoCapabilities-MediaSettingsRange
is due to the idl definition.  Any ideas?




[1] https://codereview.chromium.org/2747573002#msg20
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp?q=imagecapture.cpp&sq=package:chromium&dr&l=324
[3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.idl?sq=package:chromium&dr
[4] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagecapture/MediaSettingsRange.idl?sq=package:chromium&dr
[5] https://cs.chromium.org/chromium/src/media/capture/mojo/image_capture.mojom?sq=package:chromium&dr&q=image_capture&l=10
Can we do something like this?

class MediaSettingsRangeData {  // This is just a struct. You can pass it around.
  STATIC_ONLY();
  double m_max;
  ...;
};

class MediaSettingsRange : public GarbageCollected<MediaSettingsRange> {
  MediaSettingsRangeData m_data;
};


Project Member Comment 6 by bugdroid1@chromium.org, Mar 17
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef23b14712be35baab5b30b0176808911f9bb638

commit ef23b14712be35baab5b30b0176808911f9bb638
Author: mcasas <mcasas@chromium.org>
Date: Fri Mar 17 15:19:44 2017

Image Capture: MediaStreamTrack::applyConstraints()

This CL adds MediaStreamTrack.applyConstraints()
method behind a flag (that is implied if ImageCapture
is on).  The method essentially derives to
ImageCapture::setMediaTrackConstraints(); this one
prepares a mojo::PhotoSettings and applies it via
mojo interface.

Only simple Constrains are supported now, i.e.,
{zoom : 3.14} is supported, but {zoom : { ideal : 100}}
or {zoom : { exact : 27}} are not (TBD later).

LayoutTests are added where appropriate.

Also a cleanup: methods that return Promises are not
supposed to throw exceptions; so I removed an unused
ExceptionState from some ImageCapture methods.

applyConstraints() when called without the optional
argument |constraints|, and is supposed to do... nothing?
Filed Spec bug:
https://github.com/w3c/mediacapture-main/issues/438

BUG=700607

Review-Url: https://codereview.chromium.org/2757673005
Cr-Commit-Position: refs/heads/master@{#457771}

[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/LayoutTests/external/wpt/mediacapture-streams/MediaStreamTrack-init.https-expected.txt
[add] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-applyConstraints.html
[add] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints.html
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.h
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.idl
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
[modify] https://crrev.com/ef23b14712be35baab5b30b0176808911f9bb638/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5

Project Member Comment 7 by bugdroid1@chromium.org, Mar 20 (6 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/726f6720d94c103683515f9887082a4c9376d133

commit 726f6720d94c103683515f9887082a4c9376d133
Author: mcasas <mcasas@chromium.org>
Date: Mon Mar 20 21:10:02 2017

Image Capture: cache constraints and append them to MediaStreamTrack.getConstraints()

This CL teaches Image Capture to cache the constraints passed via
applyConstraints() and wires MediaStreamTrack::getConstraints() to
append those upon request.

Note that constraints are accumulative, i.e. two calls to
applyConstraints() with different constraints will essentially
be understood by the platform as a single applyConstraints()
call with the constraint dictionaries merged.

(Also moved MediaStreamTrack::getConstraints() to follow the
declaration order in MediaStreamTrack.h).

Added LayoutTests.

BUG=700607

Review-Url: https://codereview.chromium.org/2758063002
Cr-Commit-Position: refs/heads/master@{#458183}

[add] https://crrev.com/726f6720d94c103683515f9887082a4c9376d133/third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html
[modify] https://crrev.com/726f6720d94c103683515f9887082a4c9376d133/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/726f6720d94c103683515f9887082a4c9376d133/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
[modify] https://crrev.com/726f6720d94c103683515f9887082a4c9376d133/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp

Project Member Comment 9 by bugdroid1@chromium.org, Mar 21 (5 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/863711a1f0b27b5aa30ed18af6932eae19ccf5fc

commit 863711a1f0b27b5aa30ed18af6932eae19ccf5fc
Author: mcasas <mcasas@chromium.org>
Date: Tue Mar 21 18:56:41 2017

Image Capture: wire getSettings() from MediaStreamTrack

This CL wires the fourth and last Constrainable Pattern
method, MediaStreamTrack.getSettings(), appending
Image-Capture specific Settings to what is returned
now.

ImageCapture::getMediaTrackSettings simply extends the
parameter |settings| with whatever the current |m_capabilities|
holds.  Perhaps confusingly, ImageCapture::m_capabilities
includes _both_ working ranges and current values so, in a way,
it encompasses the capabilities and the settings; this is
due to the previous API (and mojo) design, and will be
untangled in subsequent CLs.

Also, this CL forces an update of |m_capabilities| when
configuring or reading the device.

A few LayoutTests are added, including one where the
applied constraints are retrieved with getSettings and
compared (the mojo mock is extended to latch the former
blindly).

BUG=700607

Review-Url: https://codereview.chromium.org/2766473002
Cr-Commit-Position: refs/heads/master@{#458495}

[add] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getSettings.html
[add] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints-getSettings.html
[add] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-getSettings.html
[modify] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/LayoutTests/imagecapture/resources/mock-imagecapture.js
[modify] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
[modify] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp
[modify] https://crrev.com/863711a1f0b27b5aa30ed18af6932eae19ccf5fc/third_party/WebKit/Source/modules/mediastream/MediaTrackSettings.idl

Project Member Comment 10 by bugdroid1@chromium.org, Mar 24 (2 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f9e2de323e9265aee33ba9683ba98b09fec6769

commit 7f9e2de323e9265aee33ba9683ba98b09fec6769
Author: mcasas <mcasas@chromium.org>
Date: Fri Mar 24 05:47:27 2017

Image Capture: prune Photo{Capabilities/Settings} and add MediaTrackConstraintSet.pointsOfInterest

- This CL removes from PhotoCapabilities.idl the fields that are
not there anymore according to the spec [1], leaving just four
of them.  The corresponding code and LayoutTests are updated.
Same for PhotoSettings.idl [2].

- This CL also adds ConstrainPoint2D data type [3] and uses it
for MediaTrackConstraintSet.pointsOfInterest, wiring it down
to the mojo interface and in the mock.

- This is the one and only important change visible to the users.
(There is an Origin Trial going on for Image Capture in
M56-57-58, but these changes will land in 59.x.y.z, so shouldn't
affect them, in any case an update to blink-dev@ is being
brewed and there's a migration guide:
  https://tinyurl.com/image-capture-migration-guide
)

- No more MediaStreamTrack idl changes are expected after
this CL \o/

TEST= Layouttests etc updated.
BUG=700607

[1] https://w3c.github.io/mediacapture-image/##photocapabilities-section
[2] https://w3c.github.io/mediacapture-image/##photosettings-section
[3] https://w3c.github.io/mediacapture-image/#additional-constrainable-props

Review-Url: https://codereview.chromium.org/2773593004
Cr-Commit-Position: refs/heads/master@{#459367}

[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/fast/imagecapture/ImageCapture-MediaTrackSupportedConstraints.html
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints-getSettings.html
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints.html
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-getCapabilities.html
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/imagecapture/getphotocapabilities.html
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/imagecapture/resources/mock-imagecapture.js
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/imagecapture/setoptions.html
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[add] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/ConstrainPoint2DParameters.idl
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.cpp
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.h
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/PhotoCapabilities.idl
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/imagecapture/PhotoSettings.idl
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/mediastream/MediaTrackConstraintSet.idl
[modify] https://crrev.com/7f9e2de323e9265aee33ba9683ba98b09fec6769/third_party/WebKit/Source/modules/modules_idl_files.gni

Sign in to add a comment