w3c / mediacapture-extensions

Extensions to Media Capture and Streams by the WebRTC Working Group
https://w3c.github.io/mediacapture-extensions/
Other
19 stars 14 forks source link

Restrict onconfigurationchange to background blur #80

Closed beaufortfrancois closed 1 year ago

beaufortfrancois commented 1 year ago

This CL makes it clear onconfigurationchange applies to background blur only for now.

FYI @eehakkin @riju


Preview | Diff

riju commented 1 year ago

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 onConfigurationChange(). When they land, we need to update the spec again to reflect that onConfigurationChange() is meant for all the features.

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.

youennf commented 1 year ago

This CL makes it clear onconfigurationchange applies to background blur only for now.

What is the rationale for this restriction?

beaufortfrancois commented 1 year ago

Chromium reviewer thought that it wasn't sufficiently specified: https://chromium-review.googlesource.com/c/chromium/src/+/4120505/10?tab=comments#message-c6a4e425f9b2e573a70ff61da95dfeaa850a2e33

According to https://w3c.github.io/mediacapture-extensions/#exposing-change-of-mediastreamtrack-configuration the event should be fired whenever settings, constraints or capabilities change "dunamically outside the control of web applications" ?

This does not seem to be sufficiently specified. The only example given is when background blur is enabled via the OS. What about these other examples?

  • A camera changes frame rate due to low light.
  • A user resizes a tab or window that is being captured, which causes the width/height/aspectRatio properties to change.
  • A remote peer changes the resolution of an RTCPeerConnection-backed track due to network conditions.
  • The user changes audio properties via the OS.

Is the plan to support all these cases (and many others that lead to changes in settings)? If the idea is to support just backgroundBlur maybe the spec should be amended to be more specific.

youennf commented 1 year ago

Is the plan to support all these cases (and many others that lead to changes in settings)?

The plan is to support all the cases where the page calls track.getSettings() twice and the results are not the same anymore. In that case, the even should have been fired sometimes between the two calls.

  • A camera changes frame rate due to low light.

I do not think settings will change here. I do not think capabilities range will change either. Hence no event in that case I think.

  • A user resizes a tab or window that is being captured, which causes the width/height/aspectRatio properties to change.

I do not think getSettings()/getCapabilities() will change here so no event. If Chrome implementation is changing, an event would be fired.

  • A remote peer changes the resolution of an RTCPeerConnection-backed track due to network conditions.

I did a check in Chrome and it seems getSettings() might sometime change over time. This would mean an event would be fired.

  • The user changes audio properties via the OS.

The settings will probably change here so an event will probably be fired

guidou commented 1 year ago

Another case where settings can change all the time are frameRate, width, height and aspectRatio for RTCPeerConnection-backed tracks. The spec says:

Property Name Values Notes
width ConstrainULong As a setting, this is the width, in pixels, of the latest frame received.
height ConstrainULong As a setting, this is the height, in pixels, of the latest frame received.
frameRate ConstrainDouble As a setting, this is an estimate of the frame rate based on recently received frames.
aspectRatio ConstrainDouble As a setting, this is the aspect ratio of the latest frame; this is the width in pixels divided by height in pixels as a double rounded to the tenth decimal place.

https://w3c.github.io/webrtc-pc/#mediatracksupportedconstraints-mediatrackcapabilities-mediatrackconstraints-and-mediatracksettings

I don't think we want to fire this event repeatedly, for example, due to timing of how remote frames arrive.

youennf commented 1 year ago

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. As of frameRate, I agree we do not want to fire an event for every frame. Maybe the frame rate can be rounded so that it does not change for every frame.

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. We could decide that configurationchange is only triggered for specific source types, meaning we would exclude say canvas sources or peer connection sources.

beaufortfrancois commented 1 year ago

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.

eladalon1983 commented 1 year ago

Maybe the frame rate can be rounded so that it does not change for every frame.

Even a rounded number is liable to change somewhat often on unstable networks and those with non-trivial packet loss.

beaufortfrancois commented 1 year ago

@youennf Shall we send a PR that updates onconfigurationchange to expose changes as well? If so, shall we expose capabilities and settings separately or is a list of strings (['backgroundBlur', '...']) enough?

youennf commented 1 year ago

@youennf Shall we send a PR that updates onconfigurationchange to expose changes as well?

That would be good.

If so, shall we expose capabilities and settings separately or is a list of strings (['backgroundBlur', '...']) enough?

Given capabilities and settings have synchronous getters, I would tend to use a single list for both settings and capabilities.

beaufortfrancois commented 1 year ago

@youennf Shall we send a PR that updates onconfigurationchange to expose changes as well?

That would be good.

I'll work on this.

If so, shall we expose capabilities and settings separately or is a list of strings (['backgroundBlur', '...']) enough?

Given capabilities and settings have synchronous getters, I would tend to use a single list for both settings and capabilities.

Given recent conversations, shall we limit this list to backgroundBlur for now and add more in the future?

youennf commented 1 year ago

Given recent conversations, shall we limit this list to backgroundBlur for now and add more in the future?

I prefer the default rule be to fire events. We can always have opt-out rules per source and/or per settings.

jan-ivar commented 1 year ago
  • A user resizes a tab or window that is being captured, which causes the width/height/aspectRatio properties to change.

I do not think getSettings()/getCapabilities() will change here so no event. If Chrome implementation is changing, an event would be fired.

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 onsettingschange perhaps? Or is a user resizing a window "configuration"? ("Let me reconfigure this window") Maybe?

Another idea might be to resurrect https://github.com/w3c/mediacapture-main/pull/576 and modify it to not halt output. Then JS can specify the properties they care about by constraining on them.

eehakkin commented 1 year ago

If so, should the name be onsettingschange perhaps? Or is a user resizing a window "configuration"? ("Let me reconfigure this window") Maybe?

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 oncapabilityandorsettingchange or if the name was onsettingchange and the event were triggered when only the capability changed without a setting change. Not that I really care about the event name.

jan-ivar commented 1 year ago

It would be odd if the name was oncapabilityandorsettingchange or if the name was onsettingchange and the event were triggered when only the capability changed without a setting change.

Why should an event fire if a capability changed without affecting the setting?

jan-ivar commented 1 year ago

I suppose if blur becomes available in the OS then this is useful information. Thanks for explaining!

beaufortfrancois commented 1 year ago

I think we can close this PR. WDYT?

youennf commented 1 year ago

Sounds good, let's reopen it if needed