-
Notifications
You must be signed in to change notification settings - Fork 61
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
Fitness distance steps 4 and 5 are inconsistent with the rest of the spec #933
Comments
Settings dictionaries are not IDL dictionaries but the name settings dictionary still implies that they are dictionaries. A dictionary is commonly defined to be data structure which has keys and values associated to those keys and its main immutable operations are to test the existence of a key and to retrieve the value associated to an existing key.
It depends on the definition of the constrainable property which I think is not properly defined in the spec.
The Terminology section defines Constraints and states that "[f]or each constrainable property, a constraint exists whose name corresponds with the relevant source setting name and capability name". So clearly a constrainable property and a constraint are two different things. Therefore, if a constraint is of type On the other hand, the Constrainable Properties subsection under the MediaStream API section seems to kind of imply that constrainable property and a constraint mean the same. Could the step 4 be clarified to be
Yes, that is one option for the step 4. But please note that the step 5 is in no way limited to constraints of type |
If they're not necessarily IDL dictionaries we shouldn't link to the definition of IDL dictionaries when talking about existence of fields. The implementation of SelectSettings does not require that we have a list of IDL Settings dictionaries that are checked individually (and, for example, the Chromium implementation doesn't follow that approach). It can be the case that support for a particular property is determined by calling some system API and taking a code path that covers multiple dictionaries simultaneously.
I would say that it is. The term constrainable property refers to a name (and type) associated with a constrainable object, which has Capabilities, Constraints and Settings. There is a an IDL template for these concepts. See https://w3c.github.io/mediacapture-main/#interface-definition-0
Since constrainable objects are a pattern they don't have a single explicit type, but it is easy to see that they have an implicit type that is expressed in different ways for settings, constraints and capabilities. Since properties are a pattern and not a specific IDL definition (beyond the name of the property), sometimes the spec uses the setting type when describing the property and sometimes it uses the constraint type. I agree that the spec should be more consistent here, but that doesn't change the fact that the types for settings, capabilities and constraints have to be compatible and you can't have an unrelated type as one of its values.
This doesn't make sense because if you define a type in IDL (and you have to do it for constraints) the only possible values have to correspond to that type. Having a step to cover the case where the value does not correspond to its type is unnecessary/incorrect since that value wouldn't exist.
Let's go for it then.
There are already definitions of fitness distance for strings, doubles and booleans, so I see no need to define anything different for those properties. If these properties should be treated differently to other booleans, strings and doubles they should have a different type and we can make fitness distance definitions for those types. This concept of having the settings dictionary member exist or not just introduces unnecessary complexity based on things that are either impossible or impractical and we should remove it. Why do we need anything beyond the concept of the setting dictionary not supporting the constraint value ? IMO, we should just return to the old fitness distance definition and just add an extra step on how to calculate its value for the (boolean or ConstraintDouble) type. Why do you think we should do anything different from that? |
True.
That is usually also the only practical implementation because the total number of all possible settings dictionaries is the product of number of all possible settings for each property supported by the implementation and the source.
For all positive numeric constraints, the fitness distance is the result of the formula Of course, it is possible to remove the step 5 and integrate it in and repeat at steps 7 and 8.
Wouldn't that basically be a combination of the current steps 4 and 7? |
But the concept we are interested in is whether a dictionary setting satisfies a constraint or not, see Step 2. Thus, having a numeric setting and a boolean value for the constraint doesn't mean that the setting cannot be checked to satisfy the constraint. IIUC, if the setting supports any numeric value it means it satisfies a true value for the constraint.
I see your point. The phrasing looked very confusing to me. I think we should phrase it differently for extra clarity. Something like
Now that I see the intent, I think would keep it, but with the rephrasing above (or similar).
My summary is that we should:
Do you agree? |
Oh, I see now that the step 4 is worded so that it applies both to required boolean (boolean or ConstraintDouble) constraints (a boolean constraint is required in advanced constraint sets as you know) and to ideal boolean (boolean or ConstraintDouble) constraints (a boolean constraint is ideal in the basic constraint set as you know). That should be fixed, I think.
Good.
Mostly, but I still think that the current step 4 has value for the ideal case. While at it, I noticed that the current step 6 has probably too low priority. So would something like below do?
I used your wording for my step 6 but without It would be possible to replase my step 5 with a comprehensive |
One option is also to change the meaning of a pan/tilt/zoom: false constraint. My reading of the current fitness distance algorithm is that But usually it is not question about whether a site wants a pan-capable device or a pan-incapable device but whether a site wants a pan-capable device or not (i.e. it does not matter whether the device is pan-capable or pan-incapable). So it might make more sense to refine the fitness distance algorithm so that This would also simplify the fitness distance algorithm as the |
Not specifying the fitness distance constraint is a standard and effective means of saying "no preference". |
There's no reference to WebIDL in step 4 or 5:
These link to https://infra.spec.whatwg.org/#map-exists, making settings dictionary an ordered map, a common data structure recommended to describe a set of named values (key-value pairs) internally. It's not WebIDL. WebIDL dictionaries also happen to be "used to define an ordered map data type", but that doesn't make this WebIDL. We could perhaps clarify this better in the definition of settings dictionary: - We use the term settings dictionary for the set of values that might be applied as
+ We use the term settings dictionary for the ordered map of values that might be applied as
settings to the object.
Only constraintValue here is from WebIDL (coming from CS), and it can be either a boolean or a ConstraintDouble. But the constrainable property is not WebIDL, and refers instead to the property being constrained by the constrainable pattern. E.g. Its type in the case of pan/tilt/zoom is double. |
Agreed, and thanks for spotting this! I've filed #945 to address this. Regarding the remainder of the conversation (which has gotten long), to scope it down, it seems centered on confusion regarding #766, is that fair?
Here's step 2 today: "If the constraint is required (constraintValue either contains one or more members named 'min', 'max', or 'exact', or is itself a bare value and bare values are to be treated as 'exact'), and the settings dictionary's constraintName member's value does not satisfy the constraint or doesn't exist, the fitness distance is positive infinity." What "satisfies" (before #766) was considered self-evident by language like "The maximum legal value of this property." (opening another issue #946, sigh). #766 added the "or doesn't exist" part, which perhaps requires clarification. Our thinking here was that during getUserMedia especially, the fitness algorithm needs to take into account devices that have pan/tilt/zoom and those that don't. The former devices would have pan/tilt/zoom members in their settings dictionaries, while the latter devices would not. The "or doesn't exist" part should be clear that But I agree it is perhaps unclear that the settings dictionaries of the former devices "satisfy" I'm happy to take language to clarify that sentence. I'd like to keep changes here limited however, as I don't think a major refactor of the algorithm is needed here, or desirable if we can avoid it. |
Please open a separate issue if we wish to rehash |
For me the current behavior is fine and Harald seemed to be against the change (#933 (comment)) so let's not change it and therefore let's not open an issue. |
I think the issue can be resolved if we are more explicit and consistent at defining a few things. Some examples of ambiguity:
All this said, I think the interaction between constraint values and dictionary settings only require the concept of a dictionary setting satisfying a constraint value or not, and letting implementations decide how dictionary settings are implemented and how they satisfy a constraint value. But I agree that defining dictionary settings as instances of MediaTrackSettings is probably the way that requires fewer changes. |
Steps 4 an 5 refer to settings dictionaries as if they were IDL dictionaries, having members. But settings dictionaries are not IDL dictionaries and their internal structure understood to be an implementation detail elsewhere in the spec.
The spec explicitly says
We use the term settings dictionary for the set of values that might be applied as settings to the object.
, which does not necessarily translate to they working like IDL dictionaries with explicit members that might or might not be present.Another problem is that step 4 says
If constraintValue is a boolean, but the constrainable property is not
, which sounds impossible in IDL. I understand that this is intended to support pan/tilt/zoom constraints which accept boolean and numeric values. These constraints are of type (boolean or ConstraintDouble), so the constraint value is compatible with the property. One way to make this clearer is to treat this particular constraint type separately, just like purely numeric constraints are treated separately from purely boolean/enum/string constraints.The text was updated successfully, but these errors were encountered: