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

WebIDL types needed in Constrainable application #116

Closed
dontcallmedom opened this issue Jan 9, 2015 · 16 comments
Closed

WebIDL types needed in Constrainable application #116

dontcallmedom opened this issue Jan 9, 2015 · 16 comments
Assignees

Comments

@dontcallmedom
Copy link
Member

Initially reported in bugzilla by Harald Alvestrand 2014-08-25 10:33:22 UTC

From Jan-Ivar, pointing out several issues with the Constrainable pattern as applied to MediaTrack:

The spec still leaves several types to be inferred by the implementer around constraints:

  1. In [1]:

    partial interface MediaDevices {
    static Dictionary getSupportedConstraints (DOMString kind);

"Dictionary" is not defined. To implement this, we'd need something like:

dictionary SupportedMediaTrackConstraintSet {
boolean width;
boolean height;
boolean aspectRatio;
boolean frameRate;
boolean facingMode;
boolean volume;
boolean sampleRate;
boolean sampleSize;
boolean echoCancelation;
boolean sourceId;
boolean groupId;
// basically a boolean rehash of MediaTrackConstraintSet
};

static SupportedMediaTrackConstraintSet getSupportedConstraints (DOMString kind);

or simply:

static MediaTrackConstraintSet getSupportedConstraints (DOMString kind);

since UAs can return non-zero values in MediaTrackConstraintSet just fine!

  1. In [2]:

    interface MediaStreamTrack : EventTarget {
    ...
    Capabilities getCapabilities ();
    MediaTrackConstraints getConstraints ();
    Settings getSettings ();

Capabilities and Settings are not defined in this use of the Constrainable pattern. To implement this, we'd need something like:

dictionary MediaTrackSettingSet {
long width;
long height;
double aspectRatio;
double frameRate;
VideoFacingMode facingMode;
double volume;
long sampleRate;
long sampleSize;
boolean echoCancelation;
DOMString sourceId;
DOMString groupId;
// basically a bare-value rehash of MediaTrackConstraintSet
};

interface MediaStreamTrack : EventTarget {
...
MediaTrackConstraintSet getCapabilities ();
MediaTrackConstraints getConstraints ();
MediaTrackSettingSet getSettings ();

or simply:

interface MediaStreamTrack : EventTarget {
...
MediaTrackConstraintSet getCapabilities ();
MediaTrackConstraints getConstraints ();
MediaTrackConstraintSet getSettings ();

since UAs can return bare values in MediaTrackConstraintSet just fine!

  1. In the Constrainable Pattern in [3]:

    Capabilities are dictionary containing one or more key-value pairs, where each key must be a constrainable property defined in the registry, and each value must be a subset of the set of values defined for that property in the registry. The exact syntax of the value expression depends on the type of the property but is of type ConstraintValues . The Capabilities dictionary specifies the subset of the constrainable properties and values from the registry that the UA supports.

Typo: "Capabilities are dictionary" -> "Capabilities is a dictionary". Importantly, all capabilities are in one dictionary.

The type "ConstraintValues" is not defined anywhere. Do we mean "members of ConstraintSet" here? If so, that makes Capabilities of type ConstraintSet, yet we're leaving that deduction to the reader. IMHO prose and examples are no substitute for definitions.

  1. In the Constrainable Pattern in [4]:

    A Setting is a dictionary containing one or more key-value pairs. It must contain each key returned in getCapabilities(). There must be a single value for each key and the value must a member of the set defined for that property by capabilities(). The Settings dictionary contains the actual values that the UA has chosen for the object's Capabilities. The exact syntax of the value depends on the type of the property.

Typo: "A Setting is a dictionary" -> "Settings is a dictionary". Importantly, all settings are in one dictionary.
Typo: "the value must a member" -> "the value must be a member"

We're leaving the reader to deduce that Settings is a bare-values-only subset of ConstraintSet. IMHO prose and examples are no substitute for definitions.

  1. In the Constrainable Pattern in [5]:

    typedef Dictionary ConstraintSet;

"Dictionary" is not defined, and even if it were, it wouldn't have the right members, so a typedef infers the wrong thing. We could say:

dictionary ConstraintSet { /* members */ };

  1. In [6]:

    dictionary MediaTrackConstraintSet {
    ConstrainLong width;
    ConstrainLong height;
    ConstrainDouble aspectRatio;
    ConstrainDouble frameRate;
    ConstrainVideoFacingMode facingMode;
    ConstrainDouble volume;
    ConstrainLong sampleRate;
    ConstrainLong sampleSize;
    boolean echoCancelation;
    ConstrainDOMString sourceId;
    DOMString groupId;
    };

groupId seems to have the wrong type. Why not ConstrainDOMString groupId ?

.: Jan-Ivar :.

[1] http://dev.w3.org/2011/webrtc/editor/archives/20140817/getusermedia.html#mediadevices-interface-extensions
[2] http://dev.w3.org/2011/webrtc/editor/archives/20140817/getusermedia.html#media-stream-track-interface-definition
[3] http://dev.w3.org/2011/webrtc/editor/archives/20140817/getusermedia.html#capabilities
[4] http://dev.w3.org/2011/webrtc/editor/archives/20140817/getusermedia.html#settings
[5] http://dev.w3.org/2011/webrtc/editor/archives/20140817/getusermedia.html#constraints
[6] http://dev.w3.org/2011/webrtc/editor/archives/20140817/getusermedia.html#dictionary-mediatrackconstraints-members

@dontcallmedom
Copy link
Member Author

Comment by Jan-Ivar Bruaroey [:jib] 2014-09-23 18:31:48 UTC

Pull request: #22

@dontcallmedom
Copy link
Member Author

Comment by Stefan Hakansson LK 2014-10-09 14:14:39 UTC

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)

Pull request: #22

That one has been removed.

New PR's: #36 and #37

@dontcallmedom
Copy link
Member Author

Comment by Stefan Hakansson LK 2014-10-28 08:14:17 UTC

PR #36 merged in http://w3c.github.io/mediacapture-main/archives/20141027/getusermedia.html

@dontcallmedom
Copy link
Member Author

Comment by Harald Alvestrand 2014-10-30 18:40:50 UTC

WG decision: The final disposition is to leave this to editorial discretion (under the constraint that the result is IDL valid).

@jan-ivar
Copy link
Member

I just wanted to bring up - in case it's been overlooked - that there was some support on the list for making "capabilities" the primary focus of our gUM narrative, not "constraints" [1]. To re-summarize: When we talk about "width", "height", "frameRate" etc. we historically think of them as of type "constraint" for some reason. This is bad, because a constraint is a property of a caller, which only makes sense in the narrow context of organizing demands. This is too narrow of a context to cover all the areas we need to talk about "width", "height", "frameRate" etc. in. By refocusing on capabilities, we'll avoid silly mind-benders like "is volume a constraint?", when the right question is "is volume an available capability?" - The fact that all supported capabilities can be constrained on, becomes a detail.

[1] https://lists.w3.org/Archives/Public/public-media-capture/2014Nov/0004.html

@alvestrand
Copy link
Contributor

I believe the current language in the spec talks about "constrainable properties" - the property belongs to the track, the constraint is set by the user. The main thrust of the "constraint" language is to describe the constraint mechanism - this is as it should be.

Dan is working on making this language clearer.

@jan-ivar
Copy link
Member

More narrowly, I think they're "constrainable capabilities" of the track, as one abstraction of the source. Capability ⊆ Property.

@stefhak
Copy link
Contributor

stefhak commented Jan 28, 2015

In line with what we decided at TPAC (i.e. "The final disposition is to leave this to editorial discretion (under the constraint that the result is IDL valid)." I label this issue "Editorial". I also assign it to Dan.

@alvestrand alvestrand assigned adam-be and unassigned burnburn Feb 5, 2015
@dontcallmedom
Copy link
Member Author

I think this can be closed now once the last webidl fix lands dontcallmedom@410ff73

@adam-be
Copy link
Member

adam-be commented Mar 10, 2015

The typos (mentioned above) seem to be fixed, and PR#36 and PR#37 (constraint_structs) seem to have fixed the majority of the stuff mentioned here. It's indeed the return value of the getSupportedConstraints() that's left to be addressed.

@adam-be
Copy link
Member

adam-be commented Mar 10, 2015

Thanks for PR Dom. I could live with reusing the MediaTrackSettings dictionary as the return value for getSupportedConstraints(), but we have recently gone in the direction of using specific structures for getCapabilites/Settings/Constraints(). I'm holding on for others to comment.

@dontcallmedom
Copy link
Member Author

if a 3rd structure is preferred, I would suggest using an array of values ["width","height",…] rather than a dictionary; in fact, I'll make a new pull request toward that so that we can pick or the other.

dontcallmedom added a commit to dontcallmedom/mediacapture-main that referenced this issue Mar 10, 2015
@adam-be
Copy link
Member

adam-be commented Mar 10, 2015

Then the check would be: supportedConstraints.indexOf("name") != -1

That works too.

@jan-ivar
Copy link
Member

Ugh. A dictionary guarantees non-recurrence of members, whereas an array does not. A dictionary test is a hash lookup, whereas an array is linear lookup. I think:

if (getSupportedConstraints().name)

is better semantically, syntactically and performance wise. Those who wish to do array math can always do:

if (Object.keys(getSupportedConstraints()).indexOf("name") != -1)

@jan-ivar
Copy link
Member

Nevermind that the API is literally about what keys a dictionary supports.

@jan-ivar
Copy link
Member

(@dontcallmedom asked me to move this comment here)

I hate to say it but, since the method is called getSupportedConstraints rather than getSupportedCapabilities (which I voted for!) or getSupportedSettings, returning a MediaTrackConstraintSet might have a slight advantage in my mind. On a technical note, choosing that would also let us conveniently return truthy values like this:

{ width: {}, height: {}, frameRate: {}, facingMode: {} ...  }

without getting into what the types involved are. That's perhaps a minor issue though, so maybe we'll just keep this in mind as we develop whichever PR we go for.

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

Successfully merging a pull request may close this issue.

6 participants