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

Behavior for getDisplayMedia() and getDisplayMedia({}) should be the same #65

Closed
foolip opened this issue Aug 20, 2018 · 19 comments
Closed
Assignees

Comments

@foolip
Copy link
Member

foolip commented Aug 20, 2018

https://w3c.github.io/mediacapture-screen-share/#navigator-additions says:

If the constraints argument is omitted, a default value containing a single video attribute set to true is assumed.

However, this doesn't match the MediaStreamConstraints definition in https://w3c.github.io/mediacapture-main/#mediastreamconstraints:

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

Here, video has a default value of false. Web IDL says in https://heycam.github.io/webidl/#dfn-optional-argument-default-value:

If the type of an argument is a dictionary type or a union type that has a dictionary as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified.

In other words, per Web IDL, in this case, constraints is considered to have a default value of {}, which means that getDisplayMedia() and getDisplayMedia({}) will be indistinguishable. getDisplayMedia({ audio: false }) should also have the same behavior if audio has a default value of false, which it does here.

The most straightforward fix for this would be to make the IDL match the wanted behavior, e.g.:

dictionary DisplayMediaConstraints {
             boolean video = true;
             boolean audio = false;
};

As a very unfortunate side effect getDisplayMedia({ video: false }) and getDisplayMedia({ video: undefined }) will not mean the same thing even though undefined is a falsy value, but I don't see another great solution.

@uysalere
Copy link
Contributor

Thanks for posting this in detail foolip@. Note than creating a new dictionary type DisplayMediaConstraints will differentiate the usage of getUserMedia() and getDisplayMedia() further. i.e. getUserMedia() returns an exception whereas getDisplayMedia() returns a MediaStream. Alternatively we can make getDisplayMedia() work in a similar way, such that we return an exception when the constraints argument is omitted.

@foolip
Copy link
Member Author

foolip commented Aug 20, 2018

It looks like a key difference will be there no matter what the IDL looks like, namely that getUserMedia() with no argument should fail because neither audio or video was requested, while getDisplayMedia() should behave as getDisplayMedia({ video: true }) and succeed.

The only way to avoid forking the IDL dictionary would be to remove the default value and handle a missing video member explicitly in both specs, but differently.

@foolip
Copy link
Member Author

foolip commented Aug 20, 2018

Unfortunately, to make getDisplayMedia({ audio: true }) not request video the logic has to depend on the precense of the audio member.

Perhaps it's better to recast this behavior not as a default value, but as the fallback behavior for when audio and video are both false? Written as an algorithm, that would be quite easy to express at least.

@uysalere
Copy link
Contributor

uysalere commented Aug 20, 2018

while getDisplayMedia() should behave as getDisplayMedia({ video: true }) and succeed.

My suggestion was that, getDisplayMedia() should also fail and not be a special case. That would be more consistent.

Perhaps it's better to recast this behavior not as a default value, but as the fallback behavior for when audio and video are both false? Written as an algorithm, that would be quite easy to express at least.

getDisplayMedia({ video: false }) returning a video stream does not sound right.

@guidou
Copy link
Collaborator

guidou commented Aug 21, 2018

I agree that the most straightforward change is to make getDisplayMedia() fail, just like getUserMedia() does.

@foolip
Copy link
Member Author

foolip commented Aug 21, 2018

That would be a risky change for Edge which has already shipped navigator. getDisplayMedia(). Some quick testing confirms that it shows the selection prompt, i.e. it probably works per spec.

However, I took a look in httparchive, and there are very few matches for 'getDisplayMedia' (13) and no matches for 'navigator.getDisplayMedia'. This suggests that there isn't much of a web compat contraint for this API yet. However, it'd still be best to poke from the Edge team about such a change.

@foolip
Copy link
Member Author

foolip commented Aug 21, 2018

(The 13 matches were things like https://shared.nordstrom.com/rfx/v-1.0.6603.39355/Desktop/RfxPluginContent.js, no real uses of this API that I could see.)

@alvestrand
Copy link
Contributor

ping @aboba for Edge info

@alvestrand
Copy link
Contributor

May be OK now.

@jan-ivar
Copy link
Member

jan-ivar commented Sep 6, 2018

This should be fixed now with #73.

@jan-ivar
Copy link
Member

I'm reopening this issue since the prose added in #73 did not get around the fact that with WebIDL defaults, there's no way to distinguish {video: false, audio: false} from {}, as mentioned in #85.

@jan-ivar jan-ivar reopened this Nov 28, 2018
@jan-ivar
Copy link
Member

jan-ivar commented Nov 28, 2018

Such arguments are always considered to have a default value of an empty dictionary

To clarify what I think @foolip is saying—because I tripped over it—an "empty dictionary" here isn't actually empty, but populated with its default values, which means all the following calls are indistinguishable:

getDisplayMedia()
getDisplayMedia({})
getDisplayMedia({video: false})
getDisplayMedia({audio: false})
getDisplayMedia({video: false, audio: false}) // <-- what implementation prose sees

Having our prose interpret all of these as {video: true} would be confusing.

I agree with others here that we should let getDisplayMedia fail here instead.

@jan-ivar
Copy link
Member

Alternatively, something like what @foolip proposed might work:

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

... with prose to throw TypeError on video: false, since audio-only capture is disallowed.

@martinthomson
Copy link
Member

That seems like the best outcome. It's the expected mode. Then you only need to set the video option if you have constraints, or you want audio, both being somewhat exceptional.

@foolip
Copy link
Member Author

foolip commented Nov 29, 2018

If { video: false } just shouldn't work, then another option is to just not have a boolean as part of the definition at all, so:

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

@jan-ivar
Copy link
Member

jan-ivar commented Nov 29, 2018

@foolip yes but then wouldn't {video: true} and {video: true, audio: true} produce TypeError?

I think people would expect those to work, for symmetry alone. People would then need to use {video: {}, audio: true} or {audio: true} to get unconstrained capture.

@jan-ivar
Copy link
Member

jan-ivar commented Nov 29, 2018

I mean, if we're dropping symmetry, I'd argue for getDisplayMedia({display: {}, displayAudio: true), and getUserMedia({camera: true, microphone: true}).

@foolip
Copy link
Member Author

foolip commented Nov 30, 2018

Sure, similarity to getUserMedia and not rejecting harmless attempts to opt in to video seems okay :)

@alvestrand
Copy link
Contributor

Fixed by #92

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