Skip to content
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

The (bool or ConstrainDouble) case for pan/tilt/zoom is unclear #225

Closed
guidou opened this issue Apr 28, 2020 · 20 comments
Closed

The (bool or ConstrainDouble) case for pan/tilt/zoom is unclear #225

guidou opened this issue Apr 28, 2020 · 20 comments
Labels
PTZ Pan-Tilt-Zoom

Comments

@guidou
Copy link

guidou commented Apr 28, 2020

It says true is mapped to an "empty" constraint and false means null.
What is the difference here?
I am assuming empty constraint here means unconstrained (i.e., same as not explicitly given constraint) and null means the same as saying pan: null. But this means unconstrained too, so both true and false appear to mean the same thing.
It looks simpler to just leave it unconstrained.

Note that the constraints model is not based on setting values to properties.
A constraint is a set of values for a property (where unconstrained means all possible values/similar to an universal set), a device capability is another set of values, and constraints processing is basically finding the intersection between these two sets.
If the intersection is empty, the request is rejected if it's a basic constraint, or ignored if it's an advanced constraint.
If the intersection is nonempty, the UA should configure the device using any value from the intersection. If an ideal is provided, then the value from the intersection closest to the ideal should be used.

It looks to me that the intent is to have a way to force the request of the PTZ permission, but this could be specified in other ways. For example, by saying that if any of the PTZ constraints is not unconstrained or has an ideal value, the permission must be requested.
But you have to be careful in how you specify this permission request, because there are different permission models. For example, Firefox uses a per-device model, while Chrome uses a per-class-of-device model. The language should be compatible with both models.

@riju riju added the PTZ Pan-Tilt-Zoom label Apr 28, 2020
@beaufortfrancois
Copy link
Contributor

beaufortfrancois commented Apr 28, 2020

For example, by saying that if any of the PTZ constraints is not unconstrained or has an ideal value, the permission must be requested.

I would love if we were not forcing web developers to provide "good enough default" values for pan/tilt/zoom as there's no guarantee they would mean the same for all cameras.

// Prompt user to access camera, including camera movement.
await navigator.mediaDevices.getUserMedia({
  video: {
    pan: 0, // Let's cross fingers that 0 means "middle" here ;(
    tilt: 0, // Let's cross fingers that 0 means "middle" here ;(
    zoom: 1, // Let's cross fingers that 1 means "no zoom" here ;(
  }
});

@beaufortfrancois
Copy link
Contributor

For info, here's the explanation when we introduced this pattern: #218 (comment)

@guidou
Copy link
Author

guidou commented Apr 28, 2020

I'm not strictly opposed to having the true/false values, but the current language suggests that they mean the same (unconstrained).
If true is redefined to mean an ideal value representing "no zoom" or "middle" and false means unconstrained, I think that could work.

@guidou
Copy link
Author

guidou commented Apr 28, 2020

But in that case, why not use a string instead of a boolean?

@guidou
Copy link
Author

guidou commented Apr 28, 2020

For info, here's the explanation when we introduced this pattern: #218 (comment)

The rationale seems wrong to me.
First, audio/video are media types, while PTZ are constrainable properties for the video type.
Second, it is suggested that pan: true is a faster way to say pan:{}. Well, not saying anything is an even faster way, so adding a new way to say unconstrained is inconsistent.

@eehakkin
Copy link
Contributor

For info, here's the explanation when we introduced this pattern: #218 (comment)

The rationale seems wrong to me.
First, audio/video are media types, while PTZ are constrainable properties for the video type.

It's true that those cases are conceptually different. I aimed at syntactic similarity, if that has any merit.

Second, it is suggested that pan: true is a faster way to say pan:{}. Well, not saying anything is an even faster way, so adding a new way to say unconstrained is inconsistent.

Not saying anything might be faster way but that results in a different state i.e. not requesting PTZ ( {name: "camera", panTiltZoom: true}) permission.

So, there there are three cases:

  1. getUserMedia({video: true}) or getUserMedia({video: {}}): PTZ permission is not requested and pan is unconstrained
  2. getUserMedia({video: {pan: true}}) or getUserMedia({video: {pan: {}}}): PTZ permission is requested and pan is unconstrained
  3. getUserMedia({video: {pan: 0}}) or getUserMedia({video: {pan: {ideal: 0}}}) or similar: PTZ permission is requested and pan is constrained

The API seems quite clear to me but please object if you like.

