Set muted before SRD resolves, using new set muted algorithm.

[jan-ivar]

Fix for #1568 (along with w3c/mediacapture-main#498).

[fluffy]

LGTM

[alvestrand]

Needs a little more review before merging.

[alvestrand]

should this "update" also be changed to "set"?

[jan-ivar]

Yes thanks for spotting that!

[adam-be]

The "set muted state" steps are always queued anyhow, but I guess the idea of moving out the enqueue part is to be able to use different task sources.

[adam-be]

As @alvestrand pointed out, we should be able to get rid of the old "update language". Other than that this LGTM.

[jan-ivar]

The "set muted state" steps are always queued anyhow

@adam-be Here's the one place we don't queue a task (the purpose of this PR), inside the process the removal of a remote track algorithm which itself is already called from a queued task, to solve #1568.

[henbos]

If we fire the event immediately, the event will be fired before all effects of the SRD has occurred - and inspecting the PC or related objects like sender/receiver/transceiver/streams/tracks will yield confusing results sensitive to the order of objects processed by this algorithm.

If we schedule a task to fire the event the event will occur after the SRD promise has resolved (which is inconsistent with ontrack for tracks being added) and could theoretically occur at a time when the muted state has already changed again.

We should do what we do for ontrack:
Here: Set the muted state, and if it changed as a result of this, enqueue mutedEvent on to mutedEvents.
Before SRD is resolved, but still synchronous to the rest of the SRD steps: For each mutedEvent in mutedEvents, fire mutedEvent.

It's important that all events fired in relation to an SRD are fired synchronously, but after the state of all objects has been updated, to ensure inspecting the state of the PC is meaningful.

[jan-ivar]

I also removed a confusing parenthesis in section 10.3. which covers the other two ways SSRCs can be removed.

[henbos]

If we fire the event immediately, the event will be fired before all effects of the SRD has occurred - and inspecting the PC or related objects like sender/receiver/transceiver/streams/tracks will yield confusing results sensitive to the order of objects processed by this algorithm.

If we schedule a task to fire the event the event will occur after the SRD promise has resolved (which is inconsistent with ontrack for tracks being added) and could theoretically occur at a time when the muted state has already changed again.

We should do what we do for ontrack:
Here: Set the muted state, and if it changed as a result of this, enqueue mutedEvent on to mutedEvents.
Before SRD is resolved, but still synchronous to the rest of the SRD steps: For each mutedEvent in mutedEvents, fire mutedEvent.

It's important that all events fired in relation to an SRD are fired synchronously, but after the state of all objects has been updated, to ensure inspecting the state of the PC is meaningful.

[jan-ivar]

@henbos Thanks, good point. Feedback incorporated. PTAL.

[henbos]

A track's muted state (boolean muted attribute) is part of the state that a fired event might inspect. It is therefor important to update all muted states before firing all mute events.

Imagine two tracks are muted. Looking at the second track's muted state from within the first track's onmute will reveal false even though it is one of the tracks that was muted by the SRD call (even among an audio and video track that is part of the same stream). This is confusing and sensitive to the processing order.

Also, if the event firing is implicit by referencing another spec, consider adding a NOTE similar to the one for removing a track from streams ("This will result in a removetrack event being fired at each MediaStream as described in [GETUSERMEDIA].") saying this will result in a mute event being fired at the MediaStreamTrack.

Whether we update GETUSERMEDIA to decouple setting the state from firing the event, explicitly state the steps here, or reference GETUSERMEDIA but saying "but instead of firing the resulting event, add it to tracksMuted" or the like I have no strong opinion, as long as it is clear what to do.

[jan-ivar]

consider adding a NOTE similar to the one for removing a track

@henbos Will do.

A track's muted state (boolean muted attribute) is part of the state that a fired event might inspect. It is therefor important to update all muted states before firing all mute events.

I don't see how the second follows from the first. The way I learned it, the preferred way of notifying JS of state changes is to queue a task and change the state right before firing the event (synchronously). This is as close as possible to carrying the value in the event itself. This rules out lateral consistency, i.e. people can't generally expect updates to groups of objects in the web platform to happen in sync. Do you know of a precedent to the contrary?

I realize SRD fires a bunch of events synchronously (which the Tag discourages) and that our special stream/track interrelations merited syncing. I think we had good domain reasons for doing this. I'm just not sure those same reasons extend to the more insular muted property.

Looking at the second track's muted state from within the first track's onmute will reveal false even though it it is one of the tracks that was muted by the SRD

I think it could be confusing either way. E.g. I might equally expect to only see true once my mute event handler has run, and only on tracks I've received mute events for so far (which is JS observable).

[alvestrand]

