-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add focusDistance constraint #175
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes LGTM !
@@ -894,6 +912,65 @@ A {{Point2D}} represents a location in a two dimensional space. The origin of co | |||
</pre> | |||
</div> | |||
|
|||
## Update camera focus distance and {{takePhoto()}} ## {#example4} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to have a codepen/jsfiddle/anything (a la [1]) like the other examples, so casual visitors could just click through and see this in action.
[1] https://codepen.io/miguelao/pen/mAzXpN/left?editors=1010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I can add this to the PR. I don't have a codepen and didn't want to create one and mix it in when you'd already provided a few links under the same username.
Misread that link as you having created a codepen for it. If you create one I'll update the PR with the link, that way they all stay in the collection you've already created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way it's fine, we can add the example afterwards too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave things as is for now, sounds like there's still some discussion on the content of the examples which may cause them to be revised in the near future anyway.
I'd love @jan-ivar to have a reading as well. |
Also @alexshalamov since #158 showed interest in this functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a photographer, but isn't this focal length?
Some nits.
|
||
input.oninput = function(event) { | ||
track.applyConstraints({ | ||
advanced : [{focusMode: "manual", focusDistance: event.target.value}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use advanced
. Just do:
track.applyConstraints({focusMode: "manual", focusDistance: event.target.value});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example is a cut and paste and modify from example 1 in the current spec, so both comments, regarding advanced
and the error catching apply to example 1 as well. I'm happy to make the change, just a note that it should be consistent with the other examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
advanced
is necessary bc of the way it's implemented in Chromium, and since for the examples sake the important thing is that they work, I'd say add a note here and please some one file a https://crbug.com/new ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yellowdoge Limitations in an implementation should not limit what we put in spec examples IMHO. Specs drive implementations, not the other way around. I.e. our target audience is implementers. For instance, the WebRTC examples contain code no-one has implemented yet. Web developers will hopefully have blogs to rely on for things that work today.
advanced
should be avoided where possible as its semantic complexity is higher than needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you're coming from, but in this case I'd rather avoid a casual reader copy-pasting the example and finding it not working: for that I'd add a comment in the code or a pop-up note in this Section (are examples normative?) detailing what's happening.
|
||
navigator.mediaDevices.getUserMedia({video: true}) | ||
.then(gotMedia) | ||
.catch(err => console.error('getUserMedia() failed: ', err)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All errors end up here, not just "getUserMedia()" errors, so the string is misleading. More obvious if gotMedia
were inlined.
index.bs
Outdated
@@ -670,6 +686,8 @@ When the {{getSettings()}} method is invoked on a video stream track, the user a | |||
</div> | |||
</li> | |||
|
|||
<li><dfn>Focus distance</dfn> is a numeric camera setting that controls the focus distance of the lens. The setting usually represents distance in millimeters to the optimal focus distance.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With double
there's always the option to go unit-less (which I think we've done elsewhere here? but it escapes me). However, I suppose it's best to use what users expect, which seems to be millimeters if this is focal length, and my brief internet search wasn't way off. I know the TAG recommends units like milliseconds for time, but doesn't say anything about distance. Fyi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's anything that insists its millimeters, just that it is a logical measurement to use. I believe manufacturers can make it arbitrary if they'd like. The Android API doesn't mention units at all, just the term distance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know what units the Android API uses? Whatever we choose we should be specific about it, and not leave it up to devices. This PR says milliliters which sounds fine to me, I'm just not a domain expert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the PR says usually represents distance in millimeters
similar to several other spots in the current spec which say usually. I don't think we can say anything definitive because I don't believe anything says manufacturers have to tie it to anything at all, it can be completely arbitrary.
From the Android API docs:
APPROXIMATE and CALIBRATED devices report the focus metadata in units of diopters (1/meter), so 0.0f represents focusing at infinity, and increasing positive numbers represent focusing closer and closer to the camera device. The focus distance control also uses diopters on these devices.
UNCALIBRATED devices do not use units that are directly comparable to any real physical measurement, but 0.0f still represents farthest focus, and android.lens.info.minimumFocusDistance represents the nearest focus the device can achieve.
The Windows API clearly states millimeters. So, given the 3 different sources of info, Android says an uncalibrated device will use arbitrary units, a calibrated device will use diopters, and on Windows its millimeters (maybe).
Note the difference between Android and Windows/Linux - on Android an increasing positive number is closer to the camera (diopter) but on Windows/Linux a decreasing positive number means closer to the camera (millimeters).
I suppose the spec could try to do the conversion so that they move in the same direction, but I figured platform/device specific stuff like that should be left alone since the device reports its possible values anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SI unit for distance is meters, not millimeters. Also, most of the lenses have clear markings (m or ft). Android uses diopters, which is not a problem, as it can be converted easily to meters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured platform/device specific stuff like that should be left alone since the device reports its possible values anyway.
@dsanders11 Valid point. Though having no conversion seems unfortunate in two respects:
- Inability to show units in the UI.
- Inability to hotswap one camera for another, and expect settings to persist.
I suppose apps could write javascript to transpose a setting's relative position in the legal range to the corresponding setting for a device with a different range. Though I'd rather we give this problem to the UAs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jan-ivar, fair points, especially about persisting settings across different cameras.
@yellowdoge, should I drop 'usually' and instead state that focus distance is in meters? Sounds like the consensus is to lock down the unit to meters, and if we're going to do that then it probably does more harm than good to say 'usually' since that leaves room for interpretation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the mediacapture spec, we should normalize hardware settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! One thing would be nice to address.
Focus distance should be in SI units, which is meters. Focusing distance scales on lenses are also in meters.
I've changed it to say meters as per @alexshalamov's comment. Implementation notes being that for Windows/Linux the value will need to be converted from millimeters to meters, and for Android from diopters to meters, unless the device is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks.
@yellowdoge, how are we looking? I think I've addressed any concerns outside of the more general discussion on the contents of the examples. I think the only thing I still have unanswered is whether the wording should be changed to remove 'usually' from the description that says focus distance is in meters, so that it's more definitive. |
Oops, sorry, I thought I had already approved this. Just one thing though: could you check your ipr status (seems to be failing now). |
@yellowdoge, I'm waiting on W3C regarding that, I've been in contact over email with @dontcallmedom, so just waiting for that to come to a conclusion. |
@yellowdoge, still waiting on @dontcallmedom, who had an out-of-office email which said he'd be out until the 27th, so hopefully he's back tomorrow. I'll rebase once the IPR stuff is sorted. |
@dsanders11 I see this PR has passed all the checks, does it mean your ipr status has been cleared? If so we should merge this PR. |
@yellowdoge, I noticed that as well, but I think it's actually that the repo has stopped doing the IPR status check. It says '1 successful check'. IPR status is still pending, it required sending a document by snail mail to France, which was sent last week. Once that is received...there are more instructions that will be emailed. So, it might still be another week or two. 😕 |
we've just received the snail mail; so while there will be a bit more of administrativia for @dsanders11 and me to go through, and while the IP manager will probably keep complaining, I feel confident enough that this can be safely merged form an IPR perspective. |
Merging then ! |
This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f
This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Cr-Commit-Position: refs/heads/master@{#588299}
This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Cr-Commit-Position: refs/heads/master@{#588299}
This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Cr-Commit-Position: refs/heads/master@{#588299}
This reverts commit 3015459a9bf07f62ab7a8816ff88824568d87c04. Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=880728 Original change's description: > [Image Capture] Add focusDistance constraint. > > This CL adds focusDistance to ImageCapture API. > This was added to the spec in > w3c/mediacapture-image#175 > > Layout tests and mock tests are updated to support the same. > Support for Android is added. > > TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG > use slider to change focusDistance. > > BUG=732807 > > Intent to Implement and Ship discussions: > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 > > Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f > Reviewed-on: https://chromium-review.googlesource.com/1124839 > Reviewed-by: Miguel Casas <mcasas@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Guido Urdaneta <guidou@chromium.org> > Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> > Cr-Commit-Position: refs/heads/master@{#588299} TBR=kinuko@chromium.org,rijubrata.bhaumik@intel.com,mcasas@chromium.org,guidou@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 732807 Change-Id: I03df3791095d9c0ef23c64990576140aebe7f52a
…aint., a=testonly Automatic update from web-platform-tests[Image Capture] Add focusDistance constraint. This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Cr-Commit-Position: refs/heads/master@{#588299} -- wpt-commits: 0313d9f383d954ef401e79f3b669a5781aa3441a wpt-pr: 12790
…aint., a=testonly Automatic update from web-platform-tests[Image Capture] Add focusDistance constraint. This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com> Cr-Commit-Position: refs/heads/master@{#588299} -- wpt-commits: 0313d9f383d954ef401e79f3b669a5781aa3441a wpt-pr: 12790
…aint., a=testonly Automatic update from web-platform-tests[Image Capture] Add focusDistance constraint. This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasaschromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Guido Urdaneta <guidouchromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumikintel.com> Cr-Commit-Position: refs/heads/master{#588299} -- wpt-commits: 0313d9f383d954ef401e79f3b669a5781aa3441a wpt-pr: 12790 UltraBlame original commit: 8b2d044b31da4605c0d665acb576578ebd278c0f
…aint., a=testonly Automatic update from web-platform-tests[Image Capture] Add focusDistance constraint. This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasaschromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Guido Urdaneta <guidouchromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumikintel.com> Cr-Commit-Position: refs/heads/master{#588299} -- wpt-commits: 0313d9f383d954ef401e79f3b669a5781aa3441a wpt-pr: 12790 UltraBlame original commit: 8b2d044b31da4605c0d665acb576578ebd278c0f
…aint., a=testonly Automatic update from web-platform-tests[Image Capture] Add focusDistance constraint. This CL adds focusDistance to ImageCapture API. This was added to the spec in w3c/mediacapture-image#175 Layout tests and mock tests are updated to support the same. Support for Android is added. TEST= run the demo in https://codepen.io/rijuB/pen/NzWpxG use slider to change focusDistance. BUG=732807 Intent to Implement and Ship discussions: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/oNxzXaFY9c8 Change-Id: I9b7cbf3c85fd35741a8c7ed229910a996e14ee8f Reviewed-on: https://chromium-review.googlesource.com/1124839 Reviewed-by: Miguel Casas <mcasaschromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Guido Urdaneta <guidouchromium.org> Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumikintel.com> Cr-Commit-Position: refs/heads/master{#588299} -- wpt-commits: 0313d9f383d954ef401e79f3b669a5781aa3441a wpt-pr: 12790 UltraBlame original commit: 8b2d044b31da4605c0d665acb576578ebd278c0f
No description provided.