Closed Bug 1226475 Opened 9 years ago Closed 5 years ago

Dictionary binding optimization for unions with nested dictionaries has JS-visible side-effects.

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1368949
Tracking Status
firefox45 --- affected

People

(Reporter: jib, Unassigned)

References

()

Details

STR:

With the patch and test case in Bug 1213517 comment 5, from the following input:

    track.applyConstraints({ width: 320, height: 240})

we do log(JSON.stringify(track.getConstraints())) to get the following output:

Expected result:

    {"height":240,"mediaSource":"camera","width":320}

Actual result:

    {"deviceId":{},"facingMode":{},"frameRate":{},"height":240,"mediaSource":"camera","viewportHeight":{},"viewportOffsetX":{},"viewportOffsetY":{},"viewportWidth":{},"width":320}

This seems unobvious to a JS developer and sub-optimal for readability at least.

Only the members defined using ConstrainLong etc. exhibit this behavior. E.g. the plain and default-valued members "browserWindow" and "scrollWithPage" are invisible.

Is this OK? Is there something we can do about it?
Well, so...

Is the return value of getConstraints() semantically equivalent to the thing passed to applyConstraints?  As in, is this a problem in terms of doing applyConstraints(getConstraints()) not doing the right thing, or just a problem in terms of being ugly when you stringify?

If it's the latter, then the basic issue is that WebIDL is mostly designed around dictionaries as _input_, not output.  And for input dictionaries null, undefined, and {} are all equivalent.

For output, dictionaries are converted to their canonical form, which is an object (possibly {}).

Still thinking about input, if you have a dictionary with a dictionary member, currently the spec does in fact distinguish between {} and undefined, I think.  It even does this for an optional dictionary argument in non-trailing position technically.  I sort of consider both of these to be spec bugs; the latter for sure and the former probably.  But then if we want to not distinguish between {} and undefined that means giving dictionary members a default value of {}, and here we are.

What we _could_ do is try to produce a "minimal" form for a dictionary on the way out.  Specifically, something like this:

  1) A dictionary is "elidable" (better-naming-wanted) if it is either empty or only has
     members whose values are themselves elidable dictionaries.
  2) When converting a dictionary to a JS value, skip over members whose values are
     elidable dictionaries, just like we skip over members that are not present.

I'd needinfo heycam, but he's on PTO.  We should try to talk to him about this at Orlando.
(In reply to Boris Zbarsky [:bz] from comment #1)
> Is the return value of getConstraints() semantically equivalent to the thing
> passed to applyConstraints?

The binding code makes it indistinguishable in the implementation, right? So there can't be a difference.

(working through it for myself) AFAICT our binding code has this odd case here where even though the member is optional, it doesn't generate the customary Optional<> member (with .WasPassed() etc.), in the case where the member is either a dictionary or a union containing a dictionary (in the latter case the union instead seems to default to (one of) the dictionary states?) i.e. defaulting to an empty dictionary.

> As in, is this a problem in terms of doing applyConstraints(getConstraints())
> not doing the right thing, or just a problem in terms of being ugly when you stringify?

It does the right thing as far as how we interpret what we have implemented today, though once we implement Bug 987186, I'm a bit curious what the following should mean:

    { echoCancellation: {} }  // is a ConstrainBoolean

Semantically, should this be the same as { echoCancellation: true } or { echoCancellation: false } ?

The spec isn't too clear when it comes to constraints unfortunately, but it is quite clear in a perhaps similar area:

It says I think unequivocally [1] that the following two are equivalent:

    navigator.mediaDevices.getUserMedia({ video: {}, audio: {} })

    navigator.mediaDevices.getUserMedia({ video: true, audio: true })

And indeed this works in Chrome and Firefox http://fiddle.jshell.net/jib1/1pexwkt7/ though this may still count as a spec bug, as the behavior isn't very important for any use-case I know of.

> If it's the latter, then the basic issue is that WebIDL is mostly designed
> around dictionaries as _input_, not output.  And for input dictionaries
> null, undefined, and {} are all equivalent.
> 
> For output, dictionaries are converted to their canonical form, which is an
> object (possibly {}).
> 
> Still thinking about input, if you have a dictionary with a dictionary
> member, currently the spec does in fact distinguish between {} and
> undefined, I think.

Maybe it says so because of this problem we're having here. So isn't this arguably a bug in our binding code then?

> It even does this for an optional dictionary argument
> in non-trailing position technically.  I sort of consider both of these to
> be spec bugs; the latter for sure and the former probably.  But then if we
> want to not distinguish between {} and undefined that means giving
> dictionary members a default value of {}, and here we are.

Right.

> What we _could_ do is try to produce a "minimal" form for a dictionary on
> the way out.  Specifically, something like this:
> 
>   1) A dictionary is "elidable" (better-naming-wanted) if it is either empty
> or only has
>      members whose values are themselves elidable dictionaries.
>   2) When converting a dictionary to a JS value, skip over members whose
> values are
>      elidable dictionaries, just like we skip over members that are not
> present.

