-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
How does generator.mute change track states? #81
Comments
Yes. We can either:
We cannot have both.
Seems desirable. Any assumption of the opposite would likely fail to consider that this affects all track clones from this generator (which can be an unbounded number), even ones transferred to workers (on different threads). While we could make an exception for same-thread tracks, I don't see what that would serve other than feed this assumption.
This also seems desirable to me. The prose is there to avoid the following firing 1000 events: for (let i = 0; i < 1000; i++) {
generator.muted = true;
generator.muted = false;
} ...because that seems very different from how hardware devices (which tend to run in the background) typically work. As a JS-based plug-in for what's normally a hardware device running in the background, being able to synchronously observe results on MST seems of low value to me (outside of tests). It seems better to me to separate timing of one side from the other. |
I don't understand the last argument. If I mute and unmute a track a thousand times, I expect to see a thousand mute events - not 999, not 1, not 0, but 1000. Anything else violates the principle of least astonishment. Also, this code: generator.muted = true; will then have a number of events fired that depends on both the content of and on the relative timing of the queued events. Again, violating the principle of least astonishment. If we change the prose to be: run the following steps:
we will get a totally predictable number of events. That seems desirable to me. (I agree with not changing the state of the track synchronously.) |
Do you mean:
That'll fire 1 event, not 1000, because of "2. If track.[[Muted]] is already newState, then abort these steps." That's assuming generator.[[isMuted]] isn't modified by JS during the other 999 tasks, which would then fire ghost events. Action at a distance. Or did you mean:
This would produce the following: Time --->
Let's hypothetically say each task takes 0.02 ms. That's 20 ms during which JS is led to believe the underlying device (generator) is flipping its muted state back and forth, when this isn't happening. That all happened on the same task 19.98 milliseconds ago, which wasn't enough time for any real background device to have had any back-and-forth reaction. Letting synchronous actions on a single task generate a (potentially unbounded) series of timed events like this seems like a bad idea. It also sets up a dissonance between what the JS observer sees and what is actually happening with the generator. |
We could flesh this out with a boolean [[HaveQueuedAlready]] internal slot if need be. |
I think there is no difference between the two alternatives, since all the "queue a task" operations are in the same execution cycle, so [[isMuted]] does not change value within the loop that queues the tasks. I was assuming that "queue a task" passes its arguments by value, not by reference. The second description makes it clear that it is pass-by-value, so that may be the better formulation of what I intended to say. |
Queue a task doesn't take custom arguments, just steps. So I read "queue a task to do X" as:
If X is "Use this.[[isMuted]] for something", then "this" is dereferenced when the queued task executes. This is similar to the forEach closure problem in JS.
Got it. But I still think that's undesirable. The With VideoTrackGenerator, we're opening up a way for JS to act as an underlying device, giving JS a new form of control surface that gives it new ways to control downstream and existing written JS. I think being conservative here means keeping some control over what JS can do in this new role, which to me means limiting things like setting muted many times in a row on the same task, which would be something downstream hasn't seen before, expands what we need to test for, and doesn't seem to serve any purpose |
I think we're at a fundamental difference of opinion here. You are arguing that we should insert behavior that is hard for the JS programmer to reason about and thus program to in the name of "conservatism". I am arguing that given that the mechanism exists, and we're giving access to it at all, we should give JS programmers the simplest model to reason about, so that they can do what they think is the Right Thing without worrying about the UA inserting arbitrary restrictions to limit what they can do with it. This UI does not prevent a lot of behaviors that could be considered unexpected, including:
And so on and so forth. Messing up people's mental model of what "mute a source" means ("mute all tracks, when tracks' muted state change they fire an event) because of a completely theoretical fear of the JS programmer might possibly do does not appear reasonable to me. |
Do we even need to specify behavior of track muted state in this spec? Some text from mediacapture-main for reference:
I don't see it as a necessity that muting a generator should synchronously mute all connected tracks. The UA can detect this and mute the tracks using any logic it wants (asynchronously if it so prefers). |
I don't think we can leave it as implementation defined. It's a specified API, and specified APIs should have specified consequences, unless we have strong reasons not to. I think we have consensus that the muting of tracks shouldn't be synchronous (because event firing is a synchronous operation, and that would put user JS inside the mute-a-generator processing steps - bad idea). I also think that we haven't surfaced any reason that the "mute a track" operation defined in the mediacapture-main spec needs to change. The discussion is all about when and how to call it. |
I agree with @guidou that muting as a model is something mediacapture-main defines — and that's as something happening asynchronously in the background — and that we need to respect that model here. I agree with @alvestrand we can't leave it unspecified, because it's JS observable (but I disagree about what's POLA). Web developers expecting synchronous actions to influence asynchronous operations a certain way should take this twitter poll, which highlights that any assumptions they make about synchronous and asynchronous interactions are bad ones. Applying that to our case: I don't see why it matters what the net muted state is "halfway through a JS call" (my new favorite phrasing of run-to-completion semantics). |
The POLA thing that worries me is this: generator.muted = true; "where did my mute go? There was no muted event fired!" |
This discussion died down, but reviewing it, I think I'm OK with the current language, where generator.mute and track.mute are not synchronous, and where the number of mute events fired by a series of changes to generator.muted depends on JS timing, rather than being totally predictable. |
The current spec approach makes sense to me. Maybe we could tidy up the language like we have done for MediaStreamTrack stats with task/task id... |
Discussed in WG meeting on Jan 16, 2024. Decided to go with the approach currently in the spec; no change needed. |
At the moment, the setting of "mute" has two levels of "queue a task":
("set a track's muted state" is a defined procedure in mediacapture-main that does not queue a task, but fires an event).
The existence of "queue a task" means that the following test snippet:
will fail. I suppose that's the price we pay for having an event fire when we change the state.
The double dispatch means that the following test snippet:
will not observe any event (because at the time the first task starts executing, "settledValue" will be false, which is "no change").
Would it be simpler to remove the outer "queue a task", so that an event is always fired when you change generator.mute, and the track's muted state changes exactly the same number of times as the generator's muted state?
I don't immediately see how to implement the "Unless one has been queued already" part either, or why its existence makes for more intuitive behavior of the API; I'd suggest dropping it.
The text was updated successfully, but these errors were encountered: