w3c / mediacapture-extensions

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

Add changes to configurationchange MediaStreamTrack event #83

Open beaufortfrancois opened 1 year ago

beaufortfrancois commented 1 year ago

Following https://github.com/w3c/mediacapture-extensions/pull/80#issuecomment-1406646366, here's my naive attempt to expose changes in the configurationchange MediaStreamTrack event.

Live preview: https://pr-preview.s3.amazonaws.com/beaufortfrancois/mediacapture-extensions/pull/83.html#exposing-change-of-mediastreamtrack-configuration

@youennf PTAL


Preview | Diff

beaufortfrancois commented 1 year ago

Gentle ping @youennf

youennf commented 1 year ago

Let's put this on the agenda for next interim.

alvestrand commented 1 year ago

Not clear to me what adopting this change means for other attributes than background blur - for instance, if a captured tab is resized, its size attributes change - does the event fire? and when?

jan-ivar commented 1 year ago

§ 7.7. Use plain Events for state says we generally want to fire plain events and not add state to events. It seems to me, the app can figure out what changed without this information.

For example, on devicechange we DON'T have information on the event about which device(s) were added or which were removed, as this is simple enough to figure out and would get complex real fast.

eladalon1983 commented 1 year ago

§ 7.7. Use plain Events for state says we generally want to fire plain events and not add state to events. It seems to me, the app can figure out what changed without this information.

Arguably, an app can more efficiently and elegantly bail early on unimportant changes if it can query the event for "is x among the properties changed" rather than locally store state and compare it to the new state, which it needs to call getSettings() for.

Contrast:

if (!event.changes.includes("backgroundBlur")) {
  return;
}
respondToNewState(event.target.getSettings().backgroundBlur);

With:

const settings = event.target.getSettings();
if (settings.backgroundBlur == previousState.backgroundBlur) {
  return;
}
previousState = settings;
respondToNewState(settings.backgroundBlur);
youennf commented 1 year ago

§ 7.7. Use plain Events for state says we generally want to fire plain events and not add state to events. It seems to me, the app can figure out what changed without this information.

It is true the app can figure this out on its own. On the other hand, I am also sympathetic to the desire to quickly identify what changed. The main reason is configurationchange is a single event for many different change types. An alternative is to have an event per settings/capabilities (like click events somehow do) but that seems overkill to me.

For example, on devicechange we DON'T have information on the event about which device(s) were added or which were removed, as this is simple enough to figure out and would get complex real fast.

The analogy would be more about whether exposing a device-added/device-removed enumeration to the event. In that case, there are only two alternatives, compared to configurationchange which has many more.

eehakkin commented 1 year ago

§ 7.7. Use plain Events for state says we generally want to fire plain events and not add state to events. It seems to me, the app can figure out what changed without this information.

It is true the app can figure this out on its own. On the other hand, I am also sympathetic to the desire to quickly identify what changed. The main reason is configurationchange is a single event for many different change types. An alternative is to have an event per settings/capabilities (like click events somehow do) but that seems overkill to me.

I agree. Overall, this change seems good to me.

beaufortfrancois commented 1 year ago

@jan-ivar I agree with https://github.com/w3c/mediacapture-extensions/pull/83#issuecomment-1418810612 as well.

jan-ivar commented 1 year ago

This is an event with one name returning a list of other names basically. I think this is the sort of APIs the design guides explicitly tell us to avoid, because it creates new and disparate/novel patterns that are inherently redundant with each other and creates entropy.

I think it's a false choice to say the alternative is an event name per change. This seems easy enough for JS and similar enough to devicechange which does NOT have an event for add and a separate for remove, because JS simply compares the result before and after to learn that. Same here. For example:

let lastSettings = track.getSettings();

track.onconfigurationchange = () => {
  const settings = track.getSettings();
  if (settings.backgroundBlur != lastSettings.backgroundBlur) {
    // background blur changed
  }
  lastSettings = track.getSettings();
};

We should not create new platform APIs for this simple thing. That is my position as well as my interpretation of TAG advice in § 7.7. Use plain Events for state. cc @annevk

jan-ivar commented 1 year ago

In the interest of progress, isn't this something we can add later if needed rather than block on?

beaufortfrancois commented 1 year ago

