Web MIDI support. #47

MaulingMonkey · 2019-08-31T00:16:29Z

Concerns

Web MIDI is all kinds of asyncronous. Devices won't be available if you try to block request them immediately. It's possible that sending a MIDI device input immediately after opening it, if that input device requires system exclusive access, may not work. It'd perhaps be possible to buffer data until the promise for opening the device resolves...

Testing

C:\local\midir\examples\browser> wasm-pack build --target=no-modules --dev
...
C:\local\midir\examples\browser> "%ProgramFiles(x86)%\Google\Chrome\Application\chrome.exe" --allow-file-access-from-files --auto-open-devtools-for-tabs file:///C:\local\midir\examples\browser\index.html

Note that existing chrome instances may interfere with using command line flags - you may prefer to use the msjsdiag.debugger-for-chrome extension instead. Snippets from my local .vscode config:

.vscode\tasks.json

{ "label": "examples/browser: wasm-pack build --target=no-modules --dev",       "options": { "cwd": "${workspaceFolder}/examples/browser" }, "windows": { "command": "wasm-pack build --target=no-modules --dev",       }, },
{ "label": "examples/browser: wasm-pack build --target=no-modules --profiling", "options": { "cwd": "${workspaceFolder}/examples/browser" }, "windows": { "command": "wasm-pack build --target=no-modules --profiling", }, },
{ "label": "examples/browser: wasm-pack build --target=no-modules --release",   "options": { "cwd": "${workspaceFolder}/examples/browser" }, "windows": { "command": "wasm-pack build --target=no-modules --release",   }, },

.vscode\launch.json

        // msjsdiag.debugger-for-chrome
        // "runtimeArgs": See https://peter.sh/experiments/chromium-command-line-switches/
        {
            "name": "Chrome (--dev)", "type": "chrome", "request": "launch",
            "preLaunchTask": "examples/browser: wasm-pack build --target=no-modules --dev",
            "url": "${workspaceFolder}/examples/browser/index.html",
            "runtimeArgs": ["--allow-file-access-from-files", "--auto-open-devtools-for-tabs"],
            "internalConsoleOptions": "openOnSessionStart",
        },
        {
            "name": "Chrome (--profiling)", "type": "chrome", "request": "launch",
            "preLaunchTask": "examples/browser: wasm-pack build --target=no-modules --profiling",
            "url": "${workspaceFolder}/examples/browser/index.html",
            "runtimeArgs": ["--allow-file-access-from-files", "--auto-open-devtools-for-tabs"],
            "internalConsoleOptions": "openOnSessionStart",
        },
        {
            "name": "Chrome (--release)", "type": "chrome", "request": "launch",
            "preLaunchTask": "examples/browser: wasm-pack build --target=no-modules --release",
            "url": "${workspaceFolder}/examples/browser/index.html",
            "runtimeArgs": ["--allow-file-access-from-files", "--auto-open-devtools-for-tabs"],
            "internalConsoleOptions": "openOnSessionStart",
        },

Screenshots

Boddlnagg

Wow, this is awesome! Thanks a lot!

I have only played a tiny little bit with wasm so far, so there is a lot of things here that I don't have a good understanding for, but I'll try my best.

One thing that's a bit unfortunate is that I have a branch in development (#38) which is almost finished and ready to be merged, which changes the API. So all backends need to be adapted. Do you think that you can rebase the Web MIDI support on that branch?
It might even make the code leaner and cleaner because ports are no longer identified by the index and it seems that your DeviceSet might become simpler or superfluous then.

Concerning the asychronicity:
The issue seems to be in how to handle js_sys::Promise, right? I think the only way to really fix this is to introduce Rust's new async into midir's API. Which is probably a good idea anyway, but out of scope for this PR. (Can js_sys::Promise interoperate with async?)
You can open a follow-up issue so we don't forget about it, and that also serves as documentation about this "known issue" (i.e. that messages might be lost because the asynchronous port opening has not completed yet).

I have left some more comments inline.

