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

registerProcessor is doing odd things with threads and JS values #1945

Closed
bzbarsky opened this issue Jun 11, 2019 · 10 comments
Closed

registerProcessor is doing odd things with threads and JS values #1945

bzbarsky opened this issue Jun 11, 2019 · 10 comments
Assignees
Projects
Milestone

Comments

@bzbarsky
Copy link

https://webaudio.github.io/web-audio-api/#dom-audioworkletglobalscope-registerprocessor step 10 does:

Queue a task to the control thread to add the key-value pair (name - descriptors) to the node name to parameter descriptor map of the associated BaseAudioContext.

but descriptors can be a JS Array at that point. Is the reference to this Array being handed to a different thread? How is that supposed to work?

@hoch
Copy link
Member

hoch commented Jun 12, 2019

Yes, we missed the structured serialization step:

  1. Let pair be the ordered map «["name" → `name`, "descriptors" → `descriptors`]».
  2. Let serializedPair be the result of StructuredSerialize(pair).
  3. Queue a task to the control thread to add the key-value pair (name - descriptors) to the node name to parameter descriptor map of the associated BaseAudioContext, given the serializedPair.

I guess we still need to expand on what should happen on the control thread side, preferably in a form of algorithm? In short:

  1. Let paramDescriptorMap be node name to parameter descriptor map.
  2. Let deserializedPair be StructuredDeserialize(serializedPair).
  3. Let paramDescriptorMap[deserializedPair["name"]] be deserializedPair["descriptors"].

@bzbarsky
Copy link
Author

OK, so the descriptors value gets structured cloned over to the control thread? Presumably the initialization of AudioParamDescriptor dictionaries from the things in the array then happens on that thread and is therefore unobservable? What happens with validation failures that are supposed to happen for those dictionaries?

That is, is the actual desired flow that descriptors be converted to sequence<AudioParamDescriptor> synchronously inside registerProcessor, throwing exceptions as needed, then that sequence is sent over to the control thread side (probably conceptually via reifying it into a JS object in a clean global and then structured cloning that over to the control thread, though in practice browsers can optimize bits out there because they would not be observable)?

@bzbarsky
Copy link
Author

Or does the control thread really store descriptors as a JS Array of descriptor JS objects? If so, when are the AudioParamDescriptor dictionaries created?

@hoch
Copy link
Member

hoch commented Jun 13, 2019

The array of AudioParamDescriptor is provided from the user code. (See example 16)

The step 8 of registerProcessor() should be expanded so we can check if elements in the iterable conform with AudioParamDescriptor dictionary. Otherwise, it should throw.

That is, is the actual desired flow that descriptors be converted to sequence synchronously inside registerProcessor

It's already an iterable, but do you think we still have to pack it in sequence<>? Is this necessary for structured serialization?

@bzbarsky
Copy link
Author

The array of AudioParamDescriptor is provided from the user code.

Not quite, what user code provides is some JS Value, which you then try to extract a list of AudioParamDescriptor from. This extraction process should be clearly defined.

The step 8 of registerProcessor() should be expanded so we can check if elements in the iterable conform with AudioParamDescriptor dictionary

The only sane way to do that check is to actually convert them to AudioParamDescriptor dictionaries. Otherwise, given proxies, you could have objects that look like they conform when you ask, but stop conforming when you try to actually convert them to a dictionary.

It's already an iterable

What is?

but do you think we still have to pack it in sequence<>? Is this necessary for structured serialization?

If you want to support general iterables, then yes. Structured serialization knows nothing about iterables per se; it just knows about certain types of special objects.

You can structured-serialize the whole thing over and then do your extraction of the list of values and conversion of those values to AudioParamDescriptor on the other thread. It's just observably different behavior for custom iterables and other interesting objects (e.g. Proxy-based descriptors), and you still have to define what happens with things that can't be converted to AudioParamDescriptor (which can only happen after structured cloning in this case if they have no .name).

I don't necessarily have a strong opinion on which option you should pick here, honestly, but I do want the behavior to be defined.

That said, if I were designing this API, I suspect I would do the iteration and validation before the cross-thread send, at which point what is getting sent is a sequence<AudioParamDescriptor>.

@hoch
Copy link
Member

hoch commented Jun 13, 2019

I believe this issue can be resolved by specifying an algorithm of AudioParamDescriptor extraction, which is being discussed in here.

After the descriptor extraction (including validity check), we can send sequence<AudioParamDescriptor> over the thread. So this issue is blocked on #1946.

@hoch hoch self-assigned this Jun 13, 2019
@hoch hoch moved this from Untriaged to In WG Discussion in V1 Jun 13, 2019
@mdjp mdjp added this to the Web Audio V1 milestone Jun 25, 2019
@hoch
Copy link
Member

hoch commented Aug 15, 2019

Several clarification PRs have been merged, I think now we can tackle this:

  1. Introduce a new algorithm that builds a sequence<AudioParamDescriptors> out of the static parameterDescriptors() getter.
  2. Transfer the sequence to the control thread.
  3. On the control thread, append (name -> sequence) to node name to parameter descriptor map.

@hoch hoch moved this from In WG Discussion to Ready for Editing in V1 Aug 15, 2019
@hoch
Copy link
Member

hoch commented Aug 19, 2019

I am revisiting this and the current state resolved all the issues discussed here:
https://webaudio.github.io/web-audio-api/#dom-audioworkletglobalscope-registerprocessor (step 8)

But its numbering scheme is somehow broken, so I will fix that.

@hoch
Copy link
Member

hoch commented Aug 19, 2019

The number scheme will be fixed by #2030. Otherwise this issue is addressed.

@hoch
Copy link
Member

hoch commented Aug 23, 2019

#2030 is merged. Closing.

@hoch hoch closed this as completed Aug 23, 2019
V1 automation moved this from Ready for Editing to Done Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
V1
  
Done
Development

No branches or pull requests

3 participants