Add changes to configurationchange MediaStreamTrack event #83

beaufortfrancois · 2023-01-27T17:04:36Z

Following #80 (comment), 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 · 2023-02-02T08:29:08Z

Gentle ping @youennf

eladalon1983 · 2023-02-02T08:38:16Z

index.html

- <li>
- <p>[=Fire an event=] named <var>configurationchange</var> on <var>track</var>.</p>
+ <li><p>Let <var>changes</var> be an empty [=sequence=].</p></li>
+ <li><p>For each change in <var>track</var>'s capabilities and settings, add its name to <var>changes</var>.</p></li>


"For each change... add its name"
What is the name of a change? Should that be clarified? (Depending on the answer - the issue was raised of distinguishing capabilities and settings with a similar name. Have I missed the resolution of that discussion?)

We tentatively settled on having a single array.
One reason is that both capabilities and settings might change at the same time.
Say bgblur is made mandatory, bgblur capabiliy becomes [true] and settings become true.

I agree with Elad, we can probably tidy a bit the language.
Source settings are dictionaries, ditto for capabilities, we could use for describing the matching algorithm, something like:

for each key-value pair of source settings, if track settings does not have the same pair, append key to the list if not already in the list.

for each key-value pair of track settings, if source settings does not have the same pair, append key to the list if not already in the list.

for each key-value pair of source capabilities, if track capabilities does not have the same pair, append key to the list if not already in the list.

for each key-value pair of track capabilities, if source set capabilities ings does not have the same pair, append key to the list if not already in the list.

If list is empty, abort these steps

Sort the list by lexicographical order.

Fire the event

Thank you! It is much better now!

eladalon1983 · 2023-02-02T08:42:32Z

index.html

