Improve MidiManagerAlsa manufacturer reporting

media/midi/midi_manager_alsa.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/midi/midi_manager_alsa.h"
 
 #include <alsa/asoundlib.h>
 #include <stdlib.h>
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/posix/eintr_wrapper.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "media/midi/midi_port_info.h"
 
 namespace media {
 
 namespace {
 
 // Per-output buffer. This can be smaller, but then large sysex messages
 // will be (harmlessly) split across multiple seq events. This should
 // not have any real practical effect, except perhaps to slightly reorder
 // realtime messages with respect to sysex.
 const size_t kSendBufferSize = 256;
 
 // Constants for the capabilities we search for in inputs and outputs.
 // See http://www.alsa-project.org/alsa-doc/alsa-lib/seq.html.
 const unsigned int kRequiredInputPortCaps =
     SND_SEQ_PORT_CAP_READ | SND_SEQ_PORT_CAP_SUBS_READ;
 const unsigned int kRequiredOutputPortCaps =
     SND_SEQ_PORT_CAP_WRITE | SND_SEQ_PORT_CAP_SUBS_WRITE;
 
 int AddrToInt(const snd_seq_addr_t* addr) {
   return (addr->client << 8) | addr->port;
 }
 
+#if defined(USE_UDEV)
+// Copied from components/storage_monitor/udev_util_linux.cc.

[Takashi Toyoshima, 2015/03/05 07:23:22]

I'm not an udev expert, but should we have a TODO here for factoring out common
code into device/udev_linux or somewhere?

[Adam Goode, 2015/03/05 21:46:58]

Done.

+std::string GetUdevDevicePropertyValue(udev_device* udev_device,
+                                       const char* key) {
+  const char* value = device::udev_device_get_property_value(udev_device, key);
+  return value ? value : std::string();
+}
+#endif  // defined(USE_UDEV)
+
 class CardInfo {
  public:
   CardInfo(const std::string name, const std::string manufacturer,
            const std::string driver)
       : name_(name), manufacturer_(manufacturer), driver_(driver) {
   }
   const std::string name_;
   const std::string manufacturer_;
   const std::string driver_;
 };
 
 }  // namespace
 
 MidiManagerAlsa::MidiManagerAlsa()
     : in_client_(NULL),
       out_client_(NULL),
       out_client_id_(-1),
       in_port_(-1),
       decoder_(NULL),
+#if defined(USE_UDEV)
+      udev_(device::udev_new()),
+#endif  // defined(USE_UDEV)
       send_thread_("MidiSendThread"),
       event_thread_("MidiEventThread"),
       event_thread_shutdown_(false) {
   // Initialize decoder.
   snd_midi_event_new(0, &decoder_);
   snd_midi_event_no_status(decoder_, 1);
 }
 
 void MidiManagerAlsa::StartInitialization() {
   // TODO(agoode): Move off I/O thread. See http://crbug.com/374341.
@@ skipping 46 matching lines @@
   announce_dest.port = in_port_;
   snd_seq_port_subscribe_set_sender(subs, &announce_sender);
   snd_seq_port_subscribe_set_dest(subs, &announce_dest);
   err = snd_seq_subscribe_port(in_client_, subs);
   if (err != 0) {
     VLOG(1) << "snd_seq_subscribe_port on the announce port fails: "
             << snd_strerror(err);
     return CompleteInitialization(MIDI_INITIALIZATION_ERROR);
   }
 
-  // Use a heuristic to extract the list of manufacturers for the hardware MIDI
+  // Extract the list of manufacturers for the hardware MIDI
   // devices. This won't work for all devices. It is also brittle until
   // hotplug is implemented. (See http://crbug.com/279097.)

[Takashi Toyoshima, 2015/03/05 07:23:22]

Maybe 431489 is better bug id for now, since 279097 was closed, and 43189 is
separately filed for ALSA specific implementation.

[Adam Goode, 2015/03/05 21:46:58]

Done.

-  // TODO(agoode): Make manufacturer extraction simple and reliable.
-  // http://crbug.com/377250.
   ScopedVector<CardInfo> cards;
   snd_ctl_card_info_t* card;
   snd_rawmidi_info_t* midi_out;
   snd_rawmidi_info_t* midi_in;
   snd_ctl_card_info_alloca(&card);
   snd_rawmidi_info_alloca(&midi_out);
   snd_rawmidi_info_alloca(&midi_in);
-  for (int index = -1; !snd_card_next(&index) && index >= 0; ) {
-    const std::string id = base::StringPrintf("hw:CARD=%i", index);
+  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;
     int err = snd_ctl_open(&handle, id.c_str(), 0);
     if (err != 0) {
       VLOG(1) << "snd_ctl_open fails: " << snd_strerror(err);
       continue;
     }
     err = snd_ctl_card_info(handle, card);
     if (err != 0) {
       VLOG(1) << "snd_ctl_card_info fails: " << snd_strerror(err);
       snd_ctl_close(handle);
       continue;
     }
     // Enumerate any rawmidi devices (not subdevices) and extract CardInfo.
     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;
 
+      // Get Alsa properties.
       snd_rawmidi_info_t* midi = midi_out ? midi_out : midi_in;
       const std::string name = snd_rawmidi_info_get_name(midi);
-      // We assume that card longname is in the format of
-      // "<manufacturer> <name> at <bus>". Otherwise, we give up to detect
-      // a manufacturer name here.
-      std::string manufacturer;
-      const std::string card_name = snd_ctl_card_info_get_longname(card);
-      size_t at_index = card_name.rfind(" at ");
-      if (std::string::npos != at_index) {
-        size_t name_index = card_name.rfind(name, at_index - 1);
-        if (std::string::npos != name_index)
-          manufacturer = card_name.substr(0, name_index - 1);
-      }
+      const std::string longname = snd_ctl_card_info_get_longname(card);
       const std::string driver = snd_ctl_card_info_get_driver(card);
+
+      // Get udev properties if available.
+      std::string udev_id_vendor_enc;

[Takashi Toyoshima, 2015/03/05 07:23:22]

I slightly feel it's better to replace following udev dependent code outside
this for-loop as an external function.

To keep current unit tests work as is, how about following code?

// one line code for for-loop here.
const std::string manufacturer = GetManufacturer(longname, card_index);

// and define two methods.
std::string MidiManagerAlsa::GetManufacturer(const std::string alsa_longname,
int card_index) {
#if defined(USE_UDEV)
  // udev dependent code from line 193 to 202, and line 205.
#else
  // Try a heuristic code from line 498 to 507.
#endif
}

#if defined(USE_UDEV)
// static
std::string MidiManagerAlsa::ExtractManufacturer(int card_index) {
  // line 480 to 496, and 510.
}
#endif

[Adam Goode, 2015/03/05 21:46:58]

I refactored a bit. Let me know what you think. It might still need some work.

+      std::string udev_id_vendor_id;
+      std::string udev_id_vendor_from_database;
+#if defined(USE_UDEV)
+      const std::string sysname = base::StringPrintf("card%i", card_index);
+      device::ScopedUdevDevicePtr udev_device(
+          device::udev_device_new_from_subsystem_sysname(
+              udev_.get(), "sound", sysname.c_str()));
+      udev_id_vendor_enc = GetUdevDevicePropertyValue(
+          udev_device.get(), "ID_VENDOR_ENC");
+      udev_id_vendor_id = GetUdevDevicePropertyValue(
+          udev_device.get(), "ID_VENDOR_ID");
+      udev_id_vendor_from_database = GetUdevDevicePropertyValue(
+          udev_device.get(), "ID_VENDOR_FROM_DATABASE");
+#endif  // defined(USE_UDEV)
+
+      const std::string manufacturer = ExtractManufacturer(
+      udev_id_vendor_enc, udev_id_vendor_id, udev_id_vendor_from_database,
+          name, longname);
+
       cards.push_back(new CardInfo(name, manufacturer, driver));
     }
     snd_ctl_close(handle);
   }
 
   // Enumerate all ports in all clients.
   snd_seq_client_info_t* client_info;
   snd_seq_client_info_alloca(&client_info);
   snd_seq_port_info_t* port_info;
   snd_seq_port_info_alloca(&port_info);
@@ skipping 245 matching lines @@
       }
     }
   }
 
   // Do again.
   event_thread_.message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&MidiManagerAlsa::EventLoop, base::Unretained(this)));
 }
 
