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

Filtering for relevant configurationchange events #82

Open eladalon1983 opened 1 year ago

eladalon1983 commented 1 year ago

There is the potential for many different configuration changes. What if an application is only interested in some? For example, we see this demo code by François, which currently reads:

function configurationChange(event) {
  const settings = event.target.getSettings();
  if ("backgroundBlur" in settings) {
    log(`Background blur changed to ${settings.backgroundBlur ? "ON" : "OFF"}`);
  }
}

This assumes that the only possible event is to blur, or else the following would be possible:

Background blur changed to OFF Background blur changed to OFF Background blur changed to OFF

A likely future modification of the code would be:

function configurationChange(event) {
  if (event.whatChanged != "blur") {
    return;
  }
  const settings = event.target.getSettings();
  if ("backgroundBlur" in settings) {
    log(`Background blur changed to ${settings.backgroundBlur ? "ON" : "OFF"}`);
  }
}

However, depending on what changes and how often, this could mean that the event handler is invoked in vain 99% of the time, and possible multiple times per second, needlessly wasting CPU on processing the event by JS code.

I think a more reasonable shape for the API, would be to expose a subscribe-method.

track.subscribeToConfigChanges(eventHandler, ["blur", "xxx", "yyy", "zzz"]);

Wdyt?

(Some prior art here is in w3c/mediacapture-screen-share#80. I propose my change as incremental progress over it. CC @beaufortfrancois, @guidou, @eehakkin and @youennf)

beaufortfrancois commented 1 year ago

Thanks for filing this issue!

A likely future modification of the code would be:

function configurationChange(event) {
  if (event.whatChanged != "blur") {
    return;
  }
  const settings = event.target.getSettings();
  if ("backgroundBlur" in settings) {
    log(`Background blur changed to ${settings.backgroundBlur ? "ON" : "OFF"}`);
  }
}

You're absolutely right. I'd like us to find a proper way to define this whatChanged property. A new dictionary similar to MediaTrackSupportedConstraints could contain the list of changes.

I think a more reasonable shape for the API, would be to expose a subscribe-method.

track.subscribeToConfigChanges(eventHandler, ["blur", "xxx", "yyy", "zzz"]);

The same way window.onresize is fired over and over again very rapidly, web developers already use common techniques like throttling and debouncing to address this kind of issues. Note that the UA could also do that work.

eladalon1983 commented 1 year ago

The same way window.onresize is fired over and over again very rapidly,

That event would usually fire when the user manually resizes the window, which is a relatively rare event, and therefore not much of a concern. But if video frames are coming in 30 or even 60 times a second, and audio about 50 times a second, continuously throughout the call, then there is a significant opportunity for CPU savings in not firing an event each time.

web developers already use common techniques like throttling and debouncing

Throttling by a Web developer would deal with repeated events of a type that is of interest to the developer. My concern and the solution I propose deal with the inverse case - those events which the Web developer intends to simply ignore. In that case, I am concerned with the inefficiency inherent in firing needless events into JS-land. Even a handler that does nothing but a simple string comparison and a quick return, would still incur the needless cost here. You cannot throttle or debounce that away in JS.

Note that the UA could also do that work.

If something changes N times a second, how can the UA discern that the app is not interested in exactly N notifications a second? Some apps would be.

eehakkin commented 1 year ago

@eladalon1983 in https://github.com/w3c/mediacapture-extensions/issues/82#issue-1556833276:

I think a more reasonable shape for the API, would be to expose a subscribe-method.

track.subscribeToConfigChanges(eventHandler, ["blur", "xxx", "yyy", "zzz"]);

Would that subscribe both to blur capability changes and to blur setting changes? What if the event handler is only interested in one of them?

@beaufortfrancois in https://github.com/w3c/mediacapture-extensions/issues/82#issuecomment-1403807169:

You're absolutely right. I'd like us to find a proper way to define this whatChanged property. A new dictionary similar to MediaTrackSupportedConstraints could contain the list of changes.

Note also that the name of a property might not be enough because the changed aspect of the property could be the capabilities and/or the setting (like the background blur setting could be and remain as true while the capabilities change from [false, true] to [true] or the capabilities could be and remain as [false, true] while the setting changes from falseto trueor the capabilities and the setting could change from [false, true] and false to [true] and true). So if there were dictionaries containing booleans (similar to MediaTrackSupportedConstraints), there might have to changedCapabilities and changedSettings dictionaries or something similar.

I think a more reasonable shape for the API, would be to expose a subscribe-method.

track.subscribeToConfigChanges(eventHandler, ["blur", "xxx", "yyy", "zzz"]);

The same way window.onresize is fired over and over again very rapidly, web developers already use common techniques like throttling and debouncing to address this kind of issues. Note that the UA could also do that work.

@eladalon1983 in https://github.com/w3c/mediacapture-extensions/issues/82#issuecomment-1404031624:

The same way window.onresize is fired over and over again very rapidly,

That event would usually fire when the user manually resizes the window, which is a relatively rare event, and therefore not much of a concern. But if video frames are coming in 30 or even 60 times a second, and audio about 50 times a second, continuously throughout the call, then there is a significant opportunity for CPU savings in not firing an event each time.

Are there any other property than estimated frameRate which could change on every frame? Real configuration changes are usually relatively rare event, so they will not occure multiple times per second.

web developers already use common techniques like throttling and debouncing

Throttling by a Web developer would deal with repeated events of a type that is of interest to the developer. My concern and the solution I propose deal with the inverse case - those events which the Web developer intends to simply ignore. In that case, I am concerned with the inefficiency inherent in firing needless events into JS-land. Even a handler that does nothing but a simple string comparison and a quick return, would still incur the needless cost here. You cannot throttle or debounce that away in JS.

Note that the UA could also do that work.

If something changes N times a second, how can the UA discern that the app is not interested in exactly N notifications a second? Some apps would be.

If nothing but frameRate changes N times a second, maybe UA can throttle frameRate. So webapps get notified on other changes in time (because no throttling is needed by done by UA) and webapps interested in frame rates on every frame can subscribe to receive frames using other means (like video frame callback or transform streams).

eladalon1983 commented 1 year ago

Would that subscribe both to blur capability changes and to blur setting changes?

Maybe these deserve different event handlers. Wdyt?

Are there any other property than estimated frameRate which could change on every frame?

Possibly audio latency, but I've not looked deeply into that yet.

Assuming for the sake of argument that it's only frameRate, I'd say that:

  1. We already have one good reason.
  2. The future might conjure up additional reasons.
  3. When adding settings and/or capabilities in the future, it would be nice to know that we don't need to think about how often they might change, since previously-deployed code would not be at risk of being subscribed to such events.

(The difference between 2 and 3 is that the latter applies even when we do eventually decide "this is rare enough." Consensus-building is hard enough. The less potential points of contention, the better.)

If nothing but frameRate changes N times a second, maybe UA can throttle frameRate. So webapps get notified on other changes in time (because no throttling is needed by done by UA) and webapps interested in frame rates on every frame can subscribe to receive frames using other means (like video frame callback or transform streams).

If the UA employs such a heuristic, it would make certain apps harder to write, and less accurate. For example, an app that plots the frameRate over time would now require more code, and end up less accurate due to JS-processing delays.

beaufortfrancois commented 1 year ago

How about introducing a new MediaStreamTrackObserver interface based on the Observer web pattern?

const stream = await navigator.mediaDevices.getUserMedia({ video: true });
const [track] = stream.getVideoTracks();

// New!
const options = {
  capabilities: ['backgroundBlur'],
  settings: ['backgroundBlur', 'frameRate'],
};
function configurationChange(changes, observer) {
  const change = changes[0];
  if ('backgroundBlur' in change.capabilities) {
    // Background blur capabilities have changed.
  }
  if ('backgroundBlur' in change.settings) {
    // Background blur setting has changed.
  }
  if ('frameRate' in change.settings) {
    // Frame rate setting has changed.
  }
}
const observer = new MediaStreamTrackObserver(configurationChange, options);
observer.observe(track);

// Later...
observer.unobserve(track);

It may be overkill though...

youennf commented 1 year ago

A single event seems better to me if we can stick to it. This event could contain information to identify which settings/capabilities have changed.

eladalon1983 commented 1 year ago

A single event seems better to me if we can stick to it. This event could contain information to identify which settings/capabilities have changed.

Do you mean single event for capabilities and settings? What's your opinion about restricting the event-subtypes one is registered to receive?

eladalon1983 commented 1 year ago

(I support François's proposal.)

youennf commented 1 year ago

Do you mean single event for capabilities and settings?

I tend to prefer one event. The event could tell whether capabilities and/or settings are changed.

What's your opinion about restricting the event-subtypes one is registered to receive?

This seems a bit odd to me. First, I am unsure whether this is useful if we provide a quick way for JS to filter the setting changes they are interested in. Second, the API shape seems a bit off, it smells like addEventListener, but it would really be a callback.

eladalon1983 commented 1 year ago

First, I am unsure whether this is useful if we provide a quick way for JS to filter the setting changes they are interested in.

I agree that it gives diminished returns, if we agree on quickly exposing which settings/capabilities have changed. My interest in that stems in part from my desire for a mechanism that can be used with my auto-pause mechanism, without causing frames to be needlessly dropped when an irrelevant config-change occurs, which the handler will just unpause-and-return once it sees.

Second, the API shape seems a bit off, it smells like addEventListener, but it would really be a callback.

I am open to alternative shapes.

youennf commented 1 year ago

my desire for a mechanism that can be used with my auto-pause mechanism

We should first decide whether auto-pause should be handled at the source or at each track. If the former, configuration change event is probably not the right mechanism.

eladalon1983 commented 1 year ago

Is there a reason, other than implementation complexity[*], for auto-pause to happen on sources rather than tracks?

[*] Normally a valid concern, but here I suspect the complexity is quite low, as pausing could be implemented by dropping frames where applicable. Later, the optimization could be added of not posting frames from the relevant source if all associated tracks are paused. Or wdyt?

youennf commented 1 year ago

Is there a reason, other than implementation complexity[*], for auto-pause to happen on sources rather than tracks?

Let's continue this discussion in the auto-pause issue, https://github.com/w3c/mediacapture-screen-share-extensions/issues/4

eladalon1983 commented 1 year ago

Is there a reason, other than implementation complexity[*], for auto-pause to happen on sources rather than tracks?

Let's continue this discussion in the auto-pause issue, w3c/mediacapture-screen-share-extensions#4

Makes sense.