The other option which we considered was something like:

  1. getUserMedia({video: true}) or getUserMedia({video: {}}): PTZ permission is not requested and pan is unconstrained
  2. getUserMedia({video: {pan: 0}}) or getUserMedia({video: {pan: {ideal: 0}}}) or similar: PTZ permission is not requested, pan constraint is ignored and pan is thus unconstrained
  3. getUserMedia({video: {ptzPermission: true}}): PTZ permission is requested and pan is unconstrained
  4. getUserMedia({video: {pan: 0, ptzPermission: true}}) or getUserMedia({video: {pan: {ideal: 0}, ptzPermission: true}}) or similar: PTZ permission is requested and pan is constrained

Seems quite confusing to me, especially the second case.

@guidou
Copy link
Author

guidou commented Apr 28, 2020

So the idea is to be able to force a PTZ capability while keeping the property unconstrained.

The second option is a lot more consistent with the constraintable pattern, although I wouldn't call it ptzPermission, but just ptz to indicate that you want the PTZ capability. What you want is the capability to do PTZ, not necessarily to prompt for permission, since different browsers have different permission models.

I would personally prefer the second model, although the first one can work too if the value true is more explicitly defined to mean that you are requesting the PTZ capability while leaving specific numeric values unconstrained. Do not define it as {} since that one also normally means unconstrained, even if in this case we want to interpret {} as requesting the PTZ capability too.

For numeric constraints {}, null, or not saying anything, are all equivalent and it normally means unconstrained.

@guidou
Copy link
Author

guidou commented Apr 29, 2020

cc @alvestrand in case he has thoughts about this

@alvestrand
Copy link
Contributor

I think it is harmful to mix "request permission" and "apply setting" in a single syntax.

Consider for instance:

getUserMedia({video: {pan: true, zoom: 0}})

Is the user asking for tilt permission or not?
As far as I have been able to understand, the concept of PTZ permission is that you either have all of them or none of them - which means that {pan: true} also affects tilt and zoom - which is, to my mind, less than elegant.

Of the alternatives proposed so far, I think

