-
Notifications
You must be signed in to change notification settings - Fork 168
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
Reconcile the processing algorithm with the worklet event loop #1511
Comments
Let's start with these:
My thoughts:
@padenot: so your last comment only applies to the point 1:
I think the order of queued tasks should follow its scheduled order. Do you mean we need to prioritize and change the order of queued tasks in the command queue somehow? Other than that, I think we just need to update the processing model to reflect the points 1 and 2. |
I assume the rendering thread would or at least could check for available The model of queuing messages from the main thread on the rendering thread Support for atomic changes to the graph can be provided by queuing messages Similarly, batching messages from the rendering thread to the main thread would |
We had a productive call yesterday about this. The proposed solution is to call the Say we're using a promise in an Say we're using a promise in the Those scenarios are just examples. Normative description of what MUST happen is in the event loop section of the HTML standard. There should not be a prioritization between
In addition to point 1 and 2, we want to make |
It sounds like you want to use "queue a task" and have that task run |
@padenot If you can agree on this, the rest should fall out automatically like @annevk said. All in all, any task in the render thread should be executed by the task scheduler (i.e. the command queue in our processing model), and that is how Chrome's AudioWorklet behavior on the render thread is implemented. |
I would like to add few more points here. I think this issue is closely related to the recent comment from TAG or it
I think we should create another issue to track the processing model change and make this as "blocked" by that change. |
The whole question here is where and how many event loops we have. You seem to imply in this comment (sorry if I misunderstood), that we have an even loop per
If we decide on a model where we have a single event called
I think it's rather frowned upon to call into not from its own task, but I don't remember exactly if this is true or if I'm confusing this with something else, nor the reason. Now, to try to be a bit more practical, say I have two AudioWorkletProcessor: class A extends AudioWorkletProcessor {
console.log("A");
var p = new Promise(function(resolve, reject) {
// compute
resolve("promise A");
});
p.then(function(str) {
console.log(str)
})
}
class B extends AudioWorkletProcessor {
process(inputs, outputs, parameters) {
console.log("B");
var p = new Promise(function(resolve, reject) {
// compute
resolve("promise B");
});
p.then(function(str) {
console.log(str)
})
}
} and then this document: var ac = new AudioContext();
let a = new AudioWorkletNode(context, 'A');
let b = new AudioWorkletNode(context, 'B');
a.connect(ac.destination);
b.connect(ac.destination); If we ignore the issue of the processing order (that is an issue in itself, but let's ignore it for now), an say that A is always processed before B. In the first model, we have:
in the second model we have:
Trying to synthetize the issues here:
We have to decide what behaviour we want here, it is clearly possible to define two implementation that do each do something different: just in this thread we have two or three possible definitions that are clearly implying different results for the same script. It's unclear to me yet which one of the two is preferable (I'm still thinking about it), but it's clear that we can't reach interoperability until this is resolved. I think I'm leaning towards the first solution, though, but I don't know whether spinning event loops like this has issues. |
That's why I asked the clarification about I am really against making the graph rendering task non-atomic. What if your promise resolution involves the any state change in the other processor? For this reason, Chrome implementation has a graph mutex the protect the graph during the rendering task. For the optimum rendering performance, we have to ensure the rendering task to be atomic. I strongly prefer the second model.
Do we have any precedence of multiple event loops in the single thread/scope? |
The promise resolution we're talking about happens inside the Also, I've been trying to find prose about the creation process of an
Chrome currently has one This means we would have a single event loop on which all the Anyways, this should be in the spec. Atomicity is orthogonal to the issue at hand here, we're only even dealing with one thread, the rendering thread, and as you mention, it's clearly out of the question to stop it or pause it or whatnot, as we all know.
That I don't know, @annevk, do you have an answer? |
There's no precedent for that. That sounds a little weird, though maybe it's actually different from a single loop with multiple task sources, which we do have? |
We will have to think about the mutation, but it certainly breaks the atomic nature. Any random task can be queued in the event loop between process() calls ordered by the algorithm. Perhaps we are seeing the term "atomic" differently.
I think this has more context. But I also don't think allowing more than one AWGS per BAC is really useful. I remember @joeberkovitz wanted to loosen the text for the future expansion.
Technically it is created and ready when addModule() gets resolved. This is defined in Worklet API. Not sure if we need to add more stuff on top of it since the algorithm looks well-defined.
That's correct. You can think of it as one possible implementation based on the current spec: "BAC may have at least one AWGS".
In Chrome's implementation, the graph render task is not supposed to be interrupted to ensure the optimum rendering performance. If we have to wrap each process() function to insert it to the task runner (i.e. event loop), there will be price to be paid. I will have to profile it to get the actual numbers, but this is a significant change that will affect users for sure. I also want to emphasize that the atomicity is not an AudioWorklet's problem anymore. This change applies all the processors (as in a counterpart of AudioNode) and will have the real world impact if it forces drastic changes in the existing implementation. Not to confuse the discussion, I think we should start a new issue on the revision of the processing model. |
Either you have to run the event loop tasks in between
I agree with you. Again, it is not possible with the current text to write implementation that are interoperable, we should mandate that there is a single scope or define how those
If we decide to have a single one, I think we're good indeed, but we have to decide that. Again I'm in favor of having a single scope.
Of course, this is the case for any implementation. I don't see that being a problem in practice, but it's an implementation concern, and I don't know whether your event loop infrastructure in Chrome suits that. Also, keep in mind that we are not talking about interrupting anything here, we're talking about, for example, putting an event in the event loop (a
Running script in between a the rendering of nodes, considering they can't modify the graph or block or whatever, is akin to running the script for an Yes, anybody will be able to write very bad code and overload the audio callback, but they can do so in the |
The other reason is to keep the rendering performance optimal. What if your app pushes a tons of Lastly, what is the real-world benefit of braking the render task down to an individual
Do you think we have "additional processes" on top of what is defined in the Worklet spec? I believe only thing we're missing is a clear one-to-one association between AudioWorklet and AudioWorkletGlobalScope. (c.f. CSSPaintWorklet can have 2 PWGS)
Great! I am also in favor of having a single scope. |
The order or This is the case because we are going into the direction of having a single scope, and one can hook arbitrary data on the scope (via expandos), and they are visible to other processors (this is a feature, it allows data sharing between processors, allowing, for example in turn share large assets across processors, saving tons of memory and CPU).
Authors can write arbitrary scripts and destroy the performance, this is not a very strong argument. If you want to have a high performance app, obviously all the messages will be batched into a single We have to have a defined ordering of If you really don't want to have a break in between process calls, it's rather straightforward as well: we just spec that a full iteration of the processing model algorithm runs off a task (i.e. the rendering of a render quantum in web audio terms). In this case,
Yes, this gates minimal input latency on high load. If we don't do that, we'll have a minimal input latency for a 44100Hz context of, say 128/44100, and an average and maximum latency much higher than that. It's rather important to not assume use cases here. It might be that this event is going to fire in reaction to a web midi NoteOn event, because a keyboard player hit a key on a MIDI keyboard. The latency here can be absolutely critical.
Right, but we have to formally decide this.
Does that sound good to other ? @joeberkovitz @mdjp @svgeesus. |
I am in favor of a single global scope for each context. I also find it much easier to think about things, if graph rendering happens all at once and the messages are handled after rendering (or before). |
I disagree. Finishing the render task (or The API will abused and we won't be able to stop the glitch no matter how we spec it. But we should not target for those abusive cases. IMHO, we should care about developers who care. The real-time audio has been a primary use case for Web Audio API, so I believe maintaining certain constraints to keep that quality is necessary.
The MIDI input latency from the scheduling should be less than ~3ms, if an iteration finishes in the deadline. Having a minuscule delay on MIDI event is much better than glitches in the stream. Also the MIDI data received in the current render quantum would not make difference until the rendering of next quantum begins. So I don't think it really matters. |
You mention that you're taking a lock when you're rendering from the audio thread. This must be because you're executing the script for It's in fact strictly worst because the audio thread (that has often a higher priority), will be stalled, instead of simply possibly getting a bit more scheduling slices from the system because it's set to be non-preemptible (this is possible on macOS, and is what browsers do in practice). There is no inheritance on concurrency primitives on consumer non-real time modern OSes, so the bump will not occur across threads because a thread with high priority is waiting on a mutex that is currently acquired by a thread with a lower priority. This is a classic priority inversion issue, well documented in the literature. You can also decide to do a trylock before trying to call the
I strongly agree, but the constraints are both on the implementation and on the authors. |
Perhaps I misled you somehow. In Chrome, all the operations in the audio thread are carefully written not to use lock, or use trylock if unavoidable. It has nothing to do with |
This testcase may be useful to highlight what I assume are the two contending
In the testcase, the behavior in response to
but a user event generates
|
What we have agreed upon so far:
I believe The next step for this issue is to write up a PR. Or do we have something else to discuss here? |
Considering that we'll be using this new construct to describe normative behaviour (on your second point), I don't think it's possible to have it non-normative.
I think you're right and we need to write the text. |
I am dragging you into this, so we can work on the write up together in the upcoming F2F. :) |
Just surveying the thread prior to call: I agree with @hoch's previous comment on what seems agreed, but would add the following suggested points (distilled from the conversation above):
|
For now, the AudioWorklet text uses things that need an event loop to work (
postMessage
,onmessage
,new Promises
, that kind of thing), but it's not defined how it all works.It must be defined, otherwise it can't be implemented properly.
For example, does
onmessage
execute before or after the call toprocess
? On which thread? When does resolved promises execute?This was discussed but forgotten about in #1193 (comment).
The text was updated successfully, but these errors were encountered: