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

Relinquish device when all tracks are muted or disabled. #662

Merged
merged 5 commits into from
Mar 12, 2020

Conversation

jan-ivar
Copy link
Member

Fixes #642.

@jan-ivar jan-ivar self-assigned this Feb 25, 2020
@youennf
Copy link
Contributor

youennf commented Feb 26, 2020

As I said in the issue thread, I am not sure @enabled is the right API for requesting to capture again after some time has passed. For instance, if the user agent decides to not allow to unmute the track, it can either decide to leave it muted or end the track, which requires the application to look for both events and maybe decide to call getUserMedia after some timeouts or after receiving the end event.

A promise-based API would seem more natural. We could gate it as we do for other APIs, for instance by requiring focus or user activation.
Also, it might be used for other purposes. For instance, Safari is muting a page track if user grants another web page access to the camera/microphone. This API could be used by the other page to request access again instead of calling getUserMedia again.

How about the following:

  1. Leave @enabled as it is defined
  2. Add void mute() that tells the user agent to set track.muted to true (guaranteed to succeed)
  3. Add Promise unmute() that asks the user agent to set track.muted to false (no guarantee to succeed but guaranteed to have an answer).

@jan-ivar
Copy link
Member Author

I don't think we should introduce new APIs without a need. And I don't understand the need here.

Add void mute() that tells the user agent to set track.muted to true (guaranteed to succeed)

Why would JS ever mute? Seems confusing.

I like our separation where muted is controlled by the user agent and enabled is controlled by JS.

We even leverage that in webrtc-pc.

Add Promise unmute() that asks the user agent to set track.muted to false (no guarantee to succeed but guaranteed to have an answer).

If you need a promise, just do

new Promise(r => track.onunmute = r);

@jan-ivar
Copy link
Member Author

There's no reason we couldn't gate track.enabled = true on user activation if we want.

I'm not saying we should, just that the shape of the API doesn't affect whether we can or not.

@jan-ivar
Copy link
Member Author

Safari is muting a page track if user grants another web page access to the camera/microphone. This API could be used by the other page to request access again instead of calling getUserMedia again.

@youennf I tried this fiddle in two different Safari windows and I understand what you mean now.

But why doesn't Safari automatically unmute the first window when I close the second? Or at least when I focus the first window again?

I don't see why the application should need to ask when it didn't want to be muted in the first place. It seems obvious to me that it wants to be unmuted, so why not just assume it does?

@youennf
Copy link
Contributor

youennf commented Mar 2, 2020

There's no reason we couldn't gate track.enabled = true on user activation if we want.

We should first see whether that is web compatible.
We should also decide whether that is desirable for non capture tracks.
I do not see why we should require user activation for a web audio track or a remote audio track for instance.

It is also unclear to me whether we want to restart capturing for getDisplayMedia tracks.
Should there be an indicator when enabled=false?

But why doesn't Safari automatically unmute the first window when I close the second? Or at least when I focus the first window again?

The user can perfectly do that with the camera/mic icon to switch capture between pages.

It seems obvious to me that it wants to be unmuted, so why not just assume it does?

I do not think that switching to a page that was capturing an hour ago is sufficient information to know whether the user wants to restart capturing for this page.
The intent is much clearer if user clicks on a button and the web page is asked to unmute.

Some more thoughts:

Safari camera/mic icon exposes the information of whether the page has a live capture track and if all tracks are muted or not. This proposal introduces a third variable (enabled) that should be taken into consideration in the computation of the page capture state, which does seem unnecessarily complex. It seems more consistent with how the spec is currently defined to piggy back on the muted state.

This PR is also not very clear in how should be treated the case where unmuting is denied by the user agent.
The typical case is a user agent denying recapture for a page that did not capture for several minutes/hours. It could also be that the user agent has a global switch that disables capture for all pages.
The current PR allows two possibilities: keep the track muted or end the track.
A promise-based approach allows to reject with various errors (device no longer there, access denied...).

@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 2, 2020

It is also unclear to me whether we want to restart capturing for getDisplayMedia tracks.

Screenshare has no observable device HW light, so I no reason to relinquish anything there.

Should there be an indicator when enabled=false?

Yes. But whether it needs to be any different from Live I think is up to user agents. Firefox implements a red/gray shift consistent with its cam/mic indicators FWIW (fiddle).