+std::string MidiManagerAlsa::ExtractManufacturer(

[Takashi Toyoshima, 2015/03/05 07:23:22]

It isn't mandatory, but usually we add an one line comment "// static" just
before a static member method implementation.

[Adam Goode, 2015/03/05 21:46:58]

Done.

+    const std::string& udev_id_vendor_enc,
+    const std::string& udev_id_vendor_id,
+    const std::string& udev_id_vendor_from_database,
+    const std::string& alsa_name,
+    const std::string& alsa_longname) {
+  // Let's try to determine the manufacturer. Here is the ordered preference

[Takashi Toyoshima, 2015/03/05 07:23:22]

I prefer to use #if defined(USE_UDEV) - #endif from line 480 to 496 if you don't
like my previous proposal at line 189.

[Adam Goode, 2015/03/05 21:46:58]

Acknowledged.

+  // in extraction:
+  //  1. Vendor name from the USB device iManufacturer string, stored in
+  //     udev_id_vendor_enc.
+  //  2. Vendor name from the udev hwid database.
+  //  3. Heuristic from ALSA.
+
+  // Is the vendor string not just the USB vendor hex id?
+  std::string udev_id_vendor = UnescapeUdev(udev_id_vendor_enc);
+  if (udev_id_vendor != udev_id_vendor_id) {
+    return udev_id_vendor;
+  }
+
+  // Is there a vendor string in the hardware database?
+  if (!udev_id_vendor_from_database.empty()) {
+    return udev_id_vendor_from_database;
+  }
+
+  // Ok, udev gave us nothing useful. So try a heuristic.
+  // We assume that card longname is in the format of
+  // "<manufacturer> <name> at <bus>". Otherwise, we give up to detect
+  // a manufacturer name here.
+  size_t at_index = alsa_longname.rfind(" at ");
+  if (std::string::npos != at_index) {
+    size_t name_index = alsa_longname.rfind(alsa_name, at_index - 1);
+    if (std::string::npos != name_index)
+      return alsa_longname.substr(0, name_index - 1);
+  }
+
+  // Failure.
+  return "";
+}
+
+std::string MidiManagerAlsa::UnescapeUdev(const std::string& s) {
+  std::string unescaped;
+  const size_t size = s.size();
+  for (size_t i = 0; i < size; ++i) {
+    char c = s[i];
+    if ((i + 3 < size) && c == '\\' && s[i + 1] == 'x') {
+      c = (HexDigitToInt(s[i + 2]) << 4) +
+          HexDigitToInt(s[i + 3]);
+      i += 3;
+    }
+    unescaped.push_back(c);
+  }
+  return unescaped;
+}
+
 MidiManager* MidiManager::Create() {
   return new MidiManagerAlsa();
 }
 
 }  // namespace media