Use constrainable pattern for ImageCapture (#124) #146
Conversation
|
@alvestrand can you please take a first coarse look? |
|
@miguelao Can you update examples as well? |
| </dd> | ||
| <dl class="domintro"> | ||
| <dt><dfn dict-member for="MediaTrackSupportedConstraints"><code>whiteBalanceMode</code></dfn></dt> | ||
| <dd>Whether <a>white balance mode</a> constraining is supported.</dd> |
There was a problem hiding this comment.
I suggest to use the term "recognized" here as well as in the intro, to lessen the (high!) chance that people think that it's guaranteed to work when this member is true. It's not guaranteed to work, it's just guaranteed to be recognized.
| <pre class="idl"> | ||
| partial dictionary MediaTrackConstraintSet { | ||
| MeteringMode whiteBalanceMode; | ||
| ConstraintDouble colorTemperature; |
There was a problem hiding this comment.
The type is named "ConstrainDouble". Somehow the link works, but the spelling is wrong.
There was a problem hiding this comment.
Corrected. The link was working because I defined it at the bottom of the file, and the text section was wrong while the url was right:
urlPrefix: https://www.w3.org/TR/mediacapture-streams/#; type: interface; text: ConstrainDouble ; url: idl-def-ConstrainDouble
| <li><dfn>Fill light mode</dfn> describes the flash setting of the capture device (e.g. |auto|, |off|, |on|). </li> | ||
| </ol> | ||
|
|
||
| # {{MediaSettingsRange}} # {#mediasettingsrange-section} | ||
|
|
||
| <pre class="idl"> | ||
| interface MediaSettingsRange { |
There was a problem hiding this comment.
All my previous comments on the "step" still apply. I'm sketptical.
IDL-wise, I'd like you to make this a subclass of DoubleRange, to emphasize that that's what it is.
There was a problem hiding this comment.
MediaSettingsRange is an interface
<pre class="idl">
interface MediaSettingsRange {
readonly attribute double max;
readonly attribute double min;
readonly attribute double step;
};
</pre>
whereas DoubleRange is a dictionary.
dictionary DoubleRange {
double max;
double min;
};
It makes sense to have it as a Dictionary since it's used for configuring constraints in the track; in ImageCapture it makes sense to have it as an interface because settings ranges are always composed of min-max and step. So, they're different ways to represent related concepts and both are OK.
| @@ -40,7 +40,7 @@ table td, table th { | |||
|
|
|||
| The API defined in this document captures images from a photographic device referenced through a valid {{MediaStreamTrack}}. The produced image can be in the form of a {{Blob}} (see {{takePhoto()}} method) or as a {{ImageBitmap}} (see {{grabFrame()}}). Picture-specific settings can be optionally provided as arguments that can be applied to the device for the capture. | |||
|
|
|||
| # Image Capture API # {#imagecaptureapi"} | |||
| # Image Capture API # {#imagecaptureapi} | |||
There was a problem hiding this comment.
You need an explanaition up here to explain where the constraints fit in. Something like:
"This document defines a number of new constraints for a MediaStreamTrack that can be applied in order to make its behavior more suitable for taking pictures. The use of these constraints is via the standard getUserMedia() and applyConstraints() methods for a MediaStreamTrack. They will modify the behavior of the track and its source independently of the usage of the track with the ImageCapture object."
|
@beaufortfrancois the examples as they are would still work assuming that |
|
@alvestrand comments addressed except the @gmandyam I think you can take a look now, PTAL. |
|
ping @alvestrand |
|
@alvestrand @gmandyam PTAL |
|
@miguelao |
|
|
|
Sorry - that was my mistake. I thought you had redefined ConstrainBoolean, etc. in the Image Capture spec. Turns out you had just linked to the Media Capture and Streams spec. |
|
Assume that the MediaStream is currently sourcing a video element. Upon a successful call of applyConstraints(), is the expectation that all the media track constraints defined in Image Capture will be reflected in the video being rendered in the video element (e.g. contrast, sharpness, etc.)? Or will they only be applied when one of the ImageCapture methods (e.g. takePhoto) are invoked? |
|
I believe they will be applied and visible at the application moment (plus any hardware delays, e.g. zoom) since we'll be modifying the Track constraints. |
|
@alvestrand ping |
|
I think we can go forward with this change. |
|
@alvestrand please take another look otherwise it's good to go. |
| # MediaSettingsRange | ||
| ## Constrainable Properties ## {#constrainable-properties} | ||
|
|
||
| Many of these fields mirror hardware capabilities that are hard to define since can be implemented in a number of ways. Moreover, hardware manufacturers tend to publish vague definitions to protect their intellectual property. The following definitions are assumed for individual settings and are provided for information purposes: |
There was a problem hiding this comment.
nit: should there be a they or something similar inserted between since and can?
|
It seems @burnburn does not have the time to review now, I think this PR should be merged now. |
|
@miguelao I thought we were only going to move "live" settings to constraints? This appears to move all of them. |
|
That may be fine, just checking if I missed the rationale. |
|
@jan-ivar I thought about it, but it might be too complex to have two ways to configure the device; the quintessential example are |
|
@miguelao Users would be wrong. :-) What we call "delayed" properties aren't really applied to the device at all at the time of Also, there'd never be any conflict; no need for The alternative seems more logical to me: I.e. fess up to these not really being video track properties, but properties of the Of course, logic isn't everything. Expecting users to remember which settings are video settings and which are photo settings, may frustrate more than the functional clarity helps. |
|
But if |
|
@jan-ivar I understand your last 2 comments; they'd apply to But since those 3 still have ranges/supported sets (e.g. |
|
ping @jan-ivar |
|
@miguelao Isn't there also I don't have a concrete proposal, merely challenging what is most logical:
If we chose the former, any ranges for values of
In a vacuum where constraints didn't exist, I'd say the most logical way to represent "delayed" (photo) settings would be an array of values for each thing, and then have If we chose the latter, for "delayed" (photo) properties which never collide, the I'm curious, how do we imagine a user "configuring" their device in the single constraints model? What would the UI look like? How would the app learn the discrete settings to offer as choices?
What's the ideal UI here? Maybe we should start there. |
|
Let's bring the list here for conveniency (apologies,
Code wise we could
So essentially this is the |
|
This makes sense to me - from the viewpoint of "are these constraints usable for video streams", they all seem usable. Agree that "pointsOfInterest" is likely to be immediate & useful for video. WRT the odd "fillLightMode" case (both), would it make sense to separate this into two capabilities, "light" (a constraint) and "flash" (a photo property)? |
|
Two updates: I just noticed that I like @alvestrand suggestion, so I'll split So the only remaining photo capabilities will be: Finalising a new pull request so we can continue the conversation there. |
|
#150 is the new pull request. |
👍 except note that there's no such thing as |
|
Closing this in favour of #150 . |
First draft of using
ConstrainablePatternfor ImageCapture capabilities and settings management.This PR:
Extensionssection, whereMediaTrackSupportedConstraints,MediaTrackCapabilities,MediaTrackConstraintSetandMediaTrackSettingsdictionaries are extended to include our set of capabilities/settings.getPhotoCapabilities(),setOptions(),PhotoCapabilitiesandPhotoSettingsto a legacy non-normative section, for the time being.A rendering of the contents of this PR can be found in https://rawgit.com/Miguelao/mediacapture-image/pr001_constrainable_pattern/index.html