IIUC we now have two options:

  1. Define an event handler for each type of configuration change. For background blur, it would be onbackgroundblurchange, etc. (See https://github.com/w3c/mediacapture-extensions/issues/82#issuecomment-1405202251)
track.onbackgroundblurchange = () => {
  // Background blur has changed.
  if (track.getSettings().backgroundBlur) {
    // Turn off web app own blurring so that video doesn't get double-blurred.
  }
};
  1. Don't include changes in configurationchange event and let web developers figure out by themselves if the event matters to them.
let lastSettings = track.getSettings();

track.onconfigurationchange = () => {
  const settings = track.getSettings();
  if (settings.backgroundBlur != lastSettings.backgroundBlur) {
    // Turn off web app own blurring so that video doesn't get double-blurred.
  }
  lastSettings = track.getSettings();
};

Am I missing another option?

eehakkin commented 1 year ago
  1. Define an event handler for each type of configuration change. For background blur, it would be onbackgroundblurchange, etc. (See Filtering for relevant configurationchange events #82 (comment))

I am not very fond of this approach. It either gets really complex (there are 37 constrainable properties in total defined in MediaTrackSettings at least on Chrome) or it treats constrainable properties differently from each other (like if there were event only say for background blur).

  1. Don't include changes in configurationchange event and let web developers figure out by themselves if the event matters to them.

I am quite fine with not including changes in configurationchange event. It adds some avoidable work to event handlers but it is still managable. It might also require some clarification, such as that the event should not be triggered if the frameRate setting is not a real setting but an estimate which is about to change on every or always every frame or if the color temperature changes during auto white balance mode, etc.

Am I missing another option?

In https://github.com/w3c/mediacapture-extensions/issues/82#issuecomment-1405134571 you suggested a MediaStreamTrackObserver based on the Observer web pattern. That might be an overkill but it would handle everything nicely including changed frame rate estimates.

annevk commented 1 year ago

FWIW, this API-shape doesn't look unreasonable to me. Events can come with accompanying data that explains the nature of the change. E.g., the recently-added HTML popover feature exposes the old and new states in its beforetoggle/toggle events. That can be especially useful if you want to coalesce events, but I also don't think we strive to avoid redundancy at all costs. Developer ergonomics are important too.

beaufortfrancois commented 1 year ago

According to https://github.com/w3c/mediacapture-extensions/pull/83#issuecomment-1431458996, it seems like we could actually expose changes to configurationchange event.

@youennf @jan-ivar What are the next steps?

alvestrand commented 1 year ago

For discussion in the WG in March?

jan-ivar commented 1 year ago

the recently-added HTML popover feature exposes the old and new states in its beforetoggle/toggle events.

MST changes its settings inside the queued task whereas https://html.spec.whatwg.org/#queue-a-popover-toggle-event-task appears to have a different event firing model where the event fires AFTER the state transition (which isn't in the queued task).

So oldState in that case isn't trivially available on the event target like in our case, if I read it right. Getting git blame on html is a pain, so I haven't yet dug out whether that was part of its rationale, but it seems to exist not solely for developer ergonomics, and also highlights why e.g. adding oldState to the event target would have been weird, since the point there seems to be to disambiguate state transitions that effectively have already happened.

In our case, I've seen no evidence that disambiguating the state transitions of blur is important. It seems apps are mostly interested in the most recent state, which seems trivial to ascertain from the event target, by comparing to the app's tracking of its state of interest.

annevk commented 1 year ago

That's true for toggle, but not beforetoggle, which uses the same interface.

I think your suggestion above about adding this once web developers have had a chance to write some code against a plainer version is reasonable though. Start with the minimal thing that works and then build up based on demand and patterns that emerge.

youennf commented 1 year ago

Start with the minimal thing that works and then build up based on demand and patterns that emerge.

Makes sense to me. It seems we can wait for web developer input on this.

beaufortfrancois commented 1 year ago

FYI I've started https://github.com/w3c/mediacapture-extensions/pull/89 to improve the configuration change section.

abhijeetk commented 1 year ago

@youennf @eladalon1983 For a video stream, What is expected when device orientation changes(portrait to lanscape and vice versa) ? Should it trigger a configuration change for a video track settings with updated values of width and height of video stream ?

Refer : https://jsfiddle.net/abhijeetk/k9qso14g/5/ After orientation change, Video track setting remanins same but video element dimensions are changing.

dontcallmedom-bot commented 1 year ago

This issue was discussed in WebRTC Feb 2023 Meeting – (onConfigurationChange 🎞︎)

dontcallmedom-bot commented 1 year ago

This issue was mentioned in WebRTC Feb 2023 Meeting – (onConfigurationChange 🎞︎)

jan-ivar commented 7 months ago

Nearly a year old. Is there still interest in proceeding with this approach?

jan-ivar commented 7 months ago

Prematurely closed, sorry.