Skip to content
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

AudioWorkletProcessCallback should not be a callback type #2581

Open
domenic opened this issue Apr 3, 2024 · 6 comments
Open

AudioWorkletProcessCallback should not be a callback type #2581

domenic opened this issue Apr 3, 2024 · 6 comments
Assignees
Labels
category: editorial P2 Significant developers/implementers interest; "want to have"

Comments

@domenic
Copy link
Contributor

domenic commented Apr 3, 2024

AudioWorkletProcessCallback is defined, but it is never called. Instead it seems to be misusing Web IDL to define the sort of method that web developers should put on their audio worklet classes.

Web IDL is not an appropriate language for such a description, since it has consequences for bindings generators, and the callback type has a very specific meaning in terms of what happens when you invoke them.

Instead, prose would be better.

Removing this misuse of Web IDL would help with whatwg/webidl#1399.

@mjwilson-google
Copy link
Contributor

mjwilson-google commented Apr 3, 2024

I may not be understanding the issue, but I believe the callback is called in the rendering a graph algorithm (step 4.4.4).

I also see in the linked issue that you are skeptical of performance requirements. AudioWorklet process functions could potentially be called up to once every render quantum (2.6 ms at 48 kHz sampling rate) so it is important that we give implementers as many options as we can to help make their implementations performant. This includes avoiding memory allocations when possible. Is there a way to avoid extra allocations when using sequences?

EDIT: It seems like this issue isn't about changing WebAudio's use of FrozenArray, but the use of Web IDL to define AudioWorkletProcessCallback. In which case, the above note on performance isn't relevant.

@inexorabletash
Copy link

I think @domenic's point is that while the spec has prose like:

The subclass MUST define an AudioWorkletProcessCallback named process() that ...

This isn't something Web IDL enforces or participates in. The prose in rendering a graph algorithm (step 4.4.4) does all the necessary lifting and integration with JS, so the Web IDL definition is (mostly) not adding value, and its presence is blocking other cleanup.

I said "mostly" because there's a caveat. The substeps have:

  1. Let args be a Web IDL arguments list consisting of inputs, outputs, and parameters.
  2. Let esArgs be the result of converting args to an ECMAScript arguments list.

In this, the args (inputs, outputs, parameters) link to the AudioWorkletProcessCallback definition, so it is currently serving as the definition for how to convert from IDL values to ECMAScript values, which needs to go somewhere but could be in prose inline. I think the current text is equivalent to something like:

  1. Let args be a Web IDL arguments list consisting of « FrozenArray<FrozenArray<Float32Array>> inputs, FrozenArray<FrozenArray<Float32Array>> outputs, object parameters ».
  2. Let esArgs be the result of converting args to an ECMAScript arguments list.

Which... doesn't make sense to me - it's mixing values with types? Where do the values come from?

@hoch
Copy link
Member

hoch commented Apr 3, 2024

I think this was my attempt to make the connection from a Web IDL arguments list into an ECMAScript arguments list.

The concept of AudioWorkletProcessor's process function is rather unique at the time of writing, but it might not up to the standard of today.

To answer @inexorabletash's questions:

Where do the values come from?

input buffer and input AudioParam buffer are described in the step 4.4.1.1 and 4.4.2. They are available for the AudioWorkletProcessor tasks.

I also followed up on the original issue with a question about FrozenArray.

@petervanderbeken
Copy link

As @domenic pointed out, the callback is not used at all in the algorithms. In fact the result of Get(O, "process") is never actually converted to a WebIDL callback.

To answer @inexorabletash's questions:

Where do the values come from?

input buffer and input AudioParam buffer are described in the step 4.4.1.1 and 4.4.2. They are available for the AudioWorkletProcessor tasks.

I also find it confusing, and it's unclear to me too how these relate to the arguments that are converted to ECMAScript values. Note that one talks about input buffer and the other about inputs, you're saying these are the same?

The spec links to https://webidl.spec.whatwg.org/#web-idl-arguments-list-converting, so it depends on what type of WebIDL values these are originally (regardless of the argument types in the callback). There is nothing in the spec that actually creates a FrozenArray or creates a FrozenArray from an iterable, so if inputs is a FrozenArray then where is it actually created? If it's not, what type is it and why doesn't the callback take that type? As is I don't think this works.

Given that the WebIDL spec has a way to create a FrozenArray and a way to convert a FrozenArray to an ECMAScript value and this spec doesn't use the WebIDL callback, it seems unnecessary and you can just do this in prose (and make it actually work correctly).

@hoch hoch self-assigned this May 2, 2024
@hoch hoch added category: editorial P2 Significant developers/implementers interest; "want to have" labels May 2, 2024
@hoch
Copy link
Member

hoch commented May 2, 2024

@chrisguttandin Any thoughts on this issue? Is your library (TypeScript) going to be impacted by this spec change?

@chrisguttandin
Copy link
Contributor

standardized-audio-context doesn't provide any AudioWorkletProcessor types. There is a semi-official @types/audioworklet package. It defines the process() arguments as regular arrays.

interface AudioWorkletProcessorImpl extends AudioWorkletProcessor {
    process(inputs: Float32Array[][], outputs: Float32Array[][], parameters: Record<string, Float32Array>): boolean;
}

https://www.npmjs.com/package/@types/audioworklet?activeTab=code (index.d.ts line 273 to 275)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: editorial P2 Significant developers/implementers interest; "want to have"
Projects
None yet
Development

No branches or pull requests

6 participants