It's likely confusing either way. The issue here is that the two mute events are caused by the same event, so it's entirely reasonable to assume that they will happen "simultaneously" - but as we know, there is no such thing as simultaneous in the JS execution model.

I have a (weak) preference for setting both muted states and then calling both mute event handlers (synchronously), because doing it the other way (setting the second track's muted state inbetween the two event handlers being called) leads to the order of mute events being observable - and I don't like letting users' JS inspect the state of the machinery while the events caused by a single causal event are being played out.

OTOH, I can't bring myself to care very strongly either.

[henbos]

Since ontrack updates are synced, by necessity IMO, stream.onaddtrack/onremovetrack should be too. Since all of these events have to do with tracks and streams, it would be odd to leave out the mute event of a track. Also, in most other cases, events are fired asynchronously, which would make any related changes observable when fired. I think updating all mutes first and then firing all events is the most consistent and most useful.

That said, my opinion is not as strong about mute because the only case you would need to rely on track.mute and not stream.onremovetrack would be when the track does not belong to any stream and so there are no other tracks belonging to the same stream that you might care to sync with.

For stream.onaddtrack/onremovetrack my opinion about being consistent with how ontrack is handled is stronger.

+1 for alvestand's "I don't like letting users' JS inspect the state of the machinery while the events caused by a single causal event are being played out". Also agree that it can be confusing either way and no obvious right answer.

[jan-ivar]

@henbos On a remote stream with two tracks, as a user, if you did:

  stream.onremovetrack = () => console.log(stream.getTracks().length);

...and then called SRD with a media description that removes both tracks, would you expect:

1
0

or

0
0

?

I worry the latter might surprise people and introduce hard to spot bugs, like:

let stream;

pc.ontrack = e => {
  if (!stream) {
    stream = e.streams[0];
    openSomeResource();

    stream.onremovetrack = () => {
      if (!stream.getTracks().length) {
        closeSomeResource(); // Why is this called twice?
      }
    }

    stream.onaddtrack = () => {
      if (stream.getTracks().length == 1) {
        openSomeResource(); // Re-open as needed. Why is this not called?
      }
    }
  }
}

[henbos]

I agree this can be a little confusion. I argue that the alternative is also confusing, perhaps more so, and doing it this way is the right thing to do. Forgive the wall of text, I hope I get my point across and that I'm not worrying prematurely.

Generally speaking, events are typically scheduled to fire later, so by the time they do the effects of multiple events have already occurred. I think this is bad because the event handler gets fired with objects whose state are not even guaranteed to represent the state that caused them to fire.

Firing the events before the last step is not only important because we want the events to fire before the promise resolves, but because if we don't then calling SRD twice or doing other things before the events fire will yield track information in the event handlers that don't correlate with the event that caused them to fire.

I also argue that we cannot fire events midst-processing, or else the event handler can both inspect and influence the state of processing algorithm. Something as simple as "for each transceiver, fire an event on the transceiver" would be problematic because the event handler could modify the set of transceiver (or any other state of the PC that the processing algorithm might use) while we are iterating them.

What if you close the pc or stop or add a track in one of the event handlers? The SRD algorithm would have to handle substeps of the algorithm failing. Even if we know we are safe for now, this is not resilient to future changes and implementations are error prone if they don't think about this stuff, like copying or making sure any container is iterable even if it's modified during iteration or else we might crash.

I argue we must:

1. Fire all events after any processing steps.
2. Have fired all events before the last step (before promise is resolved).

As for your question, what do I expect?

0
0

The guarantee is: the state of any object in an event is up-to-date with the result of the SRD call. Meaning the stream is empty.

This is consistent with ontrack. If you add two tracks to a stream, inspecting the stream in the first ontrack yields it contains two tracks. If this wasn't the case you would have a similar problem:

// Imagine streams only contain the tracks whoose ontrack has fired.
pc.ontrack = (e) => {
  // Shared streams are processed multiple times for each ontrack,
  // we don't know which event represents the final state of the
  // stream, and this is sensitive to the machinery. It's better
  // that each object in any event fired represent the end-result
  // of the SRD.
  processStreams(e.streams);
};

Guaranteeing the state of the objects represent the end of the SRD is easy to understand, consistent with other event mechanisms and safer and easier to implement.

People should write code handling the end-result, not half-way results. They shouldn't have to care about whether adding or removing streams is implemented as a an iteration of smaller operations or as an all-or-nothing operation.

[jan-ivar]

@henbos Thanks, I think these are important points to discuss, can you open a new issue for them? The PR here is an incremental improvement that takes care of firing before SRD.

[jan-ivar]

To clarify, while the PR represents our intent to have these events fired before SRD, I realize some of the problems you raise now need to be solved.

[henbos]

Thanks, +1 to merging this assuming my points are discussed elsewhere. I filed #1691.