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

getUserMedia algorithm should be made more synchronous #174

Closed
youennf opened this Issue May 28, 2015 · 19 comments

Comments

Projects
None yet
4 participants
@youennf

youennf commented May 28, 2015

As per http://w3c.github.io/mediacapture-main/archives/20150324/getusermedia.html#dom-mediadevices-getusermedia, step3 of the getUserMedia implementation is done asynchronously.

But steps 3.1 to 3.5 could be run synchronously.
In particular, the implementation should be able to return already rejected promises, since JS callbacks will be called asynchronously anyway.

Step 3.6 should also be started synchronouly, although actual prompting and user decision will usually happen asynchronously.

Would it be possible to rewrite the description to take that into account?

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar May 28, 2015

Contributor

To what end?

In practice there are inherent reasons to get a list of media devices in parallel as to not take up time on the main JS thread (potentially querying devices). So this matches at least one implementation.

3.1. is just a declaration for 3.2. so this seems to follow general programming best practice of defining variables as close to their use as possible.

3.6. avoids asking users for devices they don't have (3.2.) Some implementations also let the user select devices in their permission prompt (firefox).

3.5. Seems related to 3.6. and as both optionally jump to 3.11. Moving 3.5. to the synchronous part would entail duplicating 3.11.

Contributor

jan-ivar commented May 28, 2015

To what end?

In practice there are inherent reasons to get a list of media devices in parallel as to not take up time on the main JS thread (potentially querying devices). So this matches at least one implementation.

3.1. is just a declaration for 3.2. so this seems to follow general programming best practice of defining variables as close to their use as possible.

3.6. avoids asking users for devices they don't have (3.2.) Some implementations also let the user select devices in their permission prompt (firefox).

3.5. Seems related to 3.6. and as both optionally jump to 3.11. Moving 3.5. to the synchronous part would entail duplicating 3.11.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf May 28, 2015

As I read it, the spec rules out implementation that could reject/resolve the promise synchronously.
That is unfortunate.

In WebKit, step 3.2 is about to be implemented synchronously.
From the JS end-user prospective, this does not matter, the promise will call the reject callback asynchronously anyway.

If we were to follow closely the spec (or my interpretation of it), we would:

  1. schedule an asynchronous call in which we would:
    1.a check that the dictionary is empty.
    1.b. reject the promise.
  2. schedule another asynchronous call to actually call the promise reject callback.
    This seems like unnecessary overhead.

The same may be applied to step 3.5. If the browser knows already that the getUserMedia call will fail, why schedule an asynchronous call to reject the promise?

The spec should allow that behavior.

youennf commented May 28, 2015

As I read it, the spec rules out implementation that could reject/resolve the promise synchronously.
That is unfortunate.

In WebKit, step 3.2 is about to be implemented synchronously.
From the JS end-user prospective, this does not matter, the promise will call the reject callback asynchronously anyway.

If we were to follow closely the spec (or my interpretation of it), we would:

  1. schedule an asynchronous call in which we would:
    1.a check that the dictionary is empty.
    1.b. reject the promise.
  2. schedule another asynchronous call to actually call the promise reject callback.
    This seems like unnecessary overhead.

The same may be applied to step 3.5. If the browser knows already that the getUserMedia call will fail, why schedule an asynchronous call to reject the promise?

The spec should allow that behavior.

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar May 28, 2015

Contributor

Is it important to optimize failure?

As for ruling out implementations, the spec says:

Conformance requirements phrased as algorithms or specific steps may be implemented in any manner, so long as the end result is equivalent.

Contributor

jan-ivar commented May 28, 2015

Is it important to optimize failure?

As for ruling out implementations, the spec says:

Conformance requirements phrased as algorithms or specific steps may be implemented in any manner, so long as the end result is equivalent.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf May 29, 2015

I am not sure what exactly means "so long as the end result is equivalent".

The order in which resolved/rejected promise callbacks are called is guaranteed to be unambiguous.

Implementing step 3.2 synchronously or asynchronously is observable from JS, although hopefully usually irrelevant for users.

youennf commented May 29, 2015

I am not sure what exactly means "so long as the end result is equivalent".

The order in which resolved/rejected promise callbacks are called is guaranteed to be unambiguous.

Implementing step 3.2 synchronously or asynchronously is observable from JS, although hopefully usually irrelevant for users.

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar May 29, 2015

Contributor

Order relative to what? Whenever I see the phrase "Run the following steps asynchronously" I generally interpret that to mean that any timing guarantees are off. I could be wrong of course.

