Question about when to check if user's class implements AudioWorkletProcessor

[ArnaudBienner]

Describe the issue
Currently the spec states that a UA should check if a given processor implements the AudioWorkletProcessor interface at construction time [1.33.5]
I wonder why this is not done when calling registorProcessor in the AudioWorkletGlobalScope, since there we already check a bunch of things (existence of processor method, etc.) 1.33.2.2

I believe it would be better for the user to know right away if a processor is not valid, instead of failing at construction time later on.

Where Is It
See links above.

Additional Information
I'm not an expert on all those things, so forgive me if I missed the reason why it's like this :)

[hoch]

I think this is somewhat related to the lack of error report mechanism of AudioWorkletGlobalScope. registerProcessor() runs on the worklet thread thus the associated exceptions will be handled by AWGS. Unfortunately AWGS is not equipped with the error handling and user won't get notified by it. It silently fails.

However, we still have the step 6 in registerProcessor:

If the result of IsCallable(argument=Get(O=prototype, P="process")) is false, throw a TypeError and abort these steps.

This will just work when WGS has a proper error reporting channel.

Also the reason why we have another check in the AudioWorkletProcessor instantiation because here we can properly deliver the error in the construction process with onprocessorerror. This is a practical approach and it is under our control. (no external dependency)

So I believe the current spec technically doesn't have an error - the problem here is that the context above is not really well explained anywhere.

[ArnaudBienner]

OK, thanks for the clarification.
But sorry: I'm not quite convinced though :)
However, I don't think there is any error here: I just believe the current spec is confusing.

• Agreed WGS lacks a proper error mechanism to report, making it impossible to know exactly why addModule would fail.
  Since everything logged would end up in the console, it's OK for debugging purpose I guess, but I can imagine usecases where you want to programmatically check the error type if something fails.
  But this point should be discussed in [audioworklet] Error handling in AudioWorkletGlobalScope #1282 you're right.

• What I don't like here is the incoherence: some errors would be raised when calling addModule. Others when creating the AudioWorkletNode.

1. I'm in favor of raising errors as soon as we can (so in AWGS::registerProcessor).
2. If doing so is not a good option (because error reporting is not good), then we should take the opposite way, and check as much things as we can when creating AudioWorkletNode, not when calling registerProcessor (so we can more precisely catch errors).

Looking at registerProcessor spec I believe the following checks could be moved to AudioWorkletNode's constructor: 6, and maybe 3, 4, 5.
1, 2, 7, 8, 9, 10, 11 should be kept obviously, since they retrieve and checks parameters which will be stored in the maps (and the purpose registerProcessor is to fill those maps IIUC).

But, as said, I prefer option 1) meaning we would move 1.32.5 check 6) to registerProcessor.
I prefer this because:

• Coherency: perform all checks on registerProcessor
• The sooner we can let author know something is wrong, the better IMHO
• Won't change much the behavior: if registerProcessor fails because if this new check, then AudioWorkletNode's constructor will still fail, because this processor won't be in the map 1.32.5 check 2)

I know the spec is kinda frozen now, so I would understand if it's too late to change anything, but want to share my thoughts anyway :)

[ArnaudBienner]

A more serious issue is: can a AudioWorkletProcessor definition be modified after being successfully registered using registerProcessor?
If that is the case, if e.g. "process" can be removed or changed to something that is not function, it would make sense to perform all those checks in AudioWorkletNode's constructor, not in registerProcessor (or in both places).
But I also wonder how much of these can modified even after AudioWorkletNode is created, e.g. removing "process" after receiving a message through MessagePort, and what should happen in this case (do we kept a reference on the original "process" function? Do we retrieve it everytime, and will through if not callable anymore?) It might be worth clarifying this in the spec as well, but I'm unsure if anything I said could be valid.

[hoch]

Re: #1767 (comment)

But, as said, I prefer option 1) meaning we would move 1.32.5 check 6) to registerProcessor.

I think you meant 4) and 6) which are:

4. If processorConstructor is undefined, throw a NotSupportedError DOMException.
5. If processor does not implement the AudioWorkletProcessor interface, throw an InvalidStateError DOMException.