examples/browser/src/lib.rs

src/backend/webmidi/mod.rs

Boddlnagg · 2019-08-31T06:21:56Z

src/backend/webmidi/mod.rs

+    input:          web_sys::MidiInput,
+    user_data:      Arc<Mutex<Option<T>>>,
+    #[allow(dead_code)] // Must be kept alive until we decide to unregister from input
+    closure:        Closure<dyn FnMut(MidiMessageEvent)>,


If that is how you have to do things for event handlers in js_sys, that seems a bit unsafe to me ... but probably it's the only way?

I just found https://rustwasm.github.io/wasm-bindgen/api/wasm_bindgen/closure/struct.Closure.html which explain this a bit.

Yeah, I don't have a great alternative here. Dropping the closure will cause the JS function to start throwing exceptions/panicing when called. Fortunately, this starts happening immediately, so it's easy to test/catch if mistakes are made.

examples/browser/src/lib.rs

examples/browser/index.html

MaulingMonkey · 2019-08-31T07:23:41Z

Wow, this is awesome! Thanks a lot!

My pleasure 👍

I have only played a tiny little bit with wasm so far, so there is a lot of things here that I don't have a good understanding for, but I'll try my best.

Feel free to ask questions, I'm happy to chime in.

One thing that's a bit unfortunate is that I have a branch in development (#38) which is almost finished and ready to be merged, which changes the API. So all backends need to be adapted. Do you think that you can rebase the Web MIDI support on that branch?

Sure! It's what I get for my PR first, ask questions later style :). I've subscribed to #38 so I should get a notification for when that lands as well.

It might even make the code leaner and cleaner because ports are no longer identified by the index and it seems that your DeviceSet might become simpler or superfluous then.

Looks like it, yeah, I should be able to simply lookup in the map.

Concerning the asychronicity:
The issue seems to be in how to handle js_sys::Promise, right

Yes.

I think the only way to really fix this is to introduce Rust's new async into midir's API.

That would be one of the more solid options I think, yeah.

Which is probably a good idea anyway, but out of scope for this PR. (Can js_sys::Promise interoperate with async?)

I believe so - I've mucked with Futures some, but haven't actually touched async yet, since only the former has landed in stable so far.

You can open a follow-up issue so we don't forget about it, and that also serves as documentation about this "known issue" (i.e. that messages might be lost because the asynchronous port opening has not completed yet).

Sounds good! #48

I have left some more comments inline.

Will address and rebase 👍

Boddlnagg · 2019-08-31T07:28:43Z

One more thing: We should add the wasm target to the CI configuration. I'm not sure which host platform should be used to build it, though. If you're not familiar with Azure Pipelines, that's fine and I can do it as a follow-up.

MaulingMonkey · 2019-08-31T07:44:16Z

One more thing: We should add the wasm target to the CI configuration. I'm not sure which host platform should be used to build it, though. If you're not familiar with Azure Pipelines, that's fine and I can do it as a follow-up.

I'm not sure which host should be used either. Maybe windows, under the assumption that it's the best supported by azure pipelines...? On travis I've generally used linux hosts for faster boot...

And then you've got two options:

Cheap, fast

rustup target add wasm32-unknown-unknown
cargo build --target=wasm32-unknown-unknown

Test the entire CI setup, taking a few minutes to install wasm-pack

cargo install wasm-pack
cd examples\browser
wasm-pack build --target=no-modules --dev

(EDIT: cargo install, not rustup install)

MaulingMonkey · 2019-08-31T08:34:20Z

Rebased... also squashed. Was able to delete a good chunk of code. I think I addressed everything but the azure pipelines configuration.

Boddlnagg · 2019-08-31T09:05:13Z

Okay, than this is blocked on getting #38 merged (which is blocked on chris-zen/coremidi#14) ... by the way, have you checked that your structs all implement Send, now that you wrap web_sys::MidiOutput and web_sys::MidiInput?

Because #38 contains unit tests that check for this.