I don't see the significance of this optimization, therefore I can't argue that the spec prohibits it at the same time.

Contributor

jan-ivar commented May 29, 2015

Order relative to what? Whenever I see the phrase "Run the following steps asynchronously" I generally interpret that to mean that any timing guarantees are off. I could be wrong of course.

I don't see the significance of this optimization, therefore I can't argue that the spec prohibits it at the same time.

@adam-be

This comment has been minimized.

Show comment
Hide comment
@adam-be

adam-be May 29, 2015

Member

Looking some older versions of this algorithm, before we had promises, we used to have the prerequisite checks done synchronously. And the "Error Task", which may be considered unnecessary today, scheduled a task to fire the error callback. @youennf, would moving 3.1 and 3.2 out of the async section make the algorithm better in your opinion?

As @jan-ivar, says above, we can't get away from resolving/rejecting promises from the "background" since the result depends on devices being probed on the system.

Member

adam-be commented May 29, 2015

Looking some older versions of this algorithm, before we had promises, we used to have the prerequisite checks done synchronously. And the "Error Task", which may be considered unnecessary today, scheduled a task to fire the error callback. @youennf, would moving 3.1 and 3.2 out of the async section make the algorithm better in your opinion?

As @jan-ivar, says above, we can't get away from resolving/rejecting promises from the "background" since the result depends on devices being probed on the system.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf May 29, 2015

@jan-ivar, here is an example to clarify what I mean by observable:

getUserMedia({}).catch(function() { console.log("gum rejected") })
new Promise(function(resolve, reject) { reject() }).catch(function() { console.log("promise rejected") })

The second promise callback will be called before or after the first promise callback depending on how is implemented step 3.2.

Is the spec prescribing a specific order for the example above?
I tend to think so but maybe I am too restrictive in my interpretation of the spec.

More generally, cases like user prompting or system calls seem different to straightforward value/state checking. Some other promise-based APIs (streams API, AudioContext...) may be useful to check.

@adam-be, yes, moving 3.1 and 3.2 out of the async seems like an improvement to me.
But I am not pushing hard for that yet. I am more looking at clarifying the intent of the spec.

youennf commented May 29, 2015

@jan-ivar, here is an example to clarify what I mean by observable:

getUserMedia({}).catch(function() { console.log("gum rejected") })
new Promise(function(resolve, reject) { reject() }).catch(function() { console.log("promise rejected") })

The second promise callback will be called before or after the first promise callback depending on how is implemented step 3.2.

Is the spec prescribing a specific order for the example above?
I tend to think so but maybe I am too restrictive in my interpretation of the spec.

More generally, cases like user prompting or system calls seem different to straightforward value/state checking. Some other promise-based APIs (streams API, AudioContext...) may be useful to check.

@adam-be, yes, moving 3.1 and 3.2 out of the async seems like an improvement to me.
But I am not pushing hard for that yet. I am more looking at clarifying the intent of the spec.

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar May 29, 2015

Contributor

Is the spec prescribing a specific order for the example above?

TL;DR; I certainly hope not. In my experience, the spec tends to use MUST when giving guarantees.

In my view, a specification is a grand simplification of an actual implementation.

For instance, I'm working on adding the deviceId constraint to getUserMedia on Firefox right now, and here's what I'm looking at having it do to accomplish what it needs to do:

  • Just in case a deviceId constraint is used, first call (the internal equivalent of) mediaDevices.enumerateDevices() to get all persistent device ids anonymized for the current origin (essentially one-way-hashes of raw device-ids with an origin-specific key).
    • This is a cross-process call to the chrome process to access the browser's profile-dir (we must be on chrome process's main thread to access it), then over to a second thread there to do file io to read the persistent origin-key list from disk, then message back an existing or new key.
    • Once back on the content process, dispatch over to the media thread to read the raw device list, and back (because our crypto library has trouble off-main-thread).
    • Back on main thread, hash raw device-ids with the origin-key, to produce an origin-relevant list.
  • With origin-relevant list in hand, dispatch yet again to media thread where constraints are applied.
  • Then from there, notify the permission doorhanger code which is written in JavaScript (main thread).
  • Then if the end-user ever responds to any of the choices (or persitent permissions have been granted), communicate back to media thread which dispatches results back to main thread where promises are resolved/rejected.

Any definition of "Run the following steps asynchronously" stricter than "please do something equivalent to this sometime later" would disallow much of this. So no, I would not assume much about timing here.

