Web MIDI Linux: Fix enumeration when OPL3 devices are present, and fix manufacturer extraction bugs

Web MIDI Linux: Fix enumeration when OPL3 devices are present, and fix manufacturer extraction bugs

Committed: https://crrev.com/fcb6e502d22dd09b6f2501cc9dfb08da7bbf74f1
Cr-Commit-Position: refs/heads/master@{#321935}

[Adam Goode, 2015-03-21 07:03:32]

agoode@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2015-03-23 06:45:38]

https://codereview.chromium.org/1025863002/diff/1/media/midi/midi_manager_als...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/1025863002/diff/1/media/midi/midi_manager_als...
media/midi/midi_manager_alsa.cc:468: devices.push_back(new AlsaRawmidi(this,
name,
this indent style isn't bad, but how about using the same indent style with line
480?

https://codereview.chromium.org/1025863002/diff/1/media/midi/midi_manager_als...
media/midi/midi_manager_alsa.cc:477: if (iface == SND_HWDEP_IFACE_OPL2 || iface
== SND_HWDEP_IFACE_OPL3 ||
Hum... I'm not sure this is the right check to find a MIDI emulating device. Is
this a OPL series specific heuristics? If so, comment is needed.

I guess these flags are originally set for indicating that the device has an
interface to access internal FM sound chips directly. But I'm not sure which
component of ALSA emulates MIDI device on the top of the FM chip.

[Adam Goode, 2015-03-24 01:39:35]

https://codereview.chromium.org/1025863002/diff/1/media/midi/midi_manager_als...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/1025863002/diff/1/media/midi/midi_manager_als...
media/midi/midi_manager_alsa.cc:468: devices.push_back(new AlsaRawmidi(this,
name,
On 2015/03/23 06:45:38, Takashi Toyoshima (chromium) wrote:
> this indent style isn't bad, but how about using the same indent style with
line
> 480?

Done.

https://codereview.chromium.org/1025863002/diff/1/media/midi/midi_manager_als...
media/midi/midi_manager_alsa.cc:477: if (iface == SND_HWDEP_IFACE_OPL2 || iface
== SND_HWDEP_IFACE_OPL3 ||
On 2015/03/23 06:45:38, Takashi Toyoshima (chromium) wrote:
> Hum... I'm not sure this is the right check to find a MIDI emulating device.
Is
> this a OPL series specific heuristics? If so, comment is needed.
> 
> I guess these flags are originally set for indicating that the device has an
> interface to access internal FM sound chips directly. But I'm not sure which
> component of ALSA emulates MIDI device on the top of the FM chip.

Yes, explanation is needed. I've put a comment explaining the situation.

To summarize: any kernel driver can create an ALSA client (visible to us). With
modern hardware, only rawmidi devices do this. Kernel drivers create rawmidi
devices and the rawmidi subsystem makes seq clients. But the OPL3 driver is
special, it does not make a rawmidi device but a seq client directly. (I
verified this is the only one to worry about in the kernel code.)

OPL3 is very old (but still possible to get in new hardware, which is how I am
testing it). It is unlikely that new drivers would not use rawmidi and defeat
our heuristic.

Longer term, I need to add support in the kernel to expose a direct link from
card->client so that all these heuristics will be obsolete. Once that is there,
we can assume our old heuristics will work on old kernels and the new robust
code will be used on new. Then we will not need to worry about kernel internals
breaking our code.

If we wanted to forego manufacturer detection and robust device reconnection
(meaning that we won't be able to detect device reconnection as the same device
in as many cases), I can remove all the heuristics and be left with simple code.
Once the kernel is updated with the direct support, I can add simple code to
Chrome to add back manufacturer detection and robust device reconnection.

This would cause a bit of a disadvantage to Chrome OS users though, as the
heuristics will work there just fine, since only USB MIDI devices are possible
with current hardware.

[Takashi Toyoshima, 2015-03-24 04:40:05]

Thank you for a good documentation on OPL.
LGTM.

[Adam Goode, 2015-03-24 04:43:55]

The CQ bit was checked by agoode@chromium.org

[commit-bot: I haz the power, 2015-03-24 04:44:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025863002/20001

[commit-bot: I haz the power, 2015-03-24 05:08:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-24 05:09:05]

Patchset 2 (id:??) landed as
https://crrev.com/fcb6e502d22dd09b6f2501cc9dfb08da7bbf74f1
Cr-Commit-Position: refs/heads/master@{#321935}