getUserMedia({video: {ptzAbility: true})

is the least ugly solution.

@beaufortfrancois
Copy link
Contributor

Thanks for sharing your thoughts @alvestrand.
First two examples would work fine.

// Website wants to access camera PTZ if supported
await navigator.mediaDevices.getUserMedia({video: { panTiltZoom: true });
// Website wants to access camera PTZ if supported and reset zoom when accessed
await navigator.mediaDevices.getUserMedia({video: { panTiltZoom: true, zoom: 1 });

I guess we should define what would happen if either pan, tilt, or zoom DoubleConstraints are set when website has access or not to Camera PTZ already.

// Shall we allow this if website has already access to camera PTZ?
await navigator.mediaDevices.getUserMedia({video: { zoom: 1 });

Moreover, how does this model fit with applyConstraints?
Shall we have a panTiltZoom boolean as well to request permission or do we simply not allow it?

@guidou
Copy link
Author

guidou commented May 6, 2020

The panTiltZoom boolean can be used to request the capability without requesting action.
The pan, tilt, and zoom double constraints would request both the capability and action, so if you use those it's not necessary to specify the boolean one.
The same would apply to applyConstraints().

@guidou
Copy link
Author

guidou commented May 6, 2020

The only difference in applyConstraints() is that it operates on an existing track. If the corresponding source does not support PTZ, the applyConstraints() request would fail (or do nothing, if they're specified as ideal).

@beaufortfrancois
Copy link
Contributor

I thought @alvestrand didn't want to mix capabilities and actions. In this case, not request the panTiltZoom permission if just pan, tilt, or zoom was set. My understanding was that panTiltZoom would be used only for communicating to the UA that website wanted to access camera PTZ.

@guidou
Copy link
Author

guidou commented May 6, 2020

@alvestrand can clarify his proposal, but note that he mentioned "request permission" and "apply setting" which is not exactly the same thing as capabilities and actions.

My proposal is that we don't need anything specific for permissions in the API. I think we can have a separate boolean constraint to request the capability while leaving the properties unconstrained.
The other properties (pan, tilt, and zoom) can also implicitly request capability when present, so that it's not necessary to use the boolean one.
Browsers should have some freedom to decide when/how to implement a UI prompt for permissions.

An alternative approach that I'm also OK with (which I was not so convinced about before) is just having the double constraints and say that unconstrained but present constraints such as pan:{}, etc., should be interpreted as requesting capability. No booleans involved.
Then it's up to the browser when/how to request permissions.
It can be a bit confusing since pan/tilt/zoom are coupled permission/capability wise, but it would work and still be consistent with the general constrainable pattern. It's not be the first case of correlated constrainable properties.

I don't like the (boolean or DoubleConstraint) approach due to its inconsistency with the rest of the constrainable pattern. Having a boolean at the media type level (e.g., {audio:true, ptz:true}) would also be very inconsistent since PTZ is video from camera, not a new media type.

@alvestrand
Copy link
Contributor

I seem to remember that we had a long discussion with webidl folks, ending up in them saying that the spec (and the code!) makes it impossible for C++ to tell the difference between {foo: {}} and {} - ie you can't tell that something is present but empty :-(

That's a WebIDL rule, not a WebRTC rule. Not likely to change.

@beaufortfrancois
Copy link
Contributor

@jan-ivar Would you have any thoughts on this?
It seems like you 👍 #218 (comment) before.

@eehakkin
Copy link
Contributor

eehakkin commented May 6, 2020

@alvestrand:

I seem to remember that we had a long discussion with webidl folks, ending up in them saying that the spec (and the code!) makes it impossible for C++ to tell the difference between {foo: {}} and {} - ie you can't tell that something is present but empty :-(

It would be interesting to know the details and to which code you are referring to.

At least in Chrome WebIDL implementation it is possible for C++ to tell the difference between {pan: {}} and {}. I have made POC which does exactly that (and also {pan: true} for that matter).

Based on my reading of an ECMAScript value an IDL dictionary type value conversion algorithm (https://heycam.github.io/webidl/#es-dictionary), if the dictionary members do not have defaults values (like in the case of MediaTrackConstraintSet), if the ECMAScript value is null or undefined or if the ECMAScript value is Object without property key or if the value of the property is undefined, the key member of the resulting IDL dictionary value will not be set and it will thus not exists and it should be possible for C++, for instance, to detect that the key does not exist.

If however, the IDL dictionary definition has default values for members (like in the case of MediaTrackSupportedConstraints), the ECMAScript value an IDL dictionary type value conversion algorithm sets the corresponding resulting IDL dictionary value members using the default values if the ECMAScript value is null or undefined or if the ECMAScript value is Object without property key or if the value of the property is undefined.

Could it be that it is the latter case (with default values) which you are remembering?

@jan-ivar
Copy link
Member

jan-ivar commented May 6, 2020

impossible for C++ to tell the difference between {foo: {}} and {}

Mozilla's webidl compiler was updated to allow detecting absence of dictionary-typed dictionary members in bug 1368949. I believe the relevant spec change was whatwg/webidl#750. So I don't see a WebIDL issue here.

Also, I don't see that we need to tell the difference: pan: true is semantic sugar for pan: {} with the same semantic meaning (and pan: false = pan: undefined)

The purpose is to detect interest in pan/tilt/zoom from the presence of the same-named constraints, which works even without the boolean overloads. I think that's simple and great.

I have some nits around usage of null here (undefined is better) in the prose which isn't 100% correct, and I think the prose should be explicit about it only applying to getUserMedia and not applyConstraints #223 but other than that I don't see a problem.

While I'm not a super-fan of the boolean overloads, but I don't see that they do any harm, and I agree they have a slight leg up on readability, so I think they work.

@guidou
Copy link
Author

guidou commented May 19, 2020

After the discussion, I'm OK with (boolean or DoubleConstraint) as long as all the wording in the spec is replaced to indicate that it pan:true is just syntactic sugar for pan:{} and pan:false the same as pan being undefined/not provided. Do not try to give extra meaning to false and true, in particular with regards to permissions. Define that separately and ignore the boolean values there.

eehakkin added a commit to eehakkin/intel-w3c-mediacapture-image that referenced this issue May 25, 2020
eehakkin added a commit to eehakkin/intel-w3c-mediacapture-image that referenced this issue May 26, 2020
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue May 29, 2020
This CL adds 'boolean or DoubleConstraint' support for pan, tilt, and
zoom constraints in getUserMedia. Spec says that `pan: true` is just
syntactic sugar for `pan: {}` and `pan: false` the same as pan being
undefined/not provided.

Spec: w3c/mediacapture-image#225
Change-Id: I0f5788ff91bceda14bc9cdad0852e1195755a3f4
Bug: 934063
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2210237
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#773060}
riju added a commit that referenced this issue Jun 4, 2020
Clarify pan/tilt/zoom constraints (#225)
@beaufortfrancois
Copy link
Contributor

I believe we can now close this issue as #226 has now been merged.
@riju

@riju riju closed this as completed Jun 24, 2020
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL adds 'boolean or DoubleConstraint' support for pan, tilt, and
zoom constraints in getUserMedia. Spec says that `pan: true` is just
syntactic sugar for `pan: {}` and `pan: false` the same as pan being
undefined/not provided.

Spec: w3c/mediacapture-image#225
Change-Id: I0f5788ff91bceda14bc9cdad0852e1195755a3f4
Bug: 934063
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2210237
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Cr-Original-Commit-Position: refs/heads/master@{#773060}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 62ee93390521249984d3d8102a309494eb6c7fdb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTZ Pan-Tilt-Zoom
Projects
None yet
Development

No branches or pull requests

6 participants