Implement Linux backend for WebMIDI
Categories
(Core :: DOM: Device Interfaces, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox97 | --- | fixed |
People
(Reporter: qdot, Assigned: gsvelto)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
Implement platform specific linux backend for WebMIDI
Comment 1•9 years ago
|
||
See this retrospective for some needed kernel changes that I am (too slowly) working on: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093233.html
Comment 2•9 years ago
|
||
Also I would love to help with this. I have really zero time to contribute code right now but hopefully can give some guidance (and maybe some code eventually).
Reporter | ||
Comment 3•8 years ago
|
||
Making this depend on bug 1135640 since we're doing midi platform specifics in Rust.
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
Paul I'm bumping into a silly issue while working on this. I tried some simple examples which request the midi access objects, pick an input port and stuff their handler in the onmidimessage
event handler. I mash keys on my MIDI keyboard and they get delivered to the event handler via the midir implementation of the MIDI platform service. So far so good. After a while however the MIDIAccess
object disappears and all the ports with it. Looking at the code flow I noticed that this is the only reference holding the MIDIAccess
object alive. When my implementation sends the port list it gets cleared. But that's odd, because I would expect the onmidimessage
handler to hold the MIDIInput
/MIDIPort
object alive and the MIDIAccess
object with it.
Am I missing something? Maybe the MIDIPort
objects should keep an explicit reference to the MIDIAccess
object they've been obtained from? I must admit I haven't touched the DOM in a while and it shows.
Note this behavior can be seen even when using the TestMIDIPlatformService.cpp
implementation, it's not specific to mine.
Comment 5•3 years ago
|
||
Can you share your test page ? We might want to add an explicit ref indeed.
Assignee | ||
Comment 6•3 years ago
|
||
I ran into this issue on this page where it seems that the input port object gets destroyed before I have a chance to do anything with it. I also managed to reproduce the issue with this patch. If I remove the SpecialPowers.exactGC()
call and just make the output.send([0x90, 0x00, 0x7F]);
invocation directly the test passes. With the GC cycles in place however the input port goes away and the test hangs.
Assignee | ||
Comment 7•3 years ago
|
||
FYI I think I've narrowed down the issue but I'm still extremely puzzled by it. If I do the following on a MIDIInput object:
input.onmidimessage = (event) => { /* do stuff */ };
The event handler is called but the object is not kept alive unless I have another reference to it. On the other hand if I do this:
input.addEventHandler("midimessage", (event) => { /* do stuff */ });
It doesn't work at all! That is my handler is never invoked and the object is not held alive. I'm trying to figure out what's wrong but it's very puzzling because MIDIPort
inherits from EventHandler
and MIDIInput
inherits from MIDIPort
so I don't see where the problem could be coming from.
Assignee | ||
Comment 8•3 years ago
|
||
The rest of the implementation is shaping up nicely but I still have to add proper error-handling and fix some issues where some messages can be duplicated.
Comment 9•3 years ago
|
||
smaug, do you know the reason why we're seeing what comment 7 describes?
This particular handler is implemented somewhat manually, but it's not obvious to me what the difference would be between the two lines above ?
Assignee | ||
Comment 10•3 years ago
|
||
I forgot something, if I do this:
input.addEventHandler("statechange", (event) => { /* do statechange stuff */ });
input.addEventHandler("midimessage", (event) => { /* do midimessage stuff */ });
The statechange handler gets called just fine, it's just the midimessage handler that doesn't. It's as if the event handlers declared in MIDIPort
work but the ones in MIDIInput
don't.
Comment 11•3 years ago
•
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #7)
FYI I think I've narrowed down the issue but I'm still extremely puzzled by it. If I do the following on a MIDIInput object:
input.onmidimessage = (event) => { /* do stuff */ };
The event handler is called but the object is not kept alive unless I have another reference to it. On the other hand if I do this:
Sounds like some issue in the midi implementation, someone isn't keeping a strong reference to the object.
input.addEventHandler("midimessage", (event) => { /* do stuff */ });
There is no such thing as addEventHandler. It is addEventListener
Assignee | ||
Comment 12•3 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #11)
There is no such thing as addEventHandler. It is addEventListener
Yeah, sorry, typo in my comments. It's addEventListener()
that simply doesn't appear to work on MIDIInput
objects.
Comment 13•3 years ago
•
|
||
some minimal testcase would be useful here.
If you can do something like
input.addEventListener("foo", function(e) { console.log(e); });
input.dispatchEvent(new Event("foo"));
then at least the event handling is fine.
And does a debug build give any interesting warnings?
Assignee | ||
Comment 14•3 years ago
|
||
This patch applies to one of the test cases and highlights the issue: with the patch applied the test never finishes because the "midimessage" event handler is never called.
If you can do something like
input.addEventListener("foo", function(e) { console.log(e); });
input.dispatchEvent(new Event("foo"));then at least the event handling is fine.
I tried and it seems like the event is not dispatched.
And does a debug build give any interesting warnings?
No, nothing at all. What's super odd is that "statechange" events work just fine on the same object. If in the test you replace:
input.onstatechange = (event) => { checkCallbacks(event.port); };
with
input.addEventListener("statechange", (event) => { checkCallbacks(event.port); });
that works!
Assignee | ||
Comment 15•2 years ago
|
||
This also updates the tests to reflect the changes in port state
Assignee | ||
Comment 16•2 years ago
|
||
Depends on D131350
Assignee | ||
Comment 17•2 years ago
|
||
Depends on D131351
Assignee | ||
Comment 18•2 years ago
|
||
Depends on D131352
Assignee | ||
Comment 19•2 years ago
|
||
Here's my WIP which mostly works but still relies on hacks (in the last two patches) and has a few limitations:
- There's no support for detecting disconnects or new devices being added. This is down to midir not supporting this functionality directly and I assume some of the underlying implementation (I'm mostly thinking about Windows 7 here). It's possible to work around this by polling the port list once the service is running but I haven't gone there yet.
- Error management is very limited, in part because it's not exposed directly in midir (e.g. there's no way to tell when an input port has been disconnected) and in part because I couldn't find a good way to make it fit.
- Code using
addEventListener("midimessage")
doesn't work, see above. - Some sites (like www.midimonitor.com) pop up the permission dialog too many times and yield duplicate events, probably a bug somewhere in my code.
Assignee | ||
Comment 20•2 years ago
|
||
Alright, I figured out what the problem was and it's been sitting under my nose the whole time. The midimessage event listener needs to be special-cased in EventListenerManager::AddEventListenerInternal like we do with other event listeners which have an implicit behavior. Specifically we need to open the input port if the port is closed when the event listener is set like we do manually here.
BTW I wonder if we could refactor this to prevent this type of issue in the future. We have other event listeners that don't use IMPL_EVENT_HANDLER()
to implement the property setter/getter because they need to do something in addition to adding the event listener; I wonder if there are some subtle bugs lurking in there.
Comment 21•2 years ago
|
||
Event handler and listener are different. Only adding handler seems to have this special case per spec that it opens midi port.
Adding a listener shouldn't do that.
(MessagePorts have similar odd behavior per spec)
Assignee | ||
Comment 22•2 years ago
|
||
Oh my. That means Chrome doesn't implement the spec correctly because it does it in both cases. Various web pages which use the API rely on it. I feel very silly. Or maybe I missed something else?
Assignee | ||
Comment 23•2 years ago
|
||
Quick update on what I'm doing:
- I'm removing the hack to convert midir timestamps to TimeStamps directly. Instead I will be using the duration between two midir timestamps plus a TimeStamp taken when the port is opened to generate DOM timestamps for every event. This way the timestamps will be always coherent both among each other and WRT other instances of TimeStamp and the duration between each event will preserve the value and precision of the original samples (which appear to start at 0 and count the number of microseconds elapsed since the port was opened)
- I'm carefully checking how Chrome deals with event handlers, listeners and the state of ports. Once I'm done with that I'll import their test suite and adapt our implementation to pass them. This is obviously not optimal but it will guarantee that we're compatible with existing code
Updated•2 years ago
|
Assignee | ||
Comment 24•2 years ago
|
||
I've updated the patch to properly construct TimeStamps without resorting to ugly hacks but I've got bad news on the implementation front: Chrome does open ports implicitly when using MIDIInput.addEventListener("midimessage", ...)
rather than just when setting the event handler. This is doubly problematic: first of all because it doesn't match the spec (which clearly states that ports should be opened implicitly only when setting an event handler and not an event listener and secondly because it's not easy to implement. I have to ponder a bit about how to handle this.
Comment 25•2 years ago
|
||
You can override EventTarget::EventListenerAdded.
Assignee | ||
Comment 26•2 years ago
|
||
Thanks!
Comment 27•2 years ago
|
||
I filed https://github.com/WebAudio/web-midi-api/issues/225 for this. Both need to implicitly open the port for web compat.
Assignee | ||
Comment 28•2 years ago
|
||
FYI I'm looking at how the MIDIAccess
/MIDIPort
objects are being dealt with by the cycle collector. It appears that the DOM event handlers for midimessage
events do not keep the MIDIPort
objects alive but IIUC they should. If I have no direct reference to a port in a piece of JS code and the object is only referenced (in theory) by the event handler then the only actual reference is the one from the MIDIAccess
object. Once the MIDIAccess
object goes away so do the ports, irrespective of the handlers. Given this behavior I suspect there's something amiss with the NS_IMPL_CYCLE_COLLECTING_*
macros.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 29•2 years ago
|
||
It seems that the macros used for cycle collection are in order so I really can't figure out why my objects are being collected. Olli, can you give me a hand with the issue that can be reproduced with attachment 9248856 [details]? What I'm seeing is the following:
- A
MIDIAcess
object is created, it holds references to a bunch ofMIDIInput
andMIDIOutput
ports - I attach an event handler to a
MIDIInput
port - I drop all other references to the
MIDIAccess
andMIDIInput
port objects save for the handler - The
MIDIInput
object my handler is registered on gets collected
Is this expected behavior? Shouldn't the handler be sufficient to keep the MIDIInput
object alive?
The reason I'm asking is that there's plenty of pages out there doing just that and the MIDIInput
objects don't appear to be collected by Chrome. That being said Chrome seems to do things a little differently: the MIDIInput
object in their implementation has a strong reference to the MIDIAccess
one, while it doesn't in ours. I don't think that should make a difference in this case because that'd create a cycle between the two and I'm not sure if that'd help given that its external references being dropped that ultimately lead to this issue (if it is an issue at all).
Comment 30•2 years ago
|
||
Event handlers or listeners aren't supposed to keep the target alive by default.
You may want to use https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/dom/events/DOMEventTargetHelper.cpp#247-254 to explicitly keep the object alive.
Seems like the spec doesn't define exact GC behavior. There are notes, but nothing like
https://html.spec.whatwg.org/#garbage-collection-2 as an example.
Also, the spec talks about EventHandlers keeping the object alive, but what about event listeners (addEventListener)?
Assignee | ||
Comment 31•2 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #30)
Event handlers or listeners aren't supposed to keep the target alive by default.
Thanks, this is the piece of information I was missing.
You may want to use https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/dom/events/DOMEventTargetHelper.cpp#247-254 to explicitly keep the object alive.
That does the trick, now I can reproduce what Chrome does at least WRT midimessage
events. I'll check what Chrome does with the other events too to be sure we match what they do. While using KeepAliveIfHasListenersFor()
I noticed something which I'm not sure if it's a design choice or an accidental footgun. DOMEventTargetHelper()
overrides EventListenerAdded()
and EventListenerRemoved()
in order to call MaybeUpdateKeepAlive()
. If a class (in this case MIDIInput
) overrides EventListenerAdded()
/EventListenerRemoved()
then KeepAliveIfHasListenersFor()
won't work unless the overridden methods deliberately call DOMEventTargetHelper::EventListenerAdded()
and DOMEventTargetHelper::EventListenerRemoved()
. Is that intentional?
Seems like the spec doesn't define exact GC behavior. There are notes, but nothing like
https://html.spec.whatwg.org/#garbage-collection-2 as an example.
Also, the spec talks about EventHandlers keeping the object alive, but what about event listeners (addEventListener)?
I don't know, I'll add that to the list of tests I must run on Chrome to figure out what it's doing.
Comment 32•2 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #31)
Is that intentional?
Yes. But indeed that should be documented in the .h
When one uses KeepAliveIfHasListenersFor, need to be careful to not cause leaks.
Something needs to call Release eventually
https://searchfox.org/mozilla-central/rev/eea0fac2cb8cae1e5a2e38959eeaa0b7f6d23456/dom/events/DOMEventTargetHelper.cpp#299,306
Assignee | ||
Comment 33•2 years ago
|
||
Thanks! I filed bug 1745003 to add a comment about that.
Comment 34•2 years ago
|
||
Comment on attachment 9251124 [details]
WIP: Bug 1201598 - HACK: Do not clear the access holder to keep the objects alive
Revision D131352 was moved to bug 1745443. Setting attachment 9251124 [details] to obsolete.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 35•2 years ago
|
||
With the DOM object liveness under review I just need to fix the ALSA configure test and we should be good here.
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 36•2 years ago
|
||
Leave open because I'll start landing some patches here but not all of them.
Comment 37•2 years ago
|
||
Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3080df6e8f62 Adjust how port state is handled when adding and opening MIDI ports r=padenot
Comment 38•2 years ago
|
||
bugherder |
Comment 39•2 years ago
|
||
Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5e98c30bbf2 Add a midir-based implementation for WebMIDI r=padenot
Comment 40•2 years ago
|
||
bugherder |
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Description
•