Contributor

jan-ivar commented May 29, 2015

Is the spec prescribing a specific order for the example above?

TL;DR; I certainly hope not. In my experience, the spec tends to use MUST when giving guarantees.

In my view, a specification is a grand simplification of an actual implementation.

For instance, I'm working on adding the deviceId constraint to getUserMedia on Firefox right now, and here's what I'm looking at having it do to accomplish what it needs to do:

  • Just in case a deviceId constraint is used, first call (the internal equivalent of) mediaDevices.enumerateDevices() to get all persistent device ids anonymized for the current origin (essentially one-way-hashes of raw device-ids with an origin-specific key).
    • This is a cross-process call to the chrome process to access the browser's profile-dir (we must be on chrome process's main thread to access it), then over to a second thread there to do file io to read the persistent origin-key list from disk, then message back an existing or new key.
    • Once back on the content process, dispatch over to the media thread to read the raw device list, and back (because our crypto library has trouble off-main-thread).
    • Back on main thread, hash raw device-ids with the origin-key, to produce an origin-relevant list.
  • With origin-relevant list in hand, dispatch yet again to media thread where constraints are applied.
  • Then from there, notify the permission doorhanger code which is written in JavaScript (main thread).
  • Then if the end-user ever responds to any of the choices (or persitent permissions have been granted), communicate back to media thread which dispatches results back to main thread where promises are resolved/rejected.

Any definition of "Run the following steps asynchronously" stricter than "please do something equivalent to this sometime later" would disallow much of this. So no, I would not assume much about timing here.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf May 29, 2015

OK, so I am mostly fine with the intent of the spec. Would it be possible to clarify this?

Also, although I do not feel too strongly on that, mandating step 3.2 synchronous makes sense to me.

First, a natural mapping to old-style callback APIs throwing exceptions is for promise-based APIs to return a rejected promise.

Second, is there any good reason to implement step 3.2 asynchronously? Will Firefox actually do that? Consistency is good. Fingerprinting on a simple test like that is not (weak argument I know but anyway...).

youennf commented May 29, 2015

OK, so I am mostly fine with the intent of the spec. Would it be possible to clarify this?

Also, although I do not feel too strongly on that, mandating step 3.2 synchronous makes sense to me.

First, a natural mapping to old-style callback APIs throwing exceptions is for promise-based APIs to return a rejected promise.

Second, is there any good reason to implement step 3.2 asynchronously? Will Firefox actually do that? Consistency is good. Fingerprinting on a simple test like that is not (weak argument I know but anyway...).

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar May 30, 2015

Contributor

First, a natural mapping to old-style callback APIs throwing exceptions is for promise-based APIs to return a rejected promise.

Is the significance of returning an "already rejected promise" that it's immediately observable in the JS debugger? If so then I think I'm coming around.

I think the point you're making here is that 3.2. amounts to trivial argument validation not entirely unlike what WebIDL binding code performs automatically if passed 1 instead of {} (returning an already-rejected promise with TypeError). E.g. a future version of WebIDL may even grow a way to express [AtLeastOneMemberPresent] or some extended attribute like that, in which case we'd wish we'd made 3.2. synchronous.

In hindsight, we should perhaps even have considered TypeError instead of NotSupportedError, since the latter is somehwat misleading. Yes { lasers: true } resulting in NotSupportedError sounds good, but { lasers: true, audio: true } succeeds without error.

Second, is there any good reason to implement step 3.2 asynchronously?

As @adam-be mentioned, I think it moved to stay with the structure of the algorithm, to keep the number of steps low, which has some value. But the "go to step x" pattern seemed more of a win with the old callbacks than with the new less verbose "reject p" language. Maybe it's time to rethink that.

Contributor

jan-ivar commented May 30, 2015

First, a natural mapping to old-style callback APIs throwing exceptions is for promise-based APIs to return a rejected promise.

Is the significance of returning an "already rejected promise" that it's immediately observable in the JS debugger? If so then I think I'm coming around.

I think the point you're making here is that 3.2. amounts to trivial argument validation not entirely unlike what WebIDL binding code performs automatically if passed 1 instead of {} (returning an already-rejected promise with TypeError). E.g. a future version of WebIDL may even grow a way to express [AtLeastOneMemberPresent] or some extended attribute like that, in which case we'd wish we'd made 3.2. synchronous.

