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

media/midi/midi_manager_alsa.cc

Index: media/midi/midi_manager_alsa.cc
diff --git a/media/midi/midi_manager_alsa.cc b/media/midi/midi_manager_alsa.cc
index 9015be9913033bb54d4b78b2b7e2640c567c8920..2050780f5ebfb2ce000693aabf54346e57f988f0 100644
--- a/media/midi/midi_manager_alsa.cc
+++ b/media/midi/midi_manager_alsa.cc
@@ -261,8 +261,8 @@ MidiManagerAlsa::AlsaRawmidi::AlsaRawmidi(const MidiManagerAlsa* outer,
       device::UdevDeviceGetPropertyValue(udev_device.get(), kIdVendorEnc));
   // Sometimes it is not encoded.
   if (vendor.empty())
-    UdevDeviceGetPropertyOrSysattr(udev_device.get(), kIdVendor,
-                                   kSysattrVendorName);
+    vendor = UdevDeviceGetPropertyOrSysattr(udev_device.get(), kIdVendor,
+                                            kSysattrVendorName);
   // Also get the vendor string from the hardware database.
   vendor_from_database = device::UdevDeviceGetPropertyValue(
       udev_device.get(), kIdVendorFromDatabase);
@@ -344,8 +344,8 @@ std::string MidiManagerAlsa::AlsaRawmidi::ExtractManufacturerString(
   //  2. Vendor name from the udev database (property ID_VENDOR_FROM_DATABASE).
   //  3. Heuristic from ALSA.
 
-  // Is the vendor string not just the vendor hex id?
-  if (udev_id_vendor != udev_id_vendor_id) {
+  // Is the vendor string present and not just the vendor hex id?
+  if (!udev_id_vendor.empty() && (udev_id_vendor != udev_id_vendor_id)) {
     return udev_id_vendor;
   }
 
@@ -441,11 +441,9 @@ std::string MidiManagerAlsa::AlsaPortMetadata::OpaqueKey() const {
 ScopedVector<MidiManagerAlsa::AlsaRawmidi> MidiManagerAlsa::AllAlsaRawmidis() {
   ScopedVector<AlsaRawmidi> devices;
   snd_ctl_card_info_t* card;
-  snd_rawmidi_info_t* midi_out;
-  snd_rawmidi_info_t* midi_in;
+  snd_hwdep_info_t* hwdep;
   snd_ctl_card_info_alloca(&card);
-  snd_rawmidi_info_alloca(&midi_out);
-  snd_rawmidi_info_alloca(&midi_in);
+  snd_hwdep_info_alloca(&hwdep);
   for (int card_index = -1; !snd_card_next(&card_index) && card_index >= 0;) {
     const std::string id = base::StringPrintf("hw:CARD=%i", card_index);
     snd_ctl_t* handle;
@@ -460,28 +458,26 @@ ScopedVector<MidiManagerAlsa::AlsaRawmidi> MidiManagerAlsa::AllAlsaRawmidis() {
       snd_ctl_close(handle);
       continue;
     }
-    // Enumerate any rawmidi devices (not subdevices) and extract AlsaRawmidi.
+    std::string name = snd_ctl_card_info_get_name(card);
+    std::string longname = snd_ctl_card_info_get_longname(card);
+    std::string driver = snd_ctl_card_info_get_driver(card);
+
+    // Count rawmidi devices (not subdevices).
     for (int device = -1;
          !snd_ctl_rawmidi_next_device(handle, &device) && device >= 0;) {
-      bool output;
-      bool input;
-      snd_rawmidi_info_set_device(midi_out, device);
-      snd_rawmidi_info_set_subdevice(midi_out, 0);
-      snd_rawmidi_info_set_stream(midi_out, SND_RAWMIDI_STREAM_OUTPUT);
-      output = snd_ctl_rawmidi_info(handle, midi_out) == 0;
-      snd_rawmidi_info_set_device(midi_in, device);
-      snd_rawmidi_info_set_subdevice(midi_in, 0);
-      snd_rawmidi_info_set_stream(midi_in, SND_RAWMIDI_STREAM_INPUT);
-      input = snd_ctl_rawmidi_info(handle, midi_in) == 0;
-      if (!output && !input)
-        continue;
-
-      // Compute and save ALSA and udev properties.
-      snd_rawmidi_info_t* midi = midi_out ? midi_out : midi_in;
-      devices.push_back(new AlsaRawmidi(this, snd_rawmidi_info_get_name(midi),
-                                        snd_ctl_card_info_get_longname(card),
-                                        snd_ctl_card_info_get_driver(card),
-                                        card_index));
+      devices.push_back(new AlsaRawmidi(this, name,

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

this indent style isn't bad, but how about using the same indent style with line
480?

[Adam Goode, 2015/03/24 01:39:34]

Done.

+                                        longname, driver, card_index));
+    }
+
+    // Count any hwdep synths that become MIDI devices.
+    for (int device = -1;
+         !snd_ctl_hwdep_next_device(handle, &device) && device >= 0;) {
+      snd_ctl_hwdep_info(handle, hwdep);
+      snd_hwdep_iface_t iface = snd_hwdep_info_get_iface(hwdep);
+      if (iface == SND_HWDEP_IFACE_OPL2 || iface == SND_HWDEP_IFACE_OPL3 ||

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

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:34]

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.

+          iface == SND_HWDEP_IFACE_OPL4)
+        devices.push_back(
+            new AlsaRawmidi(this, name, longname, driver, card_index));
     }
     snd_ctl_close(handle);
   }