I, too, prefer coherency. But if you move these steps to AWGS.registerProcessor(), how can developers catch the potential error? I've already seen that developers are using onprocessorerror in a meaningful way (an example), so I want to be careful about taking things away from that feature. Any error happens in the AWP construction or processing can be caught by onprocessorerror. However, registerProcessor() can't do that.

Won't change much the behavior: if registerProcessor fails because if this new check, then AudioWorkletNode's constructor will still fail, because this processor won't be in the map 1.32.5 check 2)

IMO this is actually a regression. Because the error that user will see at the AWP construction is "the 'x' node is undefined", unless you go extra miles and craft some clever error messages in there.

I know the spec is kinda frozen now, so I would understand if it's too late to change anything, but want to share my thoughts anyway :)

I would say it is not completely frozen - and I really appreciate you raising the issue. The more we talk about it, perhaps we can build a stronger case for WorkletGlobalScope's proper error handling.

[hoch]

Re: #1767 (comment)

A more serious issue is: can a AudioWorkletProcessor definition be modified after being successfully registered using registerProcessor?

The step 2 of registerProcessor() algorithm prevents it.

But I also wonder how much of these can modified even after AudioWorkletNode is created, e.g. removing "process" after receiving a message through MessagePort

I think this is possible. As long as process() method has the correct signature, it should be okay. This is not about changing the class definition itself, but swapping the local function in an instance. If JS allows this behavior, I think we should leave it as is. We can certainly be more rigorous by checking IsCallable() in the rendering loop for every AWP instances, but I would prefer to have less checks in the rendering loop.

[ArnaudBienner]

IMO this is actually a regression. Because the error that user will see at the AWP construction is "the 'x' node is undefined"

So what about option 2? Moving some checks from registerProcessor to AWP construction.
Because if, said, AWP doesn't provide "process" function, the error that user will at construction time is "the node is undefined" (because registerProcessor in addModule failed, with no easy way to tell you why, as you pointed).

unless you go extra miles and craft some clever error messages in there.

Maybe something like "node is undefined. Are you sure the name is correct, and that the node was successfully registered?" or something like this? Similar to those hints your C++ compiler will give you when something goes wrong. Not very clever but would work.

[ArnaudBienner]

If JS allows this behavior, I think we should leave it as is. We can certainly be more rigorous by checking IsCallable() in the rendering loop for every AWP instances, but I would prefer to have less checks in the rendering loop.

We don't necessarily need to check IsCallable everytime, nor to prevent this behavior.
I'm just unsure what should happen in this case.
I believe calling a function that doesn't exist should throw a TypeError (by JavaScript spec), and that would be OK.
I was just wondering if this needs to be clarified in the WebAudio spec or not.

[hoch]

Re #1767 (comment):

So what about option 2? Moving some checks from registerProcessor to AWP construction.
Because if, said, AWP doesn't provide "process" function, the error that user will at construction time is "the node is undefined"

Yes, perhaps this is something we can do here, but then our register function diverges from CSS Paint API. (See step 18, 19 in the algorithm) I don't think it's necessarily a bad thing, but just so we know.

Also this actually goes against your argument, letting user know when something bad happens as soon as possible. I actually agree with it, but the option 2 is the opposite of it.

Maybe something like "node is undefined. Are you sure the name is correct, and that the node was successfully registered?"

This is UA-dependent. The spec can't mandate what sort of error messages would be printed out. We may end up vague error messages across different browsers and that was my main concern.

I don't think checks being scattered to two different places is really bad. If they serve legitimate reasons, it is better to have them there instead of merging them for an artificial reason.

[hoch]

We don't necessarily need to check IsCallable everytime, nor to prevent this behavior.
I'm just unsure what should happen in this case.
I believe calling a function that doesn't exist should throw a TypeError (by JavaScript spec), and that would be OK.
I was just wondering if this needs to be clarified in the WebAudio spec or not.

Yes, now I can see this can be actually an issue. If a process() is swapped in the middle of rendering and it's not a valid "callable" function, this should be notified to the node via onprocessorerror after the internal check. (Because you can't wrap the function itself with try/catch)