In hindsight, we should perhaps even have considered TypeError instead of NotSupportedError, since the latter is somehwat misleading. Yes { lasers: true } resulting in NotSupportedError sounds good, but { lasers: true, audio: true } succeeds without error.

Second, is there any good reason to implement step 3.2 asynchronously?

As @adam-be mentioned, I think it moved to stay with the structure of the algorithm, to keep the number of steps low, which has some value. But the "go to step x" pattern seemed more of a win with the old callbacks than with the new less verbose "reject p" language. Maybe it's time to rethink that.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf May 30, 2015

Yes, you described the point well.

Agreeing also on inlining "reject promise with...", more readable and closer to other specs wording.

youennf commented May 30, 2015

Yes, you described the point well.

Agreeing also on inlining "reject promise with...", more readable and closer to other specs wording.

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar Jun 5, 2015

Contributor

@adam-be - in case it got lost above, I do support moving the 3.2. step to the synchronous part.

Contributor

jan-ivar commented Jun 5, 2015

@adam-be - in case it got lost above, I do support moving the 3.2. step to the synchronous part.

@adam-be

This comment has been minimized.

Show comment
Hide comment
@adam-be

adam-be Jun 9, 2015

Member

I created a PR #182 to address the changes proposed here.

The PR also does some editorial changes where "labelled steps" that are only referenced once in the algorithm are inlined at that position for readability. "labelled steps" that are referenced from multiple locations are kept.

Member

adam-be commented Jun 9, 2015

I created a PR #182 to address the changes proposed here.

The PR also does some editorial changes where "labelled steps" that are only referenced once in the algorithm are inlined at that position for readability. "labelled steps" that are referenced from multiple locations are kept.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf Jun 9, 2015

Thanks for the editing and keeping track of it.

There is one minor point around "Run the steps asynchronously".
When reading it, I took it as a strong requirement (something like "MUST run the steps asynchronously"), while the intention seems more like "SHOULD run the steps asynchronously" IIUC.

youennf commented Jun 9, 2015

Thanks for the editing and keeping track of it.

There is one minor point around "Run the steps asynchronously".
When reading it, I took it as a strong requirement (something like "MUST run the steps asynchronously"), while the intention seems more like "SHOULD run the steps asynchronously" IIUC.

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar Jun 9, 2015

Contributor

In 3.6 it says " If the user never responds, this algorithm stalls on this step." That MUST NOT be done synchronously obviously, or JavaScript would stall.

Contributor

jan-ivar commented Jun 9, 2015

In 3.6 it says " If the user never responds, this algorithm stalls on this step." That MUST NOT be done synchronously obviously, or JavaScript would stall.

@adam-be

This comment has been minimized.

Show comment
Hide comment
@adam-be

adam-be Jun 10, 2015

Member

If we concluded above that resolving a promise synchronously or asynchronously was detectable from JS, can we ever have a SHOULD when it comes to running steps asynchronously? I believe the answer is no. And since the steps in question mentions probed sources and, as @jan-ivar mentions above, a stalling step, I believe we don't have an option to make them synchronous.

Member

adam-be commented Jun 10, 2015

If we concluded above that resolving a promise synchronously or asynchronously was detectable from JS, can we ever have a SHOULD when it comes to running steps asynchronously? I believe the answer is no. And since the steps in question mentions probed sources and, as @jan-ivar mentions above, a stalling step, I believe we don't have an option to make them synchronous.

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar Jun 10, 2015

Contributor

I agree it must be MUST.

Contributor

jan-ivar commented Jun 10, 2015

I agree it must be MUST.

@adam-be

This comment has been minimized.

Show comment
Hide comment
@adam-be

adam-be Jun 11, 2015

Member

@youennf, are you OK with the proposed resolution? If so, we can close this issue and report more specific issues on any related problems.

Member

adam-be commented Jun 11, 2015

@youennf, are you OK with the proposed resolution? If so, we can close this issue and report more specific issues on any related problems.

@youennf

This comment has been minimized.

Show comment
Hide comment
@youennf

youennf Jun 11, 2015

Yep, that seems good.
This gives a precise and deterministic algorithm.
Closing now.

youennf commented Jun 11, 2015

Yep, that seems good.
This gives a precise and deterministic algorithm.
Closing now.

@youennf youennf closed this Jun 11, 2015

alvestrand added a commit that referenced this issue Jun 11, 2015

Merge pull request #182 from adam-be/tidy-gum-algorithm
[Issue  #174] Move sanity checks to sync section of getUserMedia algorithm and tidy up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment