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

Reject with TypeError on required PTZ contraints in basic getUserMedia contraints #261

Conversation

eehakkin
Copy link
Contributor

@eehakkin eehakkin commented Sep 17, 2020

This is to address https://www.w3.org/2020/09/15-webrtc-minutes.html#r05

Some details related to the resolution are not entirely clear to me, so please correct me if I misunderstood something or if you prefer something else.

getUserMedia()

Required PTZ constraints in the basic constraint set are denied (and throw an OverconstraintedError) and PTZ capability constraints ({zoom: true} etc.) are ideal in the basic constraint set. Therefore, a PTZ constraint in the basic set cannot cause getUserMedia() to fail conditionally. This should be a clear case, I hope.

In this PR, required PTZ constraints are still allowed in the advanced constraint sets, for four reasons:

  1. The advanced constraint sets cannot throw an OverconstraintedError but the UA may instead ignore them.
  2. More importantly, a PTZ capability requests ({advanced: [{tilt: true}]} etc.) look like bare value constraints (while being syntactic sugar for empty constraints i.e. {advanced: [{tilt: {}}]} etc.) and bare value constraints are normally treated as exact required constraints in the advanced sets.
  3. I do not see justification for added complexity required for changing PTZ constraint types to ConstraintDoubleOrBoolean which would allow {advanced: [{pan: {ideal: true}}]} etc. to be used.
  4. The advanced constraint sets cannot be used for fingerprinting without a camera permission.

What do you think?

applyConstraints()

Required PTZ constraints are still allowed. Web apps can check capabilities with getCapabilities() before applying constraints. While only ideal and exact required (but not min and max required) constraints probably make sense, I do not see a point for special-casing PTZ constraints here.

I also clarified that PTZ capabilities cannot be obtained with applyConstraints().

@beaufortfrancois @guidou @jan-ivar @riju @youennf: PTAL

@riju riju added the PTZ Pan-Tilt-Zoom label Sep 17, 2020
index.bs Outdated
If the {{visibilityState}} of the <a>top-level browsing context</a> value is "hidden", the {{applyConstraints()}} algorithm MUST throw a {{SecurityError}} if {{MediaTrackConstraintSet/pan}} dictionary member exists after a possible normalization.
If the {{getUserMedia()}} algorithm uses a {{MediaTrackConstraintSet}} object and its {{MediaTrackConstraintSet/pan}} dictionary member which exists after a possible normalization:
<ul>
<li>The algorithm MUST throw an {{OverconstrainedError}} if the constraint is in the basic constraints and is <a>required</a>.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to state it here.
Note though that mediacapture-main will also state this as part of w3c/mediacapture-main#707

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@eehakkin eehakkin force-pushed the feature/deny-mandatory-ptz-constraints-in-gum branch from 8e088ea to edc8e07 Compare September 23, 2020 13:39
index.bs Outdated
@@ -598,6 +598,7 @@ This Section defines a number of new set of <a>Constrainable Properties</a> for
<dt><dfn dict-member for="MediaTrackConstraintSet"><code>pan</code></dfn></dt>
<dd>A value of true is normalized to a value of empty {{ConstrainDouble}}.
A value of false is normalized to a value of undefined.
An empty {{ConstrainDouble}} implies no constraints but a <a>pan capability</a> request (ideal in basic constraints and required in advanced constraints).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does ideal in basic constraints and required in advanced constraints mean?
My understanding is that we don't want to have a way to require the capability.
Since advanced constraints are for required constraints only, pan:{} in advanced should only mean no constraints and nothing else. No capability request. Otherwise we do not solve the fingerprinting issue we want to address by not having required constraints.

