-
Notifications
You must be signed in to change notification settings - Fork 16
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
Restrict onconfigurationchange to background blur #80
Conversation
Context : Background Blur is the only API which is implemented right now and hence clearer spec was requested by Chromium reviewers. We hope to land Face Framing and Lighting Correction soon, both of which should leverage the I was initially thinking if it's reasonable to specify within the callback which property changed, but I guess apps can do state handling themselves and we can keep a simple event. |
What is the rationale for this restriction? |
Chromium reviewer thought that it wasn't sufficiently specified: https://chromium-review.googlesource.com/c/chromium/src/+/4120505/10?tab=comments#message-c6a4e425f9b2e573a70ff61da95dfeaa850a2e33
|
The plan is to support all the cases where the page calls track.getSettings() twice and the results are not the same anymore.
I do not think settings will change here.
I do not think getSettings()/getCapabilities() will change here so no event.
I did a check in Chrome and it seems getSettings() might sometime change over time.
The settings will probably change here so an event will probably be fired |
Another case where settings can change all the time are frameRate, width, height and aspectRatio for RTCPeerConnection-backed tracks.
I don't think we want to fire this event repeatedly, for example, due to timing of how remote frames arrive. |
I do not think width/height/aspectRatio will change much so firing an event there is probably fine. It is true peer connection sources are somehow particular since these settings are mostly stats and not actionable to use applyConstraints. Most interesting sources for configurationchange so far are capture sources. |
I'd personally prefer the spec to explicitly say which changes should fire the event. By starting with background blur, we allow other features to be part of this algorithm. I believe it will be more difficult for browsers to align their behaviour if the spec is too vague about it. We may even face some not practical or desirable behaviours if we allow "all" features to fire the event. |
Even a rounded number is liable to change somewhat often on unstable networks and those with non-trivial packet loss. |
@youennf Shall we send a PR that updates |
That would be good.
Given capabilities and settings have synchronous getters, I would tend to use a single list for both settings and capabilities. |
I'll work on this.
Given recent conversations, shall we limit this list to |
I prefer the default rule be to fire events. |
https://jsfiddle.net/jib1/xtu1yfmv/ shows both Chrome and Firefox update width and height settings live as the user resizes a window, so this would fire a LOT of events. I'm not necessarily opposed, but we seem to have backed into this, so it seems prudent to ask: Are we sure we have good reasons to start doing this now after all these years all of a sudden? If so, should the name be Another idea might be to resurrect w3c/mediacapture-main#576 and modify it to not halt output. Then JS can specify the properties they care about by constraining on them. |
The idea is to consolidate capability and setting changes to the same event because they usually change at the same time if they both change. It would be odd if the name was |
Why should an event fire if a capability changed without affecting the setting? |
I suppose if blur becomes available in the OS then this is useful information. Thanks for explaining! |
I think we can close this PR. WDYT? |
Sounds good, let's reopen it if needed |
This CL makes it clear onconfigurationchange applies to background blur only for now.
FYI @eehakkin @riju
Preview | Diff