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

Make MIDIAccess transferable and exposed to workers #256

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

mjwilson-google
Copy link
Contributor

This is to fix #99.

Some things to note:

  • I wrote in the issue "WorkletGlobalScope" but I think it's supposed to be Worker, correct? I will edit my comment in the issue if so.
  • I don't fully understand the implications of these tags or how much implementation work it will be, so please comment if there is anything that should change or if there is anything missing.

@mjwilson-google mjwilson-google added this to the CR milestone Oct 17, 2023
@mjwilson-google mjwilson-google self-assigned this Oct 17, 2023
@cwilso
Copy link
Contributor

cwilso commented Oct 17, 2023

I think at this point it's more useful in Worklet (so it's exposed in AudioWorklet) than worker.

@padenot
Copy link
Member

padenot commented Oct 17, 2023

I think at this point it's more useful in Worklet (so it's exposed in AudioWorklet) than worker.

We've discussed this at TPAC and concluded this is best done by authors, like it is in native code.

@padenot
Copy link
Member

padenot commented Oct 17, 2023

I'm wondering if we should make it transfer-only, and refuse regular cloning. I think it would simplify things somewhat.

@hoch
Copy link
Member

hoch commented Oct 17, 2023

I think at this point it's more useful in Worklet (so it's exposed in AudioWorklet) than worker.

Re: @cwilso - What @padenot stated above means that the group believes this works better with Worker and SAB. Also the feature set in the WorkletGlobalScope is intentionally minimum, so we want to make sure all interfaces used by Web MIDI are properly supported by it.

If incoming MIDI events are bound to AWGS, we'll see a lot of bursty actions in the performance of audio callback. Creating new events is "allocation", and we don't want to do that on the RT audio thread.

I'm wondering if we should make it transfer-only, and refuse regular cloning. I think it would simplify things somewhat.

@padenot Do you have any example of that?

@mjwilson-google Have we confirmed that all the interfaces in the Web MIDI API are supported in the WorkerGlobalScope? I am assuming it does, but it doesn't hurt to check. :)

@cwilso
Copy link
Contributor

cwilso commented Oct 17, 2023

@hoch hmm, yeah, you're right, makes sense. I retract my previous statement. :)

@mjwilson-google
Copy link
Contributor Author

@hoch I don't know how to confirm that the interfaces are supported in WorkerGlobalScope. How do we do that?

@hoch
Copy link
Member

hoch commented Oct 17, 2023

From the spec(https://webaudio.github.io/web-midi-api), I see these interfaces:

5.1 MIDIInputMap Interface
5.2 MIDIOutputMap Interface
5.3 MIDIAccess Interface
5.4 MIDIPort Interface
5.4.1 MIDIInput Interface
5.4.2 MIDIOutput Interface
5.5 MIDIMessageEvent Interface
5.6MIDIConnectionEvent Interface

This PR only has a change for MIDIAccess, so it needs to be expanded to add the Worker exposure (e.g. [Exposed=Worker]) to all the interfaces.

@mjwilson-google
Copy link
Contributor Author

This PR only has a change for MIDIAccess, so it needs to be expanded to add the Worker exposure (e.g. [Exposed=Worker]) to all the interfaces.

Done. Is it fine to keep Transferable only on the MIDIAccess interface?

@padenot
Copy link
Member

padenot commented Oct 18, 2023

Done. Is it fine to keep Transferable only on the MIDIAccess interface?

In terms of developer ergonomics, I think it's fine.

The author performs the request from the main thread. This deals with the all the permission business. The author then transfers the MIDIAccess object, and does all the midi things in the Worker. This means that there is generally only one thread that deals with MIDI (it's always possible to get other MIDIAccess however).

We can also decide to allow sending ports and such to the worker, but I think I like that style less.

Copy link
Member

@hoch hoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mjwilson-google
Copy link
Contributor Author

@padenot How does the current state look? Do you want to explore making the object transfer-only? Would the way to do that be to mark it as non-serializable, to disable cloning?

We will need a follow-up PR to further define the behavior when transferred before closing the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIDI API should be available from Workers?
5 participants