To make it clear, allowing pan:{} or pan:true to mean a required capability request in an advanced set allows fingerprinting by adding additional constraints and checking if the other constraints were ignored.
For example: getUserMedia({video: {advanced: [{pan:true, width:10}]}).
In this case, in Chromium (which supports crop-and-scale), getting a track with a width different than 10 means that pan is not supported, which is basically equivalent to having an OverconstrainedError if the capability request was required in the basic set.

IMO, now that we want to treat capability requests similarly to an ideal value, such requests should be only in the basic set, which is the only one that allows ideal values.

Copy link
Member

@jan-ivar jan-ivar Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it clear, allowing pan:{} or pan:true to mean a required capability request in an advanced set allows fingerprinting by adding additional constraints and checking if the other constraints were ignored.

@guidou This is after the user has granted permission, so do we care? Can't they already check whether they were granted ptz permission? If they were, they know the user has a ptz device. If they weren't, then most likely not¹.

... which is basically equivalent to having an OverconstrainedError if the capability request was required in the basic set.

They're not equivalent, because OverconstrainedError is thrown before any permission prompt, which can be used by tracking libraries without a user's knowledge or permission.

advanced constraints cannot throw OverconstrainedError by design, so I don't see a significant fingerprint risk.


1. Sure some user agents may offer users a way to downgrade requests to non-ptz, but I doubt many would.

@eehakkin
Copy link
Contributor Author

In the intro, I stated that some details related to the resolution are not entirely clear to me and that this PR ensures that PTZ constraint cannot cause getUserMedia() to fail conditionally. In #261 (comment) @guidou noted that that is not enough to prevent fingerprinting.

So I have more questions:

  • What should getUserMedia({video: {advanced: [{pan: {exact: 1234}, width: 10}]} or getUserMedia({video: {advanced: [{pan: 1234, width: 10}]} do? Should the advanced constraint set be ignored? Note that @youennf's PR Identify the list of allowed required constraints for getUserMedia mediacapture-main#707 does not do that. If the advanced constraint set is ignored, should it be ignored silently, should it produce a console warning or equivalent or should it throw an OverconstrainedError (which advanced constraints do not normally throw)?
  • What should getUserMedia({video: {advanced: [{pan: {ideal: 1234}, width: 10}]} do? Is that ideal request for pan capability and for the pan setting of 1234?
  • What should getUserMedia({video: {advanced: [{pan: true, width: 10}]} or getUserMedia({video: {advanced: [{pan: {}, width: 10}]} do? Is that ideal request for pan capability (like getUserMedia({video: {pan: true}}))? Makes kind of sense. Or is that no constraint and no capability request? That would be easy but in that case is getUserMedia({video: {advanced: [{pan: {ideal: 1234}, width: 10}]} an ideal request for the pan setting of 1234 but not an ideal request for pan capability which does not make much sense.
  • Or should PTZ constraints in the advanced constraint sets be ignored or forbidden in getUserMedia() altogether? That might actually make most sense as advanced PTZ constraints in getUserMedia() do not really have much value. If that route is selected, should PTZ constraints in the advanced constraint sets be silently ignore, or should they produce a console warning or equivalent or should they throw an OverconstrainedError?

@guidou
Copy link

guidou commented Sep 23, 2020

  • What should getUserMedia({video: {advanced: [{pan: {exact: 1234}, width: 10}]} or getUserMedia({video: {advanced: [{pan: 1234, width: 10}]} do? Should the advanced constraint set be ignored? Note that @youennf's PR w3c/mediacapture-main#707 does not do that. If the advanced constraint set is ignored, should it be ignored silently, should it produce a console warning or equivalent or should it throw an OverconstrainedError (which advanced constraints do not normally throw)?

The whole advanced set should be ignored because it contains a required constraint for pan (naked values in advanced are exact). Thus, the examples given should be equivalent to getUserMedia({video:true}).

  • What should getUserMedia({video: {advanced: [{pan: {ideal: 1234}, width: 10}]} do? Is that ideal request for pan capability and for the pan setting of 1234?
    Ideal values are ignored for advanced sets. I believe this is equivalent to
    getUserMedia({video: {advanced: [{pan: {}, width: 10}]}), which should be equivalent to getUserMedia({video: {advanced: [{width: 10}]})
  • What should getUserMedia({video: {advanced: [{pan: true, width: 10}]} or getUserMedia({video: {advanced: [{pan: {}, width: 10}]} do? Is that ideal request for pan capability (like getUserMedia({video: {pan: true}}))? Makes kind of sense. Or is that no constraint and no capability request? That would be easy but in that case is getUserMedia({video: {advanced: [{pan: {ideal: 1234}, width: 10}]} an ideal request for the pan setting of 1234 but not an ideal request for pan capability which does not make much sense.

This should result in no constraints and no (ideal) capability request. There are no ideal values in advanced sets.

  • Or should PTZ constraints in the advanced constraint sets be ignored or forbidden in getUserMedia() altogether? That might actually make most sense as advanced PTZ constraints in getUserMedia() do not really have much value. If that route is selected, should PTZ constraints in the advanced constraint sets be silently ignore, or should they produce a console warning or equivalent or should they throw an OverconstrainedError?

Based on the conclusion in the virtual interim, PTZ constraints in the advanced sets can only have two meanings.

  1. They cause the set to be ignored if they include any required constraint (e.g., pan: 4, pan: {min: 4}, pan: {exact: 4}) since we required constraints for PTZ are not allowed.
  2. They are ignored because they specify no constraints, so they don't affect anything in the SelectSettings algorithm (e.g., pan: {} or pan: true).

Thus, in practice PTZ constraints in the advanced set are not useful in practice. The reason is that we don't want required constraints for PTZ and the only thing advanced sets can do is specify required constraints.

@eehakkin eehakkin force-pushed the feature/deny-mandatory-ptz-constraints-in-gum branch from edc8e07 to ad5ece9 Compare September 24, 2020 08:06
@eehakkin
Copy link
Contributor Author

I updated the commit and the intro.

  • What should getUserMedia({video: {advanced: [{pan: {exact: 1234}, width: 10}]} or getUserMedia({video: {advanced: [{pan: 1234, width: 10}]} do? Should the advanced constraint set be ignored? Note that @youennf's PR w3c/mediacapture-main#707 does not do that. If the advanced constraint set is ignored, should it be ignored silently, should it produce a console warning or equivalent or should it throw an OverconstrainedError (which advanced constraints do not normally throw)?

The whole advanced set should be ignored because it contains a required constraint for pan (naked values in advanced are exact). Thus, the examples given should be equivalent to getUserMedia({video:true}).

Done.

  • What should getUserMedia({video: {advanced: [{pan: {ideal: 1234}, width: 10}]} do? Is that ideal request for pan capability and for the pan setting of 1234?
    Ideal values are ignored for advanced sets. I believe this is equivalent to
    getUserMedia({video: {advanced: [{pan: {}, width: 10}]}), which should be equivalent to getUserMedia({video: {advanced: [{width: 10}]})
  • What should getUserMedia({video: {advanced: [{pan: true, width: 10}]} or getUserMedia({video: {advanced: [{pan: {}, width: 10}]} do? Is that ideal request for pan capability (like getUserMedia({video: {pan: true}}))? Makes kind of sense. Or is that no constraint and no capability request? That would be easy but in that case is getUserMedia({video: {advanced: [{pan: {ideal: 1234}, width: 10}]} an ideal request for the pan setting of 1234 but not an ideal request for pan capability which does not make much sense.

This should result in no constraints and no (ideal) capability request. There are no ideal values in advanced sets.

These result in ideal constraints and ideal capability constraints in advanced sets which do not effect the result of the SelectSettings algorithm thus basically the same thing.

Based on the conclusion in the virtual interim, PTZ constraints in the advanced sets can only have two meanings.

  1. They cause the set to be ignored if they include any required constraint (e.g., pan: 4, pan: {min: 4}, pan: {exact: 4}) since we required constraints for PTZ are not allowed.
  2. They are ignored because they specify no constraints, so they don't affect anything in the SelectSettings algorithm (e.g., pan: {} or pan: true).

Thus, in practice PTZ constraints in the advanced set are not useful in practice. The reason is that we don't want required constraints for PTZ and the only thing advanced sets can do is specify required constraints.

Done.

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a fingerprinting issue with advanced constraints, and I don't see any mention of it in the minutes, so I'm marking this as needs changing. Happy to discuss.

index.bs Outdated Show resolved Hide resolved
@guidou
Copy link

guidou commented Sep 24, 2020

Allowing required constraints in advanced sets has the same fingerprinting consequences as allowing them in the basic set.
If they were allowed in the basic set:
getUserMedia({video: {pan: {exact: 10}}) gives OverconstrainedError if pan is not supported, which is a fingerprinting issue.

Similarly, with advanced constraints and assuming 10 is never a default width,
getUserMedia({video: advanced: [{pan: 10, width: 10, resizeMode: "crop-and-scale"}]) gives you width 10 if pan is supported and width different from 10 if pan is not supported, so you get the exact same fingerprinting as in the basic set (note: naked values in advanced are all exact).

@jan-ivar
Copy link
Member

I also recommend we switching to patching algorithms¹ for greater specificity, since constraints are used in both getUserMedia and applyConstraints, and the current prose makes it hard to tell where it applies (getUserMedia only right?)

As to how to write input validation steps to throw on exact ptz constraints, there's precedent in getDisplayMedia, so I would look at that:

"4. For each existing member in constraints whose value, CS, is a dictionary, run the following steps: ... If CS contains a member whose name specifies a constrainable property ..., and whose value in turn is a dictionary containing a member named either min or exact, return a promise rejected with a newly created TypeError."

We should probably also throw TypeError, not OverconstrainedError, to be consistent with that precedent.


1. I did some algorithm patching a while ago here, if you need an example.

@guidou
Copy link

guidou commented Sep 24, 2020

I also recommend we switching to patching algorithms¹ for greater specificity, since constraints are used in both getUserMedia and applyConstraints, and the current prose makes it hard to tell where it applies (getUserMedia only right?)

As to how to write input validation steps to throw on exact ptz constraints, there's precedent in getDisplayMedia, so I would look at that:

"4. For each existing member in constraints whose value, CS, is a dictionary, run the following steps: ... If CS contains a member whose name specifies a constrainable property ..., and whose value in turn is a dictionary containing a member named either min or exact, return a promise rejected with a newly created TypeError."

We should probably also throw TypeError, not OverconstrainedError, to be consistent with that precedent.

I agree with throwing TypeError instead of OverconstrainedError in that case.

@jan-ivar
Copy link
Member

Allowing required constraints in advanced sets has the same fingerprinting consequences as allowing them in the basic set.

@guidou Not really, because

getUserMedia({video: {pan: {exact: 10}}) gives OverconstrainedError if pan is not supported, which is a fingerprinting issue.

...throws without a prompt (albeit at the risk of prompting the user, but it works even if they deny), whereas

getUserMedia({video: advanced: [{pan: 10, width: 10, resizeMode: "crop-and-scale"}]) gives you width 10 if pan is supported and width different from 10 if pan is not supported

...only fingerprints if the user grants permission. If they don't grant permission then no fingerprint; a significant difference.

@guidou
Copy link

guidou commented Sep 24, 2020

Allowing required constraints in advanced sets has the same fingerprinting consequences as allowing them in the basic set.

@guidou Not really, because

getUserMedia({video: {pan: {exact: 10}}) gives OverconstrainedError if pan is not supported, which is a fingerprinting issue.

...throws without a prompt (albeit at the risk of prompting the user, but it works even if they deny), whereas

getUserMedia({video: advanced: [{pan: 10, width: 10, resizeMode: "crop-and-scale"}]) gives you width 10 if pan is supported and width different from 10 if pan is not supported

...only fingerprints if the user grants permission. If they don't grant permission then no fingerprint; a significant difference.

I don't see the difference. Throwing OverconstrsainedErro or returning a track with width 640 (both without prompt) looks like the same fingerprinting to me. In both cases you detect that the constraint set with the required pan constraint couldn't be satisfied. Am I missing something?

@jan-ivar
Copy link
Member

I don't see the difference. Throwing OverconstrsainedErro or returning a track with width 640 (both without prompt) looks like the same fingerprinting to me. In both cases you detect that the constraint set with the required pan constraint couldn't be satisfied. Am I missing something?

@guidou Most sites on the web do not have camera or microphone permission.

Returning a track requires permission whereas OverconstrainedError does not.

Fingerprinting-wise I'd say that's a big difference. Wouldn't you?

@youennf
Copy link
Contributor

youennf commented Sep 24, 2020

Fingerprinting-wise I'd say that's a big difference. Wouldn't you?

This makes a difference. But don't we want to deprecate advanced constraints?

@guidou
Copy link

guidou commented Sep 24, 2020

I don't see the difference. Throwing OverconstrsainedErro or returning a track with width 640 (both without prompt) looks like the same fingerprinting to me. In both cases you detect that the constraint set with the required pan constraint couldn't be satisfied. Am I missing something?

@guidou Most sites on the web do not have camera or microphone permission.

Returning a track requires permission whereas OverconstrainedError does not.

Fingerprinting-wise I'd say that's a big difference. Wouldn't you?

I see your point now. You get the prompt for the regular camera permission with advanced, which is indeed a significant difference.

@jan-ivar
Copy link
Member

This makes a difference. But don't we want to deprecate advanced constraints?

@youennf I like my ducks in a row but not tied together. 😉

@eehakkin
Copy link
Contributor Author

So do I understand correctly that the consensus is to allow required PTZ constraints in the advanced sets, after all?

@youennf
Copy link
Contributor

youennf commented Sep 24, 2020

That is what mediacapture-main is mandating with w3c/mediacapture-main#707.
I am not exactly sure mediacapture-image should add normative statements on top of mediacapture-main, but it seems good to describe the intent and behavior.

As of the exact behavior, I would tend to remove advanced constraints ;)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 25, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 25, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
@eehakkin eehakkin force-pushed the feature/deny-mandatory-ptz-constraints-in-gum branch from ad5ece9 to 319c96d Compare September 25, 2020 10:38
@eehakkin eehakkin requested a review from jan-ivar September 25, 2020 10:40
A value of false is normalized to a value of undefined.
See <a>pan</a> constrainable property.</dd>
If the value after normalization is a double value or a {{ConstrainDoubleRange}} value (whether empty or not), that implies a <a>pan capability</a> request which is ideal in basic constraints and exact <a>required</a> in advanced constraints.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not happy with this sentence, because it doesn't appear to add anything normative that I can see (did you intend it to?) Please remove.

Same with the other two instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not happy with this sentence, because it doesn't appear to add anything normative that I can see (did you intend it to?) Please remove.

It should normatively state that navigator.mediaDevices.getUserMedia({video: {pan: true}}) gives lower (better) fitness distance for pan capable cameras than for pan incapable camers and that navigator.mediaDevices.getUserMedia({video: {height: 480, width: 640, advanced: [{height: 2160, pan: true, tilt: true, width: 3840}]}}) resolves either with a pan and tilt capable UHD video track or with a VGA video track without pan or tilt support.

Copy link
Member

@jan-ivar jan-ivar Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eehakkin I don't think it accomplishes that, because "implies a pan capability request" is not a thing in the constraints algorithm. I think we need #257 to fix this.

A value of false is normalized to a value of undefined.
See <a>pan</a> constrainable property.</dd>
If the value after normalization is a double value or a {{ConstrainDoubleRange}} value (whether empty or not), that implies a <a>pan capability</a> request which is ideal in basic constraints and exact <a>required</a> in advanced constraints.
See <a>pan</a> and <a>pan capability</a> constrainable properties.</dd>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"pan capability" is not a constrainable property.


Any algorithm which uses a {{MediaTrackConstraintSet}} object and its {{MediaTrackConstraintSet/pan}} dictionary member which exists after a possible normalization MUST <a>request permission to use</a> (as defined in [[!permissions]]) a PermissionDescriptor with its name member set to {{PermissionName/camera}} and its {{CameraDevicePermissionDescriptor/panTiltZoom}} member set to true, and, optionally, consider its {{DevicePermissionDescriptor/deviceId}} member set to any appropriate device's deviceId.
<dfn>Pan capability</dfn> is a boolean camera property representing camera's ability to pan which results in a proper non-degenerate setting range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"pan capability" doesn't appear used anywhere directly, only indirectly in the form of "pan capability request" which seems undefined (and unnecessary IMHO).

However, I like the definition, and think we should reference it from "pan capable cameras" which is just prose atm.

Suggested change
<dfn>Pan capability</dfn> is a boolean camera property representing camera's ability to pan which results in a proper non-degenerate setting range.
<dfn>Pan capable camera</dfn> is a camera able to pan across a proper non-degenerate setting range.

(Repeat comment for tilt and zoom.)

In the {{getUserMedia()}} algorithm, if the {{MediaTrackConstraintSet/pan}} dictionary member of a {{MediaTrackConstraintSet}} object exists after a normalization and the algorithm uses that dictionary member:
<ul>
<li>The algorithm MUST throw a {{TypeError}} if the constraint is in the basic constraints and is <a>required</a>.</li>
<li>The algorithm MUST <a>request permission to use</a> (as defined in [[!permissions]]) a PermissionDescriptor with its name member set to {{PermissionName/camera}} and its {{CameraDevicePermissionDescriptor/panTiltZoom}} member set to true, and, optionally, consider its {{DevicePermissionDescriptor/deviceId}} member set to any appropriate device's deviceId unless there are no pan capable cameras.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest referencing the pan capable cameras definition suggested above (repeat for tilt and zoom).

Comment on lines +810 to +813
In the {{getUserMedia()}} algorithm, if the {{MediaTrackConstraintSet/pan}} dictionary member of a {{MediaTrackConstraintSet}} object exists after a normalization and the algorithm uses that dictionary member:
<ul>
<li>The algorithm MUST throw a {{TypeError}} if the constraint is in the basic constraints and is <a>required</a>.</li>
<li>The algorithm MUST <a>request permission to use</a> (as defined in [[!permissions]]) a PermissionDescriptor with its name member set to {{PermissionName/camera}} and its {{CameraDevicePermissionDescriptor/panTiltZoom}} member set to true, and, optionally, consider its {{DevicePermissionDescriptor/deviceId}} member set to any appropriate device's deviceId unless there are no pan capable cameras.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By changing this from "Any algorithm which uses a {{MediaTrackConstraintSet}}" to "In the {{getUserMedia()}} algorithm", this loses the request permission requirement for applyConstraints which seems bad. 🐞

Copy link
Contributor Author

@eehakkin eehakkin Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the intention, applyConstraints never changes exposure or availability of PTZ capabilities thus there is no point for applyConstraints to request permission.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the intention, applyConstraints never changes exposure or availability of PTZ capabilities thus there is no point for applyConstraints to request permission.

@eehakkin I see no support in the minutes for this normative change. Did I miss it?

From the current spec, a site may access a camera through regular permission, discover it has pan: true capabilites thanks to #260, and then use track.applyConstraints({pan: true}) to request more (ptz) permission.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I miss it?

This was discussed so the recording should have it.
The idea was: regular getUserMedia, check capabilities, and then PTZ getUserMedia to get PTZ control.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youennf Is the recording available yet? I cannot seem to find it. In any case, consensus at w3c is written not verbal, and I see no resolution taken.

The idea was: regular getUserMedia, check capabilities, and then PTZ getUserMedia to get PTZ control.

Then why disallow: regular getUserMedia, check capabilities, and then PTZ applyConstraints to get PTZ control?

Sites calling getUserMedia again when they could be using applyConstraints is an anti-pattern. It encourages sites to structure their code in a way that frequently causes extra prompts all over the web for Firefox users who grant permission as one-shots by default (because there's a competing incentive to stop tracks before gUM to avoid limitations on phones).

While I understand that here we think this is not a problem because ptz will always prompt again, I think that's a faulty assumption. It's conceivable a user agent might allow a user to opt into always granting ptz with regular camera requests, but still gate camera requests behind one-time permissions (i.e. "always ask me for camera, but ptz is fine").

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why disallow: regular getUserMedia, check capabilities, and then PTZ applyConstraints to get PTZ control?

I previously backed up the idea of this flow and I am happy to discuss this further but I feel this is a separate issue.
This PR discussion is very long and it feels like, to make progress, we should split the PR and solely focus here on the initial scope: "Reject with TypeError on required PTZ contraints in basic getUserMedia contraints"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youennf with w3c/mediacapture-main#707 and #264 merged is there anything left here we need? Can we close it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have non normative text that tells that required constraints will throw.
We have #264 but this is not in the spec.
I do not feel strongly either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it warrants a note but a general one and not a PTZ specific one. I'll create a separate issue for that.

If the {{visibilityState}} of the <a>top-level browsing context</a> value is "hidden", the {{applyConstraints()}} algorithm MUST throw a {{SecurityError}} if {{MediaTrackConstraintSet/pan}} dictionary member exists after a possible normalization.
In the {{getUserMedia()}} algorithm, if the {{MediaTrackConstraintSet/pan}} dictionary member of a {{MediaTrackConstraintSet}} object exists after a normalization and the algorithm uses that dictionary member:
<ul>
<li>The algorithm MUST throw a {{TypeError}} if the constraint is in the basic constraints and is <a>required</a>.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that relying on definitions like "required" are unnecessary indirections here.

As I mentioned in #261 (comment) I'd much prefer this worded like we do in screen-capture, based solely on inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In w3c/mediacapture-main#707 (comment) @youennf seems to suggest the screen-capture to be changed instead. I tend to somewhat agree that copying definitions inline creates unnecessary duplication.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eehakkin w3c/mediacapture-main#707 has been merged now, so lets remove this sentence.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 26, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b

In the {{getUserMedia()}} algorithm, if the {{MediaTrackConstraintSet/zoom}} dictionary member of a {{MediaTrackConstraintSet}} object exists after a normalization and the algorithm uses that dictionary member:
<ul>
<li>The algorithm MUST throw a {{TypeError}} if the constraint is in the basic constraints and is <a>required</a>.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this should be left to mediacapture-main (which currently throws OverConstrained error, we could change it there to TypeError).
A note clarifying the intent is fine though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that you refer to w3c/mediacapture-main#707. I agree that once that is merged, there is no point to specify this here.
Also, w3c/mediacapture-main#707 applies to all Image Capture constraints so a single note in the beginning of the section "10. Photo Capabilities and Constrainable Properties" would probably be sufficient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eehakkin w3c/mediacapture-main#707 has merged, so lets remove this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is not needed anymore.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}
pull bot pushed a commit to Mu-L/chromium that referenced this pull request Sep 28, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}
@jan-ivar jan-ivar changed the title Deny mandatory PTZ contraints in basic getUserMedia contraints Throw TypeError on required PTZ contraints in basic getUserMedia contraints Sep 29, 2020
@jan-ivar jan-ivar changed the title Throw TypeError on required PTZ contraints in basic getUserMedia contraints Reject with TypeError on required PTZ contraints in basic getUserMedia contraints Sep 29, 2020
@jan-ivar
Copy link
Member

@eehakkin is this PR necessary anymore or can it be closed?

@eehakkin
Copy link
Contributor Author

@eehakkin is this PR necessary anymore or can it be closed?

The type error issue is now handled. Let's close this one and let's handle other issues separately.

@eehakkin eehakkin closed this Sep 30, 2020
@eehakkin eehakkin deleted the feature/deny-mandatory-ptz-constraints-in-gum branch September 30, 2020 10:01
pull bot pushed a commit to Alan-love/chromium that referenced this pull request Oct 1, 2020
This fixes getUserMedia to select the correct camera when cameras
support different PTZ controls (such as a zoom-only camera versus
a pan-tilt-zoom camera).

Spec:
w3c/mediacapture-image#261

Bug: 934063
Change-Id: I6162e1d3f26950151f91dca4ab14f4fa09f0964e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2428975
Commit-Queue: Eero Häkkinen <[email protected]>
Commit-Queue: Dale Curtis <[email protected]>
Auto-Submit: Eero Häkkinen <[email protected]>
Reviewed-by: Dale Curtis <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#812415}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 3, 2020
…basic getUserMedia constraints, a=testonly

Automatic update from web-platform-tests
[PTZ] Deny mandatory PTZ constraints in basic getUserMedia constraints

This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}

--

wpt-commits: dcb7fbd8b1c1223fcc72819cf1c66ac2c03b04c8
wpt-pr: 25785
ziransun pushed a commit to ziransun/wpt that referenced this pull request Oct 6, 2020
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}
ambroff pushed a commit to ambroff/gecko that referenced this pull request Nov 4, 2020
…basic getUserMedia constraints, a=testonly

Automatic update from web-platform-tests
[PTZ] Deny mandatory PTZ constraints in basic getUserMedia constraints

This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}

--

wpt-commits: dcb7fbd8b1c1223fcc72819cf1c66ac2c03b04c8
wpt-pr: 25785
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This PR makes sure required PTZ constraints in the basic constraint set
are denied (and throw a TypeError) and PTZ capability constraints
({ zoom: true } etc.) are ideal in the basic constraint set.
Therefore, a PTZ constraint in the basic set cannot cause getUserMedia()
to fail conditionally.

Spec:
w3c/mediacapture-image#261
w3c/mediacapture-main#707

Bug: 934063
Change-Id: I7324ae32b064dbf3b89325166834e2694eca593b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411946
Commit-Queue: François Beaufort <[email protected]>
Reviewed-by: Eero Häkkinen <[email protected]>
Reviewed-by: Philip Jägenstedt <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#811242}
GitOrigin-RevId: e3d45995537f476c48728aac5c62da3ac8fbd133
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This fixes getUserMedia to select the correct camera when cameras
support different PTZ controls (such as a zoom-only camera versus
a pan-tilt-zoom camera).

Spec:
w3c/mediacapture-image#261

Bug: 934063
Change-Id: I6162e1d3f26950151f91dca4ab14f4fa09f0964e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2428975
Commit-Queue: Eero Häkkinen <[email protected]>
Commit-Queue: Dale Curtis <[email protected]>
Auto-Submit: Eero Häkkinen <[email protected]>
Reviewed-by: Dale Curtis <[email protected]>
Reviewed-by: Guido Urdaneta <[email protected]>
Reviewed-by: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#812415}
GitOrigin-RevId: 072d913f5dcc206a6ed0fe466ffedaa6ad82311c
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

Successfully merging this pull request may close these issues.

6 participants