But in order to do that, we still need to check the function every single render quantum. I peeked V8's IsCallable() function and it seems to be trivial, so I don't mind adding it to the spec.

[ArnaudBienner]

Also this actually goes against your argument

I know :)
I said we have two places/options where to check the code provided by the user.
You have arguments for option 2) and you're expert. So if option 2) is really the best (because error handling is easiest here) I suggested we perform all the checks there (because coherency: this was my main concern).

I know Chrome already implemented AudioWorklet and shipped it, so I understand concerns about modifying this part of the spec (and possibly breaking existing applications). It's just that from my point of view, the spec would be nicer if we are more coherent regarding the checks performed.

but then our register function diverges from CSS Paint API

Then they have a similar issue. It might interesting to raise the issue to them and see what they think.

This is UA-dependent.

Indeed it is, and UA are more likely to print a message like "node 'x' is undefined". But what the spec says is only to throw "NotSupportedError" and nothing else. So I would expect UA to pay attention to additional error messages to explain the issue. But you're right vendors might have not necessarily add a hint about why the node is undefined.
But again, with the current spec, the node will be undefined if user passed a class with no "process" function etc. so we already have those confusing cases (and option 2 would fix these).

But in order to do that, we still need to check the function every single render quantum. I peeked V8's IsCallable() function and it seems to be trivial, so I don't mind adding it to the spec.

Again, I'm just asking question :) I'm not sure if the current spec implies that an exception will be thrown and notified through onprocessorerror or not. I just have concerns about different UA implementing different behaviors.

[ArnaudBienner]

On those two issues discussed here, I would be curious to hear about other WG members. Maybe @rtoy @padenot have an opinion.
I'm just sharing my thoughts, and don't necessarily expect the spec to change. So if you believe the spec is OK as it is, we can close this issue.

[hoch]

Hey @ArnaudBienner! This is a great point and I really appreciate you raised this point. :)

Here's what we ended up in this telecon today:

1. Remove the redundant "process" check in the instantiation of AWP.
2. Add "IsCallable()" check before AWP::process() gets call in the rendering loop. If the check returns false, fire the onprocessorerror event on AWN.

I'll come up with the more detailed spec text soon. :)

[hoch]

After having a chat with @rtoy about the point 2 above, I believe there is an issue that needs a discussion.

• What if IsCallable() fails? How should the render loop treat this bad AWP instance?

I think we have at least 2 options:

1. Mark the current AWP instance "bad" and output silence from now on.
2. Output silence for this RQ, but do IsCallable() check again in the next iteration.

For both cases, the error should be reported to the associated AWN's onprocessorerror.

The problem of option 1 is that it is impossible to revive the marked AWP instance until the context is reconstructed. The option 2 can be also problematic because it will flood the onprocessorerror event handler by firing an event every ~3ms. Still, I think option 2 is better because you can recover from the bad status.

[ArnaudBienner]

I agree with you: 2 sounds better even though that means flooding the onprocessorerror event.

[karlt]

As currently spec'd, after an onprocessorerror event, the processor will
output silence, implying that process() is no longer called.

If this were to change such that attempts to call process() continue, then
the interaction with the "active source" flag gets more complicated. If the
flag is not reset, then process() calls on an unreferenced processor would
continue to fail (or throw), and onprocessorerror events would continue
indefinitely. The node would need to be kept alive to dispatch these events,
even though the node is otherwise useless.

One option may be to record the last error and only send another event on
a change in the error. I suspect that would make garbage collection possible,
but it would be somewhat complicated.

[hoch]

A good point. The proposed change (option 2) and the active source flag make the problem quite tricky. I guess this leaves us the option 1.

• If IsCallable() check fails on process on the AWP instance, mark the instance "bad" and output silence.
• Fire the associated node's onprocessorerror event.
• The AWP instance's process() method won't get called anymore.

Considering we're keeping the old behavior, the only change needs to be made here is adding IsCallable() in the rendering loop algorithm.

[ArnaudBienner]

LGTM