Introduce USB MIDI descriptor parser

media/midi/usb_midi_descriptor_parser.cc

+// Copyright (c) 2013 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/usb_midi_descriptor_parser.h"
+
+#include "base/logging.h"
+
+namespace media {
+
+namespace {
+
+// The constants below are specified in USB spec, USB audio spec
+// and USB midi spec.
+
+enum DescriptorType {
+  TYPE_DEVICE = 1,
+  TYPE_CONFIGURATION = 2,
+  TYPE_STRING = 3,
+  TYPE_INTERFACE = 4,
+  TYPE_ENDPOINT = 5,
+  TYPE_DEVICE_QUALIFIER = 6,
+  TYPE_OTHER_SPEED_CONFIGURATION = 7,
+  TYPE_INTERFACE_POWER = 8,
+
+  TYPE_CS_INTERFACE = 36,
+  TYPE_CS_ENDPOINT = 37,
+};
+
+enum DescriptorSubType {
+  SUBTYPE_MS_DESCRIPTOR_UNDEFINED = 0,
+  SUBTYPE_MS_HEADER = 1,
+  SUBTYPE_MIDI_IN_JACK = 2,
+  SUBTYPE_MIDI_OUT_JACK = 3,
+  SUBTYPE_ELEMENT = 4,
+};
+
+enum JackType {
+  JACK_TYPE_UNDEFINED = 0,
+  JACK_TYPE_EMBEDDED = 1,
+  JACK_TYPE_EXTERNAL = 2,
+};
+
+const uint8 kAudioInterfaceClass = 1;
+const uint8 kAudioMidiInterfaceSubclass = 3;
+
+}  // namespace
+
+UsbMidiDescriptorParser::UsbMidiDescriptorParser()
+    : is_parsing_usb_midi_interface_(false),
+      current_endpoint_address_(0),
+      current_cable_number_(0) {}
+
+UsbMidiDescriptorParser::~UsbMidiDescriptorParser() {}
+
+bool UsbMidiDescriptorParser::Parse(UsbMidiDevice* device,
+                                    const uint8* data,
+                                    size_t size) {
+  Clear();

[DaleCurtis, 2014/01/03 00:56:49]

You seem to be always clearing the internal state after and before very call. 
Why not instead return the jacks list from this method and use local variables
inside ParseInternal?

I'm a little confused on what the data stream will look like.  A description of
what it looks like in the header of this file would be very helpful.

Will the order of interfaces vary between Parse() calls?  Are there multiple
instances of each interface in the stream?  This code spends some effort to
ensure it can handle any order of interfaces gracefully.  I.e., Instead, if an
interface is always expected to follow another it'd make sense to bundle those
parse calls together.

[yhirano, 2014/01/09 08:33:13]

Removing jacks_ is done.
This is not a generic parser and I think this style is reasonable to ignore
uninteresting descriptors without writing much code. For example, it is
difficult to find the end of a non-MIDI INTERFACE without writing more detailed
code, but layered parser requires it.

Added some comments in the header and ParseInternal.

+  bool result = ParseInternal(device, data, size);
+  if (result)
+    ClearExceptJacks();
+  else
+    Clear();
+  return result;
+}
+
+bool UsbMidiDescriptorParser::ParseInternal(UsbMidiDevice* device,
+                                            const uint8* data,
+                                            size_t size) {
+  DCHECK(jacks_.empty());
+  const uint8* current = data;
+  while (current < &data[size]) {

[DaleCurtis, 2014/01/03 00:56:49]

The notation here is slightly confusing.  It'd be clearer to use "current < data
+ size".

[yhirano, 2014/01/09 08:33:13]

Done.

+    uint8 length = current[0];
+    if (length <= 1) {
+      DVLOG(1) << "Descriptor Type is not accessible.";
+      return false;
+    }
+    if (&current[length] > &data[size]) {

[DaleCurtis, 2014/01/03 00:56:49]

Ditto.

[yhirano, 2014/01/09 08:33:13]

Done.

+      DVLOG(1) << "The header size is incorrect.";
+      return false;
+    }
+    DescriptorType descriptor_type = static_cast<DescriptorType>(current[1]);
+    switch (descriptor_type) {
+      case TYPE_INTERFACE:
+        if (!ParseInterface(device, current, length))
+          return false;
+        break;
+      case TYPE_CS_INTERFACE:
+        if (!ParseCSInterface(device, current, length))
+          return false;
+        break;
+      case TYPE_ENDPOINT:
+        if (!ParseEndpoint(device, current, length))
+          return false;
+        break;
+      case TYPE_CS_ENDPOINT:
+        if (!ParseCSEndpoint(device, current, length))
+          return false;
+        break;
+      default:
+        // Ignore unintersting types.

[DaleCurtis, 2014/01/03 00:56:49]

uninteresting

[yhirano, 2014/01/09 08:33:13]

Done.

+        break;
+    }
+    current += length;
+  }
+  return true;
+}
+
+bool UsbMidiDescriptorParser::ParseInterface(UsbMidiDevice* device,
+                                             const uint8* data,
+                                             size_t size) {
+  if (size != 9) {
+    DVLOG(1) << "INTERFACE header size is incorrect.";
+    return false;
+  }
+  incomplete_jacks_.clear();
+
+  uint8 interface_class = data[5];
+  uint8 interface_subclass = data[6];
+
+  // All descriptors of endpoints contained in this interface
+  // precede the next INTERFACE descriptor.
+  is_parsing_usb_midi_interface_ =
+      (interface_class == kAudioInterfaceClass &&

[DaleCurtis, 2014/01/03 00:56:49]

No need for parens.

[yhirano, 2014/01/09 08:33:13]

Done.

+       interface_subclass == kAudioMidiInterfaceSubclass);
+  return true;
+}
+
+bool UsbMidiDescriptorParser::ParseCSInterface(UsbMidiDevice* device,
+                                               const uint8* data,
+                                               size_t size) {
+  if (!is_parsing_usb_midi_interface_)
+    return true;

[DaleCurtis, 2014/01/03 00:56:49]

Should this be false?  Here and all other methods.

[yhirano, 2014/01/09 08:33:13]

This should be true, we want to ignore uninteresting descriptors.

+
+  // Descriptor Type and Descriptor Subtype should be accessible.
+  if (size <= 2) {

[DaleCurtis, 2014/01/03 00:56:49]

< 3 for consistency.

[yhirano, 2014/01/09 08:33:13]

Done.

+    DVLOG(1) << "CS_INTERFACE header size is incorrect.";
+    return false;
+  }
+
+  DescriptorSubType subtype = static_cast<DescriptorSubType>(data[2]);
+
+  switch (subtype) {
+    case SUBTYPE_MIDI_IN_JACK:
+      // FALL_THROUGH

[DaleCurtis, 2014/01/03 00:56:49]

Label is unnecessary since you're not doing anything different for this case.

[yhirano, 2014/01/09 08:33:13]

Done.

+    case SUBTYPE_MIDI_OUT_JACK: {
+      if (size < 6) {
+        DVLOG(1) << "CS_INTERFACE (MIDI JACK) header size is incorrect.";
+        return false;
+      }
+      uint8 jack_type = data[3];
+      uint8 id = data[4];
+      if (jack_type == JACK_TYPE_EMBEDDED) {
+        // We can't determine the associated endpoint now.
+        incomplete_jacks_.insert(

[DaleCurtis, 2014/01/03 00:56:49]

incomplete_jacks_[id] = ... ? no need for make_pair then.  How many entries do
you expect to be in this structure on a regular basis?  If it's only a few, you
should probably just use a vector instead.

[yhirano, 2014/01/09 08:33:13]

Done.

+            std::make_pair(id, UsbMidiJack(device, id, 0, 0)));
+      }
+      break;
+    }
+    default:
+      break;
+  }
+  return true;
+}
+
+bool UsbMidiDescriptorParser::ParseEndpoint(UsbMidiDevice* device,
+                                            const uint8* data,
+                                            size_t size) {
+  if (!is_parsing_usb_midi_interface_)
+    return true;
+
+  if (size < 4) {

[DaleCurtis, 2014/01/03 00:56:49]

Ditto.

[yhirano, 2014/01/09 08:33:13]

Sorry, what ditto?

+    DVLOG(1) << "ENDPOINT header size is incorrect.";
+    return false;
+  }
+  current_endpoint_address_ = data[2];
+  current_cable_number_ = 0;
+  return true;
+}
+
+bool UsbMidiDescriptorParser::ParseCSEndpoint(UsbMidiDevice* device,
+                                              const uint8* data,
+                                              size_t size) {
+  if (!is_parsing_usb_midi_interface_)
+    return true;
+  // CS_ENDPOINT must be of size 4 + n where n is the number of associated
+  // jacks.
+  if (size < 4) {
+    DVLOG(1) << "CS_ENDPOINT header size is incorrect.";
+    return false;
+  }
+  uint8 num_jacks = data[3];
+  if (size != static_cast<size_t>(4 + num_jacks)) {

[DaleCurtis, 2014/01/03 00:56:49]

4 is used multiple times, so it should be extracted to a local constant.

[yhirano, 2014/01/09 08:33:13]

Done.

+    DVLOG(1) << "CS_ENDPOINT header size is incorrect.";
+    return false;
+  }
+
+  for (size_t i = 0; i < num_jacks; ++i) {
+    uint8 jack = data[4 + i];
+    base::hash_map<uint8, UsbMidiJack>::iterator it =
+        incomplete_jacks_.find(jack);
+    if (it == incomplete_jacks_.end()) {
+      DVLOG(1) << "A non-existing MIDI jack is associated.";
+      return false;
+    }
+    if (current_cable_number_ >= 0x10) {

[DaleCurtis, 2014/01/03 00:56:49]

Using > 0xF is clearer and more consistent with the comment.

[yhirano, 2014/01/09 08:33:13]

Done.

+      DVLOG(1) << "Cable number should range from 0x0 to 0xf.";
+      return false;
+    }
+    // CS_ENDPOINT follows ENDPOINT and hence we can use the following
+    // member variables.
+    it->second.cable_number = current_cable_number_++;
+    it->second.endpoint_address = current_endpoint_address_;
+    jacks_.push_back(it->second);
+    incomplete_jacks_.erase(it);
+  }
+  return true;
+}
+
+void UsbMidiDescriptorParser::Clear() {
+  ClearExceptJacks();
+  jacks_.clear();
+}
+
+void UsbMidiDescriptorParser::ClearExceptJacks() {
+  is_parsing_usb_midi_interface_ = false;
+  current_endpoint_address_ = 0;
+  current_cable_number_ = 0;
+  incomplete_jacks_.clear();
+}
+
+}  // namespace media