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

Constraints for Captured Display Surfaces should move to MediaTrackSettings #66

Closed
uysalere opened this issue Aug 24, 2018 · 13 comments
Closed
Assignees

Comments

@uysalere
Copy link
Contributor

The spec currently allows constraints to observe the properties of the selected display surface. These are only observable, such that they cannot be changed after returned.

The way it is working right now, they would be better defined under getSettings() and partial dictionary should be defined on MediaTrackSettings. The aim here seem to be to allow the application to query the current settings of the object's constrainable properties, which getSettings() does exactly. Putting these as read-only exceptional cases under getConstraints() seems to overlap with what getsettings() should do.

@alvestrand
Copy link
Contributor

What properties of the selected display surface are you referring to?
Note that changes to the spec need to make sense after #49 is resolved.

@jan-ivar
Copy link
Member

The constrainable pattern appears to force our hand here. It has no concept of read-only constrainable properties, which I think means that if we want to humor that pattern, we must define these as constrainable properties (and ignore/throw OverconstrainedError on changes to them), for POLA.

That said, good observation that we're not extending MediaTrackSettings, which we should. We need to extend all 4 dictionaries mentioned in the extensibility section.

@uysalere
Copy link
Contributor Author

What properties of the selected display surface are you referring to?

I am referring to the new constraints defined in section 5.3. "Since the source of media cannot be changed after a MediaStreamTrack has been returned and constraints do not affect the selection of display surfaces, these constraints cannot be changed by an application." With this restriction in place, I believe it would be more appropriate to define them as "Setting" which is read-only.
https://w3c.github.io/mediacapture-main/getusermedia.html#terminology

Note that changes to the spec need to make sense after #49 is resolved.

The new constraints defined in section 5.3 are not allowed within those constraints.

That said, good observation that we're not extending MediaTrackSettings, which we should. We need to extend all 4 dictionaries mentioned in the extensibility section.

I agree. Should we also mention which methods should user expect to fill these properties? i.e., getSettings(), getCapabilities() should but getConstraints() should not.

@jan-ivar
Copy link
Member

more appropriate to define them as "Setting" which is read-only

That's not a separate thing. "A setting refers to the immediate, current value of the source's constrainable properties".

I think "read-only" in this context means OverconstrainedError.

@jan-ivar
Copy link
Member

E.g. like we do for "setting deviceId constraint can only cause the resulting MediaStreamTrack to become overconstrained. "

@uysalere
Copy link
Contributor Author

That's not a separate thing. "A setting refers to the immediate, current value of the source's constrainable properties". I think "read-only" in this context means OverconstrainedError.

Correct me if I got this wrong. What you are suggesting to still add these properties as a partial dictionary on MediaTrackConstraints, but always return OverconstrainedError when these properties are set. MediaTrackSupportedConstraints should include these properties as well, because they can be set. I was thinking it might be confusing for the user, as we are defining but not actually supporting them. Unless it is necessary for this partial dictionary to be consistent on all these dictionaries.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 29, 2018

but always return OverconstrainedError when these properties are set

Sorry, I should have said they will be ignored, because they're not required (min, max or exact) constraints.

E.g. track.applyConstraints({logicalSurface: true}) would succeed and be indistinguishable from track.applyConstraints({}) (OverconstrainedError would only happen if we used a max constraint, but I can't think of an example, assuming #73 is merged).

MediaTrackSupportedConstraints should include these properties as well, because they can be set

Well, because they are "supported", but yes. The current definition of getSupportedConstraints does not explicitly guarantee that they do something when set, only that the browser implements those constrainable properties (not that they're effectively actually constrainable).

I was thinking it might be confusing for the user, as we are defining but not actually supporting them.

Would it be more confusing than deviceId? We "support" them, they're just defined to be unchangeable, much like deviceId (which already appears in getSupportedConstraints()).

@jan-ivar
Copy link
Member

I see three options here for logicalSurface and displaySurface:

  1. Implement them as no-op constraints (current spec)
  2. Invent new non-constrainable constrainable property that only shows up in getSettings()
  3. Evict these from getSettings() and force a subclass of MediaStreamTrack with read-only args

@uysalere
Copy link
Contributor Author

uysalere commented Aug 29, 2018

Thanks for the clarification. Considering #62 as well, I think defining these properties as partial dictionary on all 4 dictionaries as you suggested earlier makes more sense now.

  • getSettings() and getCapabilities() should return the properties of MediaStreamTrack user selected. Note that there wouldn't really be a range on getCapabilities() elements. i.e., {displaySurface: monitor}.
  • getSupportedConstraints() result should include these properties.
  • getConstraints() should return the only the constraints that user actually set on these properties. i.e. track.applyConstraints({logicalSurface: true}) should return {logicalSurface: true} whereas track.applyConstraints({}) should be empty. This way we wouldn't have any exceptional case for getDisplayMedia().

@alvestrand
Copy link
Contributor

#75 solves a lot, @jan-ivar to jump on the rest.

@henbos henbos assigned jan-ivar and unassigned uysalere Dec 20, 2018
@guest271314
Copy link
Contributor

@jan-ivar

Well, because they are "supported", but yes. The current definition of getSupportedConstraints does not explicitly guarantee that they do something when set, only that the browser implements those constrainable properties (not that they're effectively actually constrainable).

That is apparently the case for Firefox, where getSupportedConstraints() includes viewportOffsetX and viewportOffsetY, though setting those properties and values at a MediaStreamTrack of kind "video" following executing getDisplayMedia() when trying to resolve #105 has no effect, and the set properties are not set at the object returned by getSettings(). Essentially making the call to applyConstraints() null and void.

Can the various stream and media capture API specifications clarify what constraints are and are not actually applicable and capable of being set relevant to the respective media stream and capture APIs?

@guest271314
Copy link
Contributor

@jan-ivar Not entirely sure if attempting to set viewportOffsetX and viewportOffsetY at MediaStreamTrack within fulfilled MediaStream after getDisplayMedia() is executed is the issue, though doing so occasionally (more than twice) freezes the entire operating system (save for cursor, which is not able to click on an application) at *nix https://bugzilla.mozilla.org/show_bug.cgi?id=1549095.

@eladalon1983
Copy link
Member

This issue has not seen any activity in a long time. It appears that, important though it is, it is not important enough to merit action. I am closing this. Feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants