-
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
Update pan, tilt and zoom constraints #218
Conversation
@eehakkin : Let's edit this PR to add the JS example in Examples Section |
@beaufortfrancois , @reillyeon, @kenchris PTAL |
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 looks good to me.
ConstrainDouble sharpness; | ||
|
||
ConstrainDouble focusDistance; | ||
(boolean or ConstrainDouble) pan; |
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.
Can you walk through the rationale of this boolean or ConstrainDouble
?
Why introducing this pattern now?
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.
My rationale is to keep similarity with audio/video. So while it is possible to say navigator.mediaDevices.getUserMedia({video: {}})
, it is maybe clearer to say navigator.mediaDevices.getUserMedia({video: true})
. Similarly while should be possible to say navigator.mediaDevices.getUserMedia({video: {pan: {}, zoom: {}}})
, it might be clearer to say navigator.mediaDevices.getUserMedia({video: {pan: true, zoom: true}})
.
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 agree with you about the simplicity. It looks much better.
I'm wondering why this pattern wasn't used before for other constraints than audio/video. Maybe there's something we're missing.
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 maybe would know
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.
@beaufortfrancois no particular reason, IIRC this spec predates the major clean up and homogeneisation of the constraints about 3-4y ago...? So perhaps it was just due an overhaul. IMHO this looks better & we should create implementation bugs.
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.
Thank you!
c4140e4
to
6f55db9
Compare
ConstrainDouble sharpness; | ||
|
||
ConstrainDouble focusDistance; | ||
(boolean or ConstrainDouble) pan; |
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 maybe would know
input.oninput = function(event) { | ||
track.applyConstraints({advanced: [{tilt: event.target.value}]}); | ||
}; | ||
input.parentElement.hidden = false; |
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 think we can remove the hidden
part as it doesn't add that much value in the context of an example in a spec.
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.
Note that it wasn't present in the pan
example section
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.
LGTM with an updated codepen example that reflects new pan and tilt constraints.
index.bs
Outdated
@@ -835,7 +847,7 @@ A {{Point2D}} represents a location in a two dimensional space. The origin of co | |||
Slightly modified versions of these examples can be found in e.g. <a href="https://codepen.io/collection/nLeobQ/">this codepen collection</a>. | |||
</div> | |||
|
|||
## Update camera zoom and {{takePhoto()}} ## {#example1} | |||
## Update camera pan, tilt and zoom and {{takePhoto()}} ## {#example1} | |||
|
|||
<div class="note"> | |||
The following example can also be found in e.g. <a href="https://codepen.io/miguelao/pen/BLPzKx/left?editors=1010">this codepen</a> with minimal modifications. |
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 believe we can now link to an updated sample
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 @eehakkin and @beaufortfrancois !
@eehakkin Was the reason for the new navigator.mediaDevices.getUserMedia({video: {pan: 0, tilt: 0, zoom: 1}}); |
Yes, exactly that. So with the new |
Our model of thinking has been :
The Pan-Tilt-Zoom permission can only be requested (and only shown in the prompt) if the currently connected/selected camera supports it.
Camera permission without PTZ (obtained using an older version of the user agent, or with another camera) is not implicitly upgraded to Camera permission with PTZ (even if the hardware supports it). The permission has to be re-requested.
In w3ctag/design-reviews#358, TAG wants to make sure that from the web app’s perspective: Pan-Tilt-Zoom needs to be explicitly requested as an extension to video capture permission.
To facilitate the previous requirements we want to add a new CameraDevicePermissionDescriptor dictionary introduced in w3c/permissions/pull/204 to differentiate between the two cases and to modify media capture so that using a pan, tilt or zoom constrainable property prompts for a
{name: "camera", panTiltZoom: true}
permission and that it is possible to request elevated permission prompt duringgetUserMedia
call in order to avoid double permission prompt ({name: "camera"}
duringgetUserMedia
and{name: "camera", panTiltZoom: true}
duringapplyConstraints
).This allows the following kind of code:
For API, it would of course be possible to combine
{video: {pan: true, tilt: true, zoom: true}}
to{video: {panTiltZoom: true}}
but in that case one had to usenavigator.mediaDevices.getUserMedia({video: {panTiltZoom: true, zoom: {min: 2}}})
instead ofnavigator.mediaDevices.getUserMedia({video: {zoom: {min: 2}}})
or similar if one wants to use pan, tilt or zoom constraints in getUserMedia. With separate pan, tilt and zoom (instead of a combined panTiltZoom) it is also easier to expand to roll if pan/tilt/roll/zoom cameras ever become more widespread. But please share your opinions./cc @riju