MaulingMonkey · 2019-08-31T09:28:19Z

Most of them don't implement Send. AFAIK wasm threads are implemented as web workers sharing a memory buffer but not sharing JS execution contexts, nor having any sort of access to the DOM, or anything else fun.

I could get them implementing Send by storing String s instead I guess, but all API calls would still fail unless on the main/ui/tab thread, so I'm not sure if that's actually a good idea. I suppose it might be possible to workaround that with message queues and setInterval or requestAnimationFrame draining said message queue on the main thread...?

Boddlnagg · 2019-08-31T10:20:05Z

Okay, I guess it's fine to live with the fact that Send is not implemented for the Web MIDI backend (because there's good reason for it) and disable the test for that backend.

MaulingMonkey · 2019-08-31T20:10:37Z

So I have a changeset to get:

C:\local\midir>wasm-pack test --chrome

compiling (and running, not that there's anything to run).

The most awkward bit is the need to duplicate MidiInput::connect in common.rs to have a non-Send variation in order for test_forward.rs to compile, as otherwise it needs conn_out (MidiOutput) to be Send.

I can either commit that as-is to this branch (this commit if you want to see it for yourself: MaulingMonkey@6c76a9b ), or I can take a stab at doing the message queue thing to make things Send able. Any preference...?

Boddlnagg · 2019-09-01T15:17:30Z

Hm, I see ... so in Web MIDI the input callback is guaranteed to be called on the main thread, i.e. on the (only) thread that you can also send messages from?

Is there prior experience with threading on WebAssembly (in conjunction with web_sys), or is all of this not really settled yet? I feel that the story there is not clear yet and I guees that other users of web_sys might have similar problems with regards to Send and threading.

Also, these trait bounds may need to be reconsidered anyway when we tackle #48, so I would be fine to just disable the stuff that doesn't work yet (including test_forward.rs) for now.
I don't like the duplication of connect in common.rs.

MaulingMonkey · 2019-09-01T21:18:27Z

Hm, I see ... so in Web MIDI the input callback is guaranteed to be called on the main thread, i.e. on the (only) thread that you can also send messages from?

As I understand it, yes.

Is there prior experience with threading on WebAssembly (in conjunction with web_sys), or is all of this not really settled yet? I feel that the story there is not clear yet and I guees that other users of web_sys might have similar problems with regards to Send and threading.

It's still kinda early but there's at least a multithreaded ray tracing demo out there. Chrome WASM threads are still behind a chrome://flags gate as I understand it:

Also, these trait bounds may need to be reconsidered anyway when we tackle #48, so I would be fine to just disable the stuff that doesn't work yet (including test_forward.rs) for now.

Fair.

I don't like the duplication of connect in common.rs.

Me either 😄

Boddlnagg · 2019-09-14T16:20:48Z

I now merged #38 and have changed the base branch of this PR back to master. Can you please rebase your commits?

Furthermore, I tried wasm-pack test --chrome and it says no tests to run! ... what did I do wrong?

MaulingMonkey · 2019-09-14T18:18:11Z

I now merged #38 and have changed the base branch of this PR back to master. Can you please rebase your commits?

Will do.

Furthermore, I tried wasm-pack test --chrome and it says no tests to run! ... what did I do wrong?

Nothing! Currently, wasm tests also need to be marked #[wasm_bindgen_test] to run. I can mark common.rs 's test_trait_impls with it if you want, but since the whole point of the test is just to ensure it compiles, I left it alone, I didn't see much need.

It'll still log the same message for virtual.rs - where we intentionally #[cfg(...)] out the entire module due to missing virtual port support. Ideally wasm-pack wouldn't try to run tests for that file at all...

(cherry picked from commit e36019c)

(cherry picked from commit 155b3ce)

MaulingMonkey · 2019-09-15T17:17:54Z

Ahh, I should probably actually test my rebase shouldn't I...

MaulingMonkey · 2019-09-15T17:27:41Z

Rebase tested, works on my machine (tm) 👍

Boddlnagg · 2019-09-17T17:35:33Z

@MaulingMonkey Thanks again for the PR! I added CI integration in 109a884.

MaulingMonkey · 2019-09-17T18:06:24Z

Thanks for providing the solid base for me to contribute to! :)