In short, I think mediacapture-screen-share is fine and out of scope (let's discuss there not) here.

It seems obvious to me that it wants to be unmuted, so why not just assume it does?

I do not think that switching to a page that was capturing an hour ago is sufficient information to know whether the user wants to restart capturing for this page.

I didn't say it should do that (although s/hour/seconds/ and I might, given that all other browsers support concurrent access here).

I only said we can assume what the page wants, because it's still holding onto a live track. With that knowledge, Safari should be able to figure this out with UX (e.g. provide said button) without involving the web page.

I'm in favor of Safari experimenting here (I particularly like the idea of an "hour" heuristic), provided it's done within the existing spec model. I.e. I'm not in favor of adding more API surface for it. Having web sites go through new APIs specifically for Safari would be a mistake IMHO.

With mute, enabled and ended, I hope we have sufficient surface to accommodate most UA controls.

@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 2, 2020

The consensus at the virtual interim last Thursday was to merge this PR.

This PR is also not very clear in how should be treated the case where unmuting is denied by the user agent.

By "unmuting" I assume you mean setting track.enabled = true which is all JS can do.

This has no impact on the track's muted state which is "out of control for the application".

It could also be that the user agent has a global switch that disables capture for all pages.
The current PR allows two possibilities: keep the track muted or end the track.

This seems true even without this PR (errors don't appear only while muted).

A promise-based approach allows to reject with various errors (device no longer there, access denied...).

We've discussed exposing errors on the ended event in the past and rejected it.

@youennf
Copy link
Contributor

youennf commented Mar 3, 2020

Screenshare has no observable device HW light, so I no reason to relinquish anything there.

I might be missing something, but a device HW light does not seem like a good criteria.
Any OS level/user agent icon seems in scope to me.
Microphone seems in scope as well although there is often no HW light, at least for built-in mics.

This has no impact on the track's muted state which is "out of control for the application".

It has impact on the track's muted state.
An application can actually change the muted state if it is capturing and not having focus by doing enabled=false/enabled=true.

IIRC, the spec currently does not mandate the user agent to mutate the muted state of a capture track. This PR breaks the assumption that the capture muted state is in full control of the user agent.

Having web sites go through new APIs specifically for Safari would be a mistake IMHO.

This is not only about Safari, this PR tries to minimise the amount of changes to the existing API at the expense of convenience and backward compatibility.

If we add a user activation requirement for this, which I think we should do in this PR and not as a follow-up, we will use a pattern of setting an attribute based on user activation, but this requirement might only apply for a subset of all MediaStreamTrack objects. This seems like a new pattern.

If we add activation requirement, this API does not allow to tell whether the task was rejected because of lack of user activation or something else. Or we will add something like an exception, which might not be very backward compatible. Usually we try to provide this information, see for instance video.play() or device orientation requestPermission.

Setting this attribute might trigger a prompt, which might be a new pattern.
To know whether this async-by-nature task is completed, a web page might have to combine the information given by two event listeners. This also seems like a new pattern.

Web sites will have to adapt whatever the API we choose.
If a web page uses track cloning or requests several media stream tracks, the page will need to make sure to disable all tracks related to a given device to actually get the desired effect. Ditto for enable, potentially in the same runloop if we add the user activation requirement.
This seems error prone and not very web developer friendly.

Given the fact that the target of this PR is specifically about capture sources and MediaStreamTrack can have non-capture sources, I am even wondering whether MediaStreamTrack is the right place for this API.
From a model point of view, it seems this feature is something like 'mute that device' which is closer to MediaDeviceInfo for instance.

The consensus at the virtual interim last Thursday was to merge this PR.

I was planning to attend but unfortunately had to miss it.
I guess I would have expressed the points stated above.
I hope they can be taken into consideration.

@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 3, 2020

@youennf Sorry you missed the interim. The goal and scope was specifically to address known web compat problems around temporary camera and microphone disabling only. See slides. I'm happy to jump on a call to run through them.

IIRC, the spec currently does not mandate the user agent to mutate the muted state of a capture track. This PR breaks the assumption that the capture muted state is in full control of the user agent.

Good point. I've updated the PR to reduce the mandate only to muting, not unmuting. I agree the user agent should remain in full control of unmuting.

The reason for mandating muting in this instance is to provide stronger guarantees around privacy, but it's peripheral to the main goal of the PR. Is SHOULD better?

An application can actually change the muted state if it is capturing and not having focus by doing enabled=false/enabled=true.

True, though a page isn't generally in control of whether it has focus or not, so I don't think we need to change the sentence "Muted is out of control for the application" over this...

There is no consensus for user activation or new APIs.
Track cloning is already well-defined, so their behavior in relation to this is a feature.
There's lots of precedence in Mediacapture-main for discussing camera/mic-specific tracks.
Minimizing the amount of changes to an existing API is generally helps backwards compatibility.

@youennf
Copy link
Contributor

youennf commented Mar 4, 2020

but it's peripheral to the main goal of the PR. Is SHOULD better?

SHOULD for muting sounds good to me.
This allows to both describe the problem in the spec and provide some concrete guidelines for implementors. And it should be easy to move back to a MUST when feasible.
For unmuting, I would personally go with MAY to give time for experimentation to validate or update the proposed approach.

@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 4, 2020

I went with "UA SHOULD instead queue a task to mute the track, and not queue a task to unmute it until the document regains focus"

@youennf
Copy link
Contributor

youennf commented Mar 4, 2020

I was maybe unclear. I am fine with a SHOULD for the whole PR, including relinquishing.
In general, this PR seems to go deep into User Agent and OS UI/UX. We usually try to provide guidelines in that area, not mandate behaviours.

@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 4, 2020

We usually try to provide guidelines in that area, not mandate behaviours.

Yes, and that failed miserably in this case, as I presented at the interim. The consensus was to enforce web compat around behavior in all browsers on this.

@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 4, 2020

I believe the meeting was recorded.

@henbos
Copy link
Contributor

henbos commented Mar 5, 2020

Editors can integrate if you change it to a SHOULD. We need to make all the steps well defined (there is a difference between "relinquish device" and "relinquish permission", which could pose problems). We need to follow up on these issues though, in the end we want MUSTs not SHOULDs...

getusermedia.html Outdated Show resolved Hide resolved
@jan-ivar
Copy link
Member Author

Travis appears to be failing over infra here? @dontcallmedom?

syntax error at support/link-checker/bin/checklink line 1342, near "$ulinks{"

@dontcallmedom
Copy link
Member

@jan-ivar I've disabled the failing script for now and will look at a better fix soon

@jan-ivar jan-ivar merged commit 6609f32 into w3c:master Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only Firefox turns off device on disabled track. Stronger language needed?
4 participants