+const [track] = stream.getVideoTracks();
+track.addEventListener("configurationchange", (event) => {
+ if (event.changes.includes("backgroundBlur")) {
+ // Background blur configuration has changed.


Nit, and completely up to you, but maybe a clearer example would also indicate why an app might care about this? Maybe the comment could mention it would toggle its own blurring to be the inverse, so as to always be blurred but never double-blurred?

Thanks for the suggestion!

youennf · 2023-02-02T08:52:27Z

index.html

 <p>
 <p>When the [=User Agent=] detects <dfn data-export id="change-track-configuration">a change of configuration</dfn>
 in a <var>track</var>'s underlying source, the [=User Agent=] MUST run the following steps:</p>
 <ol>
 <li><p>If <var>track</var>.{{MediaStreamTrack/muted}} is <code>true</code>, wait for <var>track</var>.{{MediaStreamTrack/muted}}
 to become <code>false</code> or <var>track</var>.{{MediaStreamTrack/readyState}} to be "ended".</p></li>
 <li><p>If <var>track</var>.{{MediaStreamTrack/readyState}} is "ended", abort these steps.</p></li>
- <li><p>If <var>track</var>'s capabilities, constraints and settings are matching <var>source</var> configuration, abort these steps.
+ <li><p>If <var>track</var>'s capabilities, constraints and settings are matching <var>source</var> configuration, abort these steps.</li>


Thinking more about it, if capabilities and settings are matching, why should constraints change?
Maybe we can remove constraints here (or we could leave that to a follow-up)

I definitely agree.

youennf · 2023-02-02T08:52:41Z

index.html

- <li>
- <p>[=Fire an event=] named <var>configurationchange</var> on <var>track</var>.</p>
+ <li><p>Let <var>changes</var> be an empty [=sequence=].</p></li>
+ <li><p>For each change in <var>track</var>'s capabilities and settings, add its name to <var>changes</var>.</p></li>


We tentatively settled on having a single array.
One reason is that both capabilities and settings might change at the same time.
Say bgblur is made mandatory, bgblur capabiliy becomes [true] and settings become true.

youennf · 2023-02-02T08:57:12Z

index.html

- <li>
- <p>[=Fire an event=] named <var>configurationchange</var> on <var>track</var>.</p>
+ <li><p>Let <var>changes</var> be an empty [=sequence=].</p></li>
+ <li><p>For each change in <var>track</var>'s capabilities and settings, add its name to <var>changes</var>.</p></li>


I agree with Elad, we can probably tidy a bit the language.
Source settings are dictionaries, ditto for capabilities, we could use for describing the matching algorithm, something like:

for each key-value pair of source settings, if track settings does not have the same pair, append key to the list if not already in the list.

for each key-value pair of track settings, if source settings does not have the same pair, append key to the list if not already in the list.

for each key-value pair of source capabilities, if track capabilities does not have the same pair, append key to the list if not already in the list.

for each key-value pair of track capabilities, if source set capabilities ings does not have the same pair, append key to the list if not already in the list.

If list is empty, abort these steps

Sort the list by lexicographical order.

Fire the event

youennf · 2023-02-02T09:00:42Z

index.html

+ is an [=event handler IDL attribute=] for the `onconfigurationchange`
+ [=event handler=], whose [=event handler event type=] is
+ <dfn event for=MediaStreamTrack>configurationchange</dfn>.
+ </p>
 <p>
 <p>When the [=User Agent=] detects <dfn data-export id="change-track-configuration">a change of configuration</dfn>
 in a <var>track</var>'s underlying source, the [=User Agent=] MUST run the following steps:</p>


This is a pre-existing issue but it is not very clear whether each clone track will fire in its own event loop task or in the same event loop task.
We might want to file an issue about this.

I've filed an issue at #86.

beaufortfrancois

@youennf @eladalon1983 Thanks for your review! I've addressed your feedback.

beaufortfrancois · 2023-02-02T09:47:05Z

index.html

 <p>
 <p>When the [=User Agent=] detects <dfn data-export id="change-track-configuration">a change of configuration</dfn>
 in a <var>track</var>'s underlying source, the [=User Agent=] MUST run the following steps:</p>
 <ol>
 <li><p>If <var>track</var>.{{MediaStreamTrack/muted}} is <code>true</code>, wait for <var>track</var>.{{MediaStreamTrack/muted}}
 to become <code>false</code> or <var>track</var>.{{MediaStreamTrack/readyState}} to be "ended".</p></li>
 <li><p>If <var>track</var>.{{MediaStreamTrack/readyState}} is "ended", abort these steps.</p></li>
- <li><p>If <var>track</var>'s capabilities, constraints and settings are matching <var>source</var> configuration, abort these steps.
+ <li><p>If <var>track</var>'s capabilities, constraints and settings are matching <var>source</var> configuration, abort these steps.</li>


I definitely agree.

beaufortfrancois · 2023-02-02T09:47:22Z

index.html

+const [track] = stream.getVideoTracks();
+track.addEventListener("configurationchange", (event) => {
+ if (event.changes.includes("backgroundBlur")) {
+ // Background blur configuration has changed.


Thanks for the suggestion!

beaufortfrancois · 2023-02-02T09:50:39Z

index.html

+ is an [=event handler IDL attribute=] for the `onconfigurationchange`
+ [=event handler=], whose [=event handler event type=] is
+ <dfn event for=MediaStreamTrack>configurationchange</dfn>.
+ </p>
 <p>
 <p>When the [=User Agent=] detects <dfn data-export id="change-track-configuration">a change of configuration</dfn>
 in a <var>track</var>'s underlying source, the [=User Agent=] MUST run the following steps:</p>


I've filed an issue at #86.

beaufortfrancois · 2023-02-02T12:53:52Z

index.html

- <li>
- <p>[=Fire an event=] named <var>configurationchange</var> on <var>track</var>.</p>
+ <li><p>Let <var>changes</var> be an empty [=sequence=].</p></li>
+ <li><p>For each change in <var>track</var>'s capabilities and settings, add its name to <var>changes</var>.</p></li>


Thank you! It is much better now!

beaufortfrancois · 2023-02-02T14:40:08Z

index.html

+ <li><p>If <var>changes</var> [=list/is empty=], abort these steps.</p></li>
+ <li><p>[=list/Sort=] <var>changes</var> in lexicographical order.</p></li>
+ <li> 
+ <p>[=Fire an event=] named {{MediaStreamTrack/configurationchange}}


I think we should also specify the boolean values of bubbles and cancellable for this event. I'd say they should be both false. Do you agree?

These are the default values in EventInit so we might not need it.

index.html

youennf · 2023-02-02T15:09:46Z

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

alvestrand · 2023-02-02T15:09:49Z

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 · 2023-02-02T15:13:29Z

§ 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 · 2023-02-03T15:34:15Z

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

Co-authored-by: youennf <[email protected]>

youennf · 2023-02-06T09:57:43Z

§ 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 · 2023-02-09T11:39:35Z

§ 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 · 2023-02-09T13:16:51Z

@jan-ivar I agree with #83 (comment) as well.

jan-ivar · 2023-02-09T14:48:21Z

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 · 2023-02-09T15:38:51Z

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

beaufortfrancois · 2023-02-10T08:54:39Z

IIUC we now have two options:

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

track.onbackgroundblurchange = () => {
  // Background blur has changed.
  if (track.getSettings().backgroundBlur) {
    // Turn off web app own blurring so that video doesn't get double-blurred.
  }
};

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 · 2023-02-14T17:17:41Z

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

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 #82 (comment) 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 · 2023-02-15T14:29:07Z

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 · 2023-02-16T07:38:09Z

According to #83 (comment), it seems like we could actually expose changes to configurationchange event.

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

alvestrand · 2023-02-16T15:45:44Z

For discussion in the WG in March?

jan-ivar · 2023-03-02T14:02:00Z

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 · 2023-03-02T15:06:01Z

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 · 2023-03-02T15:48:52Z

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 · 2023-03-02T16:04:23Z

FYI I've started #89 to improve the configuration change section.

abhijeetk · 2023-04-19T10:26:58Z

@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 · 2023-05-03T15:14:43Z

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

dontcallmedom-bot · 2023-05-03T15:14:46Z

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

jan-ivar · 2024-02-22T15:42:57Z

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

jan-ivar · 2024-02-22T15:45:57Z

Prematurely closed, sorry.

beaufortfrancois requested a review from youennf January 27, 2023 17:04

beaufortfrancois force-pushed the changes branch 2 times, most recently from 83fe9e5 to da7d291 Compare January 27, 2023 17:10

Add changes to configurationchange event

6c0aa31

beaufortfrancois force-pushed the changes branch from da7d291 to 6c0aa31 Compare January 27, 2023 17:10

eladalon1983 reviewed Feb 2, 2023

View reviewed changes

youennf reviewed Feb 2, 2023

View reviewed changes

beaufortfrancois mentioned this pull request Feb 2, 2023

Clarify which event loop task will be used for track in configuration change . #86

Closed

Address feedback

9bb2380

beaufortfrancois commented Feb 2, 2023

View reviewed changes

youennf reviewed Feb 2, 2023

View reviewed changes

index.html Show resolved Hide resolved

beaufortfrancois and others added 2 commits February 6, 2023 10:41

Update index.html

54fdd5e

Co-authored-by: youennf <[email protected]>

Fix nit

5294850

beaufortfrancois mentioned this pull request Mar 2, 2023

Update configuration change section #89

Merged

youennf added the Icebox Stuff that may be done one day, but not today. label Mar 16, 2023

jan-ivar closed this Feb 22, 2024

jan-ivar reopened this Feb 22, 2024

Add changes to configurationchange MediaStreamTrack event #83

Are you sure you want to change the base?

Add changes to configurationchange MediaStreamTrack event #83

Conversation

beaufortfrancois commented Jan 27, 2023 • edited by pr-preview bot

beaufortfrancois commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaufortfrancois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youennf commented Feb 2, 2023

alvestrand commented Feb 2, 2023

jan-ivar commented Feb 2, 2023

eladalon1983 commented Feb 3, 2023

youennf commented Feb 6, 2023

eehakkin commented Feb 9, 2023

beaufortfrancois commented Feb 9, 2023

jan-ivar commented Feb 9, 2023 • edited

jan-ivar commented Feb 9, 2023

beaufortfrancois commented Feb 10, 2023

eehakkin commented Feb 14, 2023

annevk commented Feb 15, 2023

beaufortfrancois commented Feb 16, 2023

alvestrand commented Feb 16, 2023

jan-ivar commented Mar 2, 2023 • edited

annevk commented Mar 2, 2023

youennf commented Mar 2, 2023

beaufortfrancois commented Mar 2, 2023 • edited

abhijeetk commented Apr 19, 2023

dontcallmedom-bot commented May 3, 2023

dontcallmedom-bot commented May 3, 2023

jan-ivar commented Feb 22, 2024 • edited

jan-ivar commented Feb 22, 2024

beaufortfrancois commented Jan 27, 2023 •

edited by pr-preview bot

jan-ivar commented Feb 9, 2023 •

edited

jan-ivar commented Mar 2, 2023 •

edited

beaufortfrancois commented Mar 2, 2023 •

edited

jan-ivar commented Feb 22, 2024 •

edited