mediacapture-extensions icon indicating copy to clipboard operation
mediacapture-extensions copied to clipboard

Add changes to configurationchange MediaStreamTrack event

Open beaufortfrancois opened this issue 3 years ago • 24 comments

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 avatar Jan 27 '23 17:01 beaufortfrancois

Gentle ping @youennf

beaufortfrancois avatar Feb 02 '23 08:02 beaufortfrancois

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

youennf avatar Feb 02 '23 15:02 youennf

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?

alvestrand avatar Feb 02 '23 15:02 alvestrand

§ 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.

jan-ivar avatar Feb 02 '23 15:02 jan-ivar

§ 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);

eladalon1983 avatar Feb 03 '23 15:02 eladalon1983

§ 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.

youennf avatar Feb 06 '23 09:02 youennf

§ 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.

eehakkin avatar Feb 09 '23 11:02 eehakkin

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

beaufortfrancois avatar Feb 09 '23 13:02 beaufortfrancois

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 avatar Feb 09 '23 14:02 jan-ivar

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

jan-ivar avatar Feb 09 '23 15:02 jan-ivar

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?

beaufortfrancois avatar Feb 10 '23 08:02 beaufortfrancois

  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.

eehakkin avatar Feb 14 '23 17:02 eehakkin

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.

annevk avatar Feb 15 '23 14:02 annevk

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?

beaufortfrancois avatar Feb 16 '23 07:02 beaufortfrancois

For discussion in the WG in March?

alvestrand avatar Feb 16 '23 15:02 alvestrand

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.

jan-ivar avatar Mar 02 '23 14:03 jan-ivar

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.

annevk avatar Mar 02 '23 15:03 annevk

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.

youennf avatar Mar 02 '23 15:03 youennf

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

beaufortfrancois avatar Mar 02 '23 16:03 beaufortfrancois

@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.

abhijeetk avatar Apr 19 '23 10:04 abhijeetk

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

jan-ivar avatar Feb 22 '24 15:02 jan-ivar

Prematurely closed, sorry.

jan-ivar avatar Feb 22 '24 15:02 jan-ivar