Bug 250485 - AudioWorkletProcessor fails to load when some parameter descriptors with minValue/maxValue are specified
Summary: AudioWorkletProcessor fails to load when some parameter descriptors with minV...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari 16
Hardware: Mac (Apple Silicon) macOS 13
: P2 Minor
Assignee: Nobody
URL: https://webaudio.github.io/web-audio-...
Keywords: BrowserCompat, InRadar
Depends on:
Blocks:
 
Reported: 2023-01-11 15:41 PST by Casey Primozic
Modified: 2023-01-22 04:57 PST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Casey Primozic 2023-01-11 15:41:01 PST
When defining `AudioWorkletProcessor`s that have parameter descriptors which define `minValue` or `maxValue` can cause the AWP to fail to register in some cases.

It happens when the `defaultValue` for the param descriptor is out of the range implied by `minValue` and `maxValue`.  For example, defining an AWP with this param descriptor fails:

```
static get parameterDescriptors() {
  return [{ name: "x", minValue: 0.01 }];
}
```

I believe this happens because the default `defaultValue` is 0, which is below the set `minValue`.  Changing it to this makes it register successfully:

```
static get parameterDescriptors() {
  return [{ name: "x", minValue: 0.01, defaultValue: 0.4 }];
}
```

The same thing happens for `maxValue` if `defaultValue` is set higher than `maxValue`.

I'm not sure what the spec says about situations like this, but it works in other browsers I've tested.

Here is a working minimal repro of the bug:

index.html:
```
<html>
  <head>
    <script type="text/javascript">
      const ctx = new AudioContext();

      ctx.audioWorklet
        .addModule("/awp.js")
        .then(() => {
          const node = new AudioWorkletNode(ctx, "bug-repro-awp");
          document.write("SUCCESS");
        })
        .catch((e) => {
          document.write("ERROR: " + e);
        });
    </script>
  </head>
</html>
```

awp.js:
```
class BugReproAWP extends AudioWorkletProcessor {
  static get parameterDescriptors() {
    return [{ name: "bottom_ratio", minValue: 0.01 }];
  }

  constructor(_options) {}

  process(inputs, outputs, params) {
    return true;
  }
}

registerProcessor("bug-repro-awp", BugReproAWP);
```
Comment 1 Casey Primozic 2023-01-11 15:44:45 PST
One thing I forgot to mention is that when this bug happens, the promise returned by `ctx.audioWorklet.addModule` itself doesn't reject.

The AWP registration failure happens silently, and the bug manifests when trying to create a `AudioWorkletNode` with the name of the processor.
Comment 2 Radar WebKit Bug Importer 2023-01-18 15:41:18 PST
<rdar://problem/104401007>
Comment 3 Chris Dumez 2023-01-20 13:57:24 PST
Likely due to the following logic in AudioWorkletGlobalScope::registerProcessor():
```
    if (!parameterDescriptorsValue.isUndefined()) {
        parameterDescriptors = convert<IDLSequence<IDLDictionary<AudioParamDescriptor>>>(*globalObject, parameterDescriptorsValue);
        RETURN_IF_EXCEPTION(scope, Exception { ExistingExceptionError });
        UNUSED_PARAM(parameterDescriptors);
        HashSet<String> paramNames;
        for (auto& descriptor : parameterDescriptors) {
            auto addResult = paramNames.add(descriptor.name);
            if (!addResult.isNewEntry)
                return Exception { NotSupportedError, makeString("parameterDescriptors contain duplicate AudioParam name: ", name) };
            if (descriptor.defaultValue < descriptor.minValue)
                return Exception { InvalidStateError, makeString("AudioParamDescriptor with name '", name, "' has a defaultValue that is less than the minValue") };
            if (descriptor.defaultValue > descriptor.maxValue)
                return Exception { InvalidStateError, makeString("AudioParamDescriptor with name '", name, "' has a defaultValue that is greater than the maxValue") };
        }
    }
```
Comment 4 Chris Dumez 2023-01-20 13:59:44 PST
Relevant specification:
https://webaudio.github.io/web-audio-api/#dom-audioworkletglobalscope-registerprocessor

```
For each descriptor of parameterDescriptorSequence:
1. Let paramName be the value of the member name in descriptor. Throw a NotSupportedError if paramNames already contains paramName value.
2. Append paramName to the paramNames array.
3. Let defaultValue be the value of the member defaultValue in descriptor.
4. Let minValue be the value of the member minValue in descriptor.
5. Let maxValue be the value of the member maxValue in descriptor.
6. If the expresstion minValue <= defaultValue <= maxValue is false, throw an InvalidStateError.
```
Comment 5 Chris Dumez 2023-01-20 14:01:58 PST
So it seems our implementation is close to the specification text. What may be surprising though is that the addModule() promise apparently doesn't get rejected, even though the module script got an exception when running.
Comment 6 Chris Dumez 2023-01-20 14:55:32 PST
(In reply to Chris Dumez from comment #5)
> So it seems our implementation is close to the specification text. What may
> be surprising though is that the addModule() promise apparently doesn't get
> rejected, even though the module script got an exception when running.

Hmm, it actually seems it is per specification to not reject the promise if the audio worklet script throws an exception.

See https://wpt.live/worklets/audio-worklet-import.https.html
Test: Importing a script which throws should still resolve the given promise.

If I forward the exception from the audioworklet to reject the promise, I start failing this standards test.
Comment 7 Chris Dumez 2023-01-20 14:59:09 PST
(In reply to Chris Dumez from comment #6)
> (In reply to Chris Dumez from comment #5)
> > So it seems our implementation is close to the specification text. What may
> > be surprising though is that the addModule() promise apparently doesn't get
> > rejected, even though the module script got an exception when running.
> 
> Hmm, it actually seems it is per specification to not reject the promise if
> the audio worklet script throws an exception.
> 
> See https://wpt.live/worklets/audio-worklet-import.https.html
> Test: Importing a script which throws should still resolve the given promise.
> 
> If I forward the exception from the audioworklet to reject the promise, I
> start failing this standards test.

Relevant specification for addModule(): https://html.spec.whatwg.org/multipage/worklets.html#dom-worklet-addmodule
Comment 8 Chris Dumez 2023-01-20 15:03:59 PST
Chrome/Blink indeed doesn't throw for invalid parameter descriptor values, I looked at their code and they have a FIXME comment about it:
      // TODO(crbug.com/1078546): The steps 7.3.3 ~ 7.3.6 are missing. 

Steps 7.3.3 ~ 7.3.6 are exactly the ones about validating the parameter values.
Comment 9 Chris Dumez 2023-01-20 15:14:20 PST
I filed https://github.com/WebAudio/web-audio-api/issues/2534 and cc'd the Blink engineer who works on this, and who is also an editor for this spec.

At the moment, our behavior seems the most spec-compliant, though not interoperable with Blink. This is not an ideal situation but it is not clear yet how we should proceed.