|
|
DescriptionImage 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}
Committed: https://chromium.googlesource.com/chromium/src/+/726f6720d94c103683515f9887082a4c9376d133
Patch Set 1 : #
Total comments: 4
Patch Set 2 : reillyg@s comments #
Total comments: 7
Patch Set 3 : guidou@ TODO()s #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Image Capture: cache passed constraints and append them to MST.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. Added LayoutTests. BUG=700607 ========== to ========== Image Capture: cache passed constraints and append them to MST.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 ==========
Description was changed from ========== Image Capture: cache passed constraints and append them to MST.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 ========== to ========== 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 ==========
Patchset #2 (id:20001) has been deleted
mcasas@chromium.org changed reviewers: + guidou@chromium.org, reillyg@chromium.org
reillyg@ ptal guidou@ ptal at the mediastream/ part plz
https://codereview.chromium.org/2758063002/diff/30001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2758063002/diff/30001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:233: // Add the image capture constraints as another entry to the advanced(). *to advanced https://codereview.chromium.org/2758063002/diff/30001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:251: constraints.setAdvanced(vector); This line should perhaps be in the body of the if above?
Patchset #1 (id:1) has been deleted
ptal https://codereview.chromium.org/2758063002/diff/30001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2758063002/diff/30001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:233: // Add the image capture constraints as another entry to the advanced(). On 2017/03/18 01:17:13, Reilly Grant wrote: > *to advanced Done. https://codereview.chromium.org/2758063002/diff/30001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:251: constraints.setAdvanced(vector); On 2017/03/18 01:17:13, Reilly Grant wrote: > This line should perhaps be in the body of the if above? Ouch yeah - sorry, I must have lost in a rebase. Probably this'll make the LayoutTests fail too.
lgtm
https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html:38: const constraintsIn = { advanced : [ c ]}; what if you use the basic constraint set too? https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:234: if (constraints.hasAdvanced()) Shouldn't you support basic constraints as well? If you support advanced, why not multiple advanced? If this is correct, comment why. What if you update MediaConstraintsImpl::convertConstraints to support the new imageCaptureConstraints?
leclair329@gmail.com changed reviewers: + LeClair329@gmail.com
mcasas@chromium.org changed reviewers: + leclair329@gmail.com - LeClair329@gmail.com
https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html:38: const constraintsIn = { advanced : [ c ]}; On 2017/03/19 13:53:12, Guido Urdaneta wrote: > what if you use the basic constraint set too? I considered adding a non-advanced constraint entry in this test but didn't work because applyConstraints() is not wired for non-ImageCapture constraints and this test doesn't use getUserMedia() (l.35). https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:234: if (constraints.hasAdvanced()) On 2017/03/19 13:53:12, Guido Urdaneta wrote: > Shouldn't you support basic constraints as well? > If you support advanced, why not multiple advanced? > If this is correct, comment why. IIUC all ImageCapture constraints go under the advanced section; happy to add a comment here as to why if this assumption is correct. > > What if you update MediaConstraintsImpl::convertConstraints to support the new > imageCaptureConstraints? That method translates a WebMediaConstraints into MediaConstraints, and it's used to interact with the content/renderer implementation of the constraints; image capture goes directly to content/browser and its devices, and doesn't use WebX versions.
lgtm, but add extra comments. https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/imagecapture/MediaStreamTrack-getConstraints.html:38: const constraintsIn = { advanced : [ c ]}; On 2017/03/19 20:37:40, mcasas wrote: > On 2017/03/19 13:53:12, Guido Urdaneta wrote: > > what if you use the basic constraint set too? > > I considered adding a non-advanced constraint entry in this > test but didn't work because applyConstraints() is not > wired for non-ImageCapture constraints and this test > doesn't use getUserMedia() (l.35). Acknowledged. https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:234: if (constraints.hasAdvanced()) On 2017/03/19 20:37:40, mcasas wrote: > On 2017/03/19 13:53:12, Guido Urdaneta wrote: > > Shouldn't you support basic constraints as well? > > If you support advanced, why not multiple advanced? > > If this is correct, comment why. > > IIUC all ImageCapture constraints go under the > advanced section; happy to add a comment here > as to why if this assumption is correct. > I see that the code in the previous CL just passes advanced[0] to be processed. I think this is unusual and not necessarily what the users would expect, but since it's still behind a flag and we're still working on making spec-compliant constraints work I guess it's OK for now. Add a comment explaining that. > > > > What if you update MediaConstraintsImpl::convertConstraints to support the new > > imageCaptureConstraints? > > That method translates a WebMediaConstraints into > MediaConstraints, and it's used to interact with the > content/renderer implementation of the constraints; > image capture goes directly to content/browser and > its devices, and doesn't use WebX versions. content/renderer uses WebMediaConstraints, but the whole implementation is in Blink. You can add this conversion there, but since for now you are supporting only the first advanced set, it's OK to leave as is for now, with a TODO to move the code to MediaConstraintsImpl.
https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2758063002/diff/50001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:234: if (constraints.hasAdvanced()) On 2017/03/20 09:17:35, Guido Urdaneta wrote: > On 2017/03/19 20:37:40, mcasas wrote: > > On 2017/03/19 13:53:12, Guido Urdaneta wrote: > > > Shouldn't you support basic constraints as well? > > > If you support advanced, why not multiple advanced? > > > If this is correct, comment why. > > > > IIUC all ImageCapture constraints go under the > > advanced section; happy to add a comment here > > as to why if this assumption is correct. > > > > I see that the code in the previous CL just passes advanced[0] to be processed. > I think this is unusual and not necessarily what the users would expect, but > since it's still behind a flag and we're still working on making spec-compliant > constraints work I guess it's OK for now. Add a comment explaining that. > Correct, but note that the use of advanced[0] in applyConstraints() and the hasAdvanced() here have nothing to do: in applyConstraints() it's a limitation, whereas here it simply is a way to append |imageCaptureConstraints| to |constraints|. > > > > > > What if you update MediaConstraintsImpl::convertConstraints to support the > new > > > imageCaptureConstraints? > > > > That method translates a WebMediaConstraints into > > MediaConstraints, and it's used to interact with the > > content/renderer implementation of the constraints; > > image capture goes directly to content/browser and > > its devices, and doesn't use WebX versions. > > content/renderer uses WebMediaConstraints, but the whole implementation is in > Blink. You can add this conversion there, but since for now you are supporting > only the first advanced set, it's OK to leave as is for now, with a TODO to move > the code to MediaConstraintsImpl. Added TODO. Usually WebX types are used for interchanges to and from content/renderer, so if the idea of MediaConstraintsImpl is to centralise the constraints management using WebX-only naming would be inaccurate, I'm assuming that class is still WIP and that's fine.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2758063002/#ps70001 (title: "guidou@ TODO()s")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 70001, "attempt_start_ts": 1490038922278260, "parent_rev": "7966f4f285614fc7792618dbdac7243677380657", "commit_rev": "726f6720d94c103683515f9887082a4c9376d133"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/726f6720d94c103683515f988708... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:70001) as https://chromium.googlesource.com/chromium/src/+/726f6720d94c103683515f988708... |