Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation

media/midi/midi_message_util.h

Index: media/midi/midi_message_util.h
diff --git a/media/midi/midi_message_util.h b/media/midi/midi_message_util.h
new file mode 100644
index 0000000000000000000000000000000000000000..0eb5a09cfc0bc21b5ab3ae8c2f949910f1816821
--- /dev/null
+++ b/media/midi/midi_message_util.h
@@ -0,0 +1,79 @@
+// 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.
+
+#ifndef MEDIA_MIDI_MIDI_MESSAGE_UTIL_H_
+#define MEDIA_MIDI_MIDI_MESSAGE_UTIL_H_
+
+#include <deque>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+// A simple message splitter for possibly unsafe MIDI data stream. This class
+// allows you to

[scherkus (not reviewing), 2013/11/20 20:16:00]

s/to/to:/

[yukawa, 2013/11/21 01:19:40]

Done.

+// - maintain fragmented MIDI message.
+// - skip any invalid data sequence.
+// - reorder MIDI messages so that "System Real Time Message", which can be
+//   inserted at any point of the byte stream, is placed at the boundary of
+//   complete MIDI messages.
+// - (Optional) reconstruct complete MIDI messages from data stream that is
+//   compressed with "running status".
+//
+// Example (pseudo message loop):
+//   MIDIMessageQueue queue(true);  // true to support "running status"
+//   while (true) {
+//     if (is_incomming_midi_data_available()) {

[scherkus (not reviewing), 2013/11/20 20:16:00]

s/incomming/incoming/

here + rest of example code

[yukawa, 2013/11/21 01:19:40]

Done.

+//       std::vector<uint8> incomming_data;
+//       read_incomming_midi_data(&incomming_data)
+//       queue.Add(incomming_data);
+//     }
+//     while (true) {
+//       std::vector<uint8> next_message;
+//       queue.Get(&next_message);
+//       if (!next_message.empty())
+//         dispatch(next_message);
+//     }
+//   }
+class MEDIA_EXPORT MIDIMessageQueue {
+ public:
+  // Initializes the queue. Set true to |allow_running_status| to enable
+  // "MIDI running status" reconstruction.
+  explicit MIDIMessageQueue(bool allow_running_status);

[scherkus (not reviewing), 2013/11/20 20:16:00]

it looks like test code is the only code that passes in |false|

do we really need to keep allow_running_status=false code path around? can we
add it in a future CL when production code is using it?

[yukawa, 2013/11/21 01:19:40]

Yes. A subsequent CL depends on this functionality.
https://codereview.chromium.org/29793006/diff/650001/media/midi/midi_manager_...

+  ~MIDIMessageQueue();
+
+  // Enqueues |data| to the internal buffer.
+  void Add(const std::vector<uint8>& data);
+  // Enqueues |data| to the internal buffer. |data| must be valid until
+  // this method returns.
+  void Add(const uint8* data, size_t length);
+
+  // Fills the next complete MIDI message into |message|. If |message| is
+  // not empty, the data sequence falls into one of the following types of
+  // MIDI message.
+  // - Single "Channel Voice Message"    (w/o "System Real Time Messages")
+  // - Single "Channel Mode Message"     (w/o "System Real Time Messages")
+  // - Single "System Exclusive Message" (w/o "System Real Time Messages")
+  // - Single "System Common Message"    (w/o "System Real Time Messages")
+  // - Single "System Real Time message"
+  // |message| is empty if there is no complete MIDI message any more.
+  void Get(std::vector<uint8>* message);
+
+ private:
+  std::deque<uint8> queue_;
+  std::vector<uint8> next_message_;
+  const bool allow_running_status_;
+  DISALLOW_COPY_AND_ASSIGN(MIDIMessageQueue);
+};
+
+// Returns true if |data| is valid for MIDIOutput.send API. Note that:
+// - Fragmentation is not allowed except for real time message.
+// - Running status is not allowed.
+MEDIA_EXPORT bool IsValidWebMIDIMessage(const std::vector<uint8>& data);

[scherkus (not reviewing), 2013/11/20 20:16:00]

if this returns false, am I still allowed to call MIDIMessageQueue::Add()?

I also don't know what the comment about running status refers to here since
that seems to be a concept of MIDIMessageQueue and not this function

in short ... I'm confused as to how this should be used. It's also something
that's named after a web api and hints that it should be inside
content/browser/renderer_host/media/midi_host.cc rather than in midi

(I'd also like for this file to be renamed to midi_message_queue.h)

[yukawa, 2013/11/21 01:19:40]

MIDIMessageQueue is originally designed to format data that is retrieved from
native MIDI API rather than WebMIDI API.  Then I realized that we also need
something to validate data sent from WebMIDI API but MIDIMessageQueue is not so
efficient and useful to such data validation. This is why I added
IsValidWebMIDIMessage.

Regarding 'running status', it is a kind of abbreviation technique that is
widely recognized in MIDI world.  I revised the comment to be a bit more clear.

Regarding file name, I'm hesitating to put IsValidWebMIDIMessage in a file named
'midi_message_queue.h' because MIDIMessageQueue and IsValidWebMIDIMessage are
used for different purposes in practice.  Another option is extracting
MIDIMessageQueue into 'midi_message_queue.h' with keeping IsValidWebMIDIMessage
in 'midi_message_util.h'.  I also realized we need to expose
GetMIDIMessageLength for a subsequent CL
https://codereview.chromium.org/29793006/ so having 'midi_message_queue.h' as
well as 'midi_message_util.h' might be an option IMO.  What do you think?

[scherkus (not reviewing), 2013/11/25 23:14:36]

I would advocate for the following:
  1) Have MIDIMessageQueue placed in midi_message_queue.{cc,h}
  2) Introduce midi_util.{cc,h} for GetMIDIMessageLength()
  3) Move IsValidWebMIDIMessage() to
content/browser/renderer_host/media/midi_host.{cc,h} (why? well AFAIK WebMIDI is
a web platform and src/content concept and shouldn't "leak" into a lower level
library such as src/media, especially if no other code in src/media needs to
call it)

WDYT?

+
+}  // namespace media
+
+#endif  // MEDIA_MIDI_MIDI_MESSAGE_UTIL_H_