1201598 - Implement Linux backend for WebMIDI

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, task, P3)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Implement platform specific linux backend for WebMIDI

Comment 1

[Adam Goode]

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

[Adam Goode]

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).

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Making this depend on bug 1135640 since we're doing midi platform specifics in Rust.

Comment 4

[Gabriele Svelto [:gsvelto]]

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

[Paul Adenot (:padenot)]

Can you share your test page ? We might want to add an explicit ref indeed.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: GC'd MIDI ports test

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.

Comment 7

[Gabriele Svelto [:gsvelto]]

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.

Comment 8

[Gabriele Svelto [:gsvelto]]

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

[Paul Adenot (:padenot)]

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 ?

Comment 10

[Gabriele Svelto [:gsvelto]]

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

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 12

[Gabriele Svelto [:gsvelto]]

(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

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: midimessage event listener test failure

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!

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1201598 - Adjust how port state is handled when adding and opening MIDI ports r=padenot

This also updates the tests to reflect the changes in port state

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1201598 - Add a midir-based implementation for WebMIDI r=padenot

Depends on D131350

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: WIP: Bug 1201598 - HACK: Do not clear the access holder to keep the objects alive

Depends on D131351

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: WIP: Bug 1201598 - HACK: Allow creation of TimeStamp objects from system-provided timestamps on Windows

Depends on D131352

Comment 19

[Gabriele Svelto [:gsvelto]]

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.

Comment 20

[Gabriele Svelto [:gsvelto]]

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

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 22

[Gabriele Svelto [:gsvelto]]

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?

Comment 23

[Gabriele Svelto [:gsvelto]]

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

Comment 24

[Gabriele Svelto [:gsvelto]]

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

[Olli Pettay [:smaug][[e-mail]]]

You can override EventTarget::EventListenerAdded.

Comment 26

[Gabriele Svelto [:gsvelto]]

Thanks!

Comment 27

[Paul Adenot (:padenot)]

I filed https://github.com/WebAudio/web-midi-api/issues/225 for this. Both need to implicitly open the port for web compat.

Comment 28

[Gabriele Svelto [:gsvelto]]

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.

Comment 29

[Gabriele Svelto [:gsvelto]]

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 of MIDIInput and MIDIOutput ports
• I attach an event handler to a MIDIInput port
• I drop all other references to the MIDIAccess and MIDIInput 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

[Olli Pettay [:smaug][[e-mail]]]

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)?

Comment 31

[Gabriele Svelto [:gsvelto]]

(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

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 33

[Gabriele Svelto [:gsvelto]]

Thanks! I filed bug 1745003 to add a comment about that.

Comment 34

[Phabricator Automation]

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.

Comment 35

[Gabriele Svelto [:gsvelto]]

With the DOM object liveness under review I just need to fix the ALSA configure test and we should be good here.

Comment 36

[Gabriele Svelto [:gsvelto]]

Leave open because I'll start landing some patches here but not all of them.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3080df6e8f62
Adjust how port state is handled when adding and opening MIDI ports r=padenot

Comment 38

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/3080df6e8f62

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5e98c30bbf2
Add a midir-based implementation for WebMIDI r=padenot

Comment 40

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/c5e98c30bbf2