Makes sense to me. We still get the invariant of null, undefined, and {} being equivalent, we just choose undefined over {} as the JS representation for this.

> I'd needinfo heycam, but he's on PTO.  We should try to talk to him about
> this at Orlando.

Great!

[1] "Let requestedMediaTypes be the set of media types in constraints with either a dictionary value or a value of "true"."
http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-getusermedia
> The binding code makes it indistinguishable in the implementation, right?

I think so, yes.  Just wanted to make sure that was the case per spec too.  ;)

> (working through it for myself) AFAICT our binding code has this odd case 

Right.  What the binding code has is that all optional dictionaries come with a default value and that value is "empty dictionary" (well, actually null in the impl, but since null has the same behavior as empty dictionary in terms of js-to-c++ conversions...).  So it acts just like:

  DictionarType foo = null;

would if that were valid to write.  Hence no Optional<>.

> Semantically, should this be the same as { echoCancellation: true } or
> { echoCancellation: false } ?

So a ConstrainBoolean is (boolean or ConstrainBooleanParameters) and ConstraintBooleanParameters has "exact" and "ideal" which can both be missing.  {} is the state when they're both missing, which I assume is not the same as either "true" or "false"?  At least that's what it would be in our impl.  I see no ConstrainBoolean in the spec.

> Maybe it says so because of this problem we're having here.

Well, more like no one really thought it through.  Semantically, the intent of dictionaries is to have empty dictionary and missing match.  ;)

> So isn't this arguably a bug in our binding code then?

Or arguably a bug in the spec, yes.

> we just choose undefined over {} as the JS representation for this.

We actually choose "missing", which is not the same thing as undefined.  But yes, they look much alike.
Oh, one other note.  {} vs missing vs expliciy undefined are observably different if someone starts sticking random stuff on Object.prototype.  I don't know that we care.
(In reply to Boris Zbarsky [:bz] from comment #3)
> I see no ConstrainBoolean in the spec.

Here http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-ConstrainBoolean

> Well, more like no one really thought it through.  Semantically, the intent
> of dictionaries is to have empty dictionary and missing match.  ;)

So { video: {} } == { } ?

(yes according to #content)
(Thinking about this some more)

Where it gets tricky is that, in WebRTC, { video: {} } really means "give me an unconstrained video source, not "don't give me a video source". That's how they designed it (I say "they" now since it predates my involvement :))

E.g.

  var getCam = videoConstraints => navigator.mediaDevices({ video: videoConstraints });
  return getCam(getConstraintsFromUser());

If the user specifies at least one constraint, then voila they get video.
If they specify no constraints, then this fails, which seems unintuitive.

Ugly workaround:

  var getCam = videoConstraints => navigator.mediaDevices(videoConstraints? { video: videoConstraints } : true);
  return getCam(getConstraintsFromUser());

So I'm less sure now that {} (an object) can count as missing in all cases.
Forgot 'getUserMedia' there. Pretend it reads:

  var getCam = videoConstraints => navigator.mediaDevices.getUserMedia({ video: videoConstraints });
And I messed up the workaround code too:

  var getCam = vidconstr => navigator.mediaDevices.getUserMedia({ video: vidconstr? vidconstr : true });
  return getCam(getConstraintsFromUser());
bz resolved my concern in comment 6 by pointing out that WebRTC seems saved by having default values [1]:

  dictionary MediaStreamConstraints {
    (boolean or MediaTrackConstraints) video = false;
    (boolean or MediaTrackConstraints) audio = false;
  };

With a default of false rather than {}, our implementation can detect someone passing in { video: {} } and interpret that to mean "gimme unconstrained video".

The actual track constraints (width, height etc.) never have default values.

So I'm onboard again, with the notion that e.g. { video: { width: {} } } can be elided to { video: {} }.

(The fact that we have unions at two different levels of depth will no doubt continue to confuse discussions)

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#mediastreamconstraints
Component: DOM → DOM: Core & HTML

It looks to me like bug 1368949 fixed this, as reported, right?

Depends on: 1368949
Flags: needinfo?(jib)

\o/

Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jib)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.