Boddlnagg requested changes Aug 31, 2019

View reviewed changes

MaulingMonkey force-pushed the pr-web-midi branch from 5a59ec1 to ab30c2b Compare August 31, 2019 08:26

MaulingMonkey changed the base branch from master to port-ids August 31, 2019 08:27

MaulingMonkey force-pushed the pr-web-midi branch from ab30c2b to e36019c Compare August 31, 2019 08:30

Boddlnagg approved these changes Aug 31, 2019

View reviewed changes

Boddlnagg force-pushed the port-ids branch 5 times, most recently from 7b865ed to 39ed62e Compare September 14, 2019 16:04

Boddlnagg changed the base branch from port-ids to master September 14, 2019 16:06

Boddlnagg mentioned this pull request Sep 14, 2019

Figure out the story around Send/threading with WASM #49

Open

MaulingMonkey added 3 commits September 15, 2019 10:12

Web MIDI support.

514c7bb

(cherry picked from commit e36019c)

Fix wasm-pack test, disable test_forward/systex.

6cf2fce

(cherry picked from commit 155b3ce)

Reference send issues for webmidi.

af473ab

MaulingMonkey force-pushed the pr-web-midi branch from 5bfc04d to af473ab Compare September 15, 2019 17:16

Boddlnagg merged commit 8d91223 into Boddlnagg:master Sep 17, 2019

Boddlnagg mentioned this pull request Apr 8, 2021

How to run WebMIDI example? #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web MIDI support. #47

Web MIDI support. #47

MaulingMonkey commented Aug 31, 2019 •

edited

Loading

Boddlnagg left a comment

Boddlnagg Aug 31, 2019

Boddlnagg Aug 31, 2019

MaulingMonkey Aug 31, 2019

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Aug 31, 2019

MaulingMonkey commented Aug 31, 2019 •

edited

Loading

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Aug 31, 2019

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Aug 31, 2019

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Sep 1, 2019 •

edited

Loading

MaulingMonkey commented Sep 1, 2019

Boddlnagg commented Sep 14, 2019

MaulingMonkey commented Sep 14, 2019

MaulingMonkey commented Sep 15, 2019

MaulingMonkey commented Sep 15, 2019

Boddlnagg commented Sep 17, 2019

MaulingMonkey commented Sep 17, 2019

Web MIDI support. #47

Web MIDI support. #47

Conversation

MaulingMonkey commented Aug 31, 2019 • edited Loading

Concerns

Testing

.vscode\tasks.json

.vscode\launch.json

Screenshots

Boddlnagg left a comment

Choose a reason for hiding this comment

Boddlnagg Aug 31, 2019

Choose a reason for hiding this comment

Boddlnagg Aug 31, 2019

Choose a reason for hiding this comment

MaulingMonkey Aug 31, 2019

Choose a reason for hiding this comment

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Aug 31, 2019

MaulingMonkey commented Aug 31, 2019 • edited Loading

Cheap, fast

Test the entire CI setup, taking a few minutes to install wasm-pack

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Aug 31, 2019

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Aug 31, 2019

MaulingMonkey commented Aug 31, 2019

Boddlnagg commented Sep 1, 2019 • edited Loading

MaulingMonkey commented Sep 1, 2019

Boddlnagg commented Sep 14, 2019

MaulingMonkey commented Sep 14, 2019

MaulingMonkey commented Sep 15, 2019

MaulingMonkey commented Sep 15, 2019

Boddlnagg commented Sep 17, 2019

MaulingMonkey commented Sep 17, 2019

MaulingMonkey commented Aug 31, 2019 •

edited

Loading

MaulingMonkey commented Aug 31, 2019 •

edited

Loading

Boddlnagg commented Sep 1, 2019 •

edited

Loading