Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation

media/midi/midi_message_queue.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/midi_message_queue.h"
+
+#include <algorithm>
+
+#include "base/logging.h"
+
+namespace media {
+namespace {
+
+const uint8 kSysEx = 0xf0;
+const uint8 kEndOfSysEx = 0xf7;
+
+bool IsDataByte(uint8 data) {
+  return data < 128;

[Takashi Toyoshima, 2013/11/19 01:45:31]

I'm slightly prefer (data & 0x80) == 0 or something which reminds me bit
operations.

[yukawa, 2013/11/19 09:41:41]

Done.

+}
+
+bool IsFirstStatusByte(uint8 data) {
+  return !IsDataByte(data) && data != kEndOfSysEx;
+}
+
+bool IsSystemRealTimeMessage(uint8 data) {
+  return 0xf8 <= data && data <= 0xff;
+}
+
+size_t GetMessageLength(uint8 data) {

[Takashi Toyoshima, 2013/11/19 01:45:31]

We may want to report some UMA for undefined messages, e.g., 0xf4, 0xf5, 0xf9,
0xfd?

[yukawa, 2013/11/19 09:41:41]

Hmm, I cannot imagine how we can utilize such UMA.

[Takashi Toyoshima, 2013/11/19 12:49:59]

We can catch up unknown usage, or spec update for undefined messages.
But, MIDI is mature enough. As you said, it may not worth.

+  if (IsDataByte(data))
+    return 0;
+  else if (0x80 <= data && data <= 0xbf)
+    return 3;
+  else if (0xc0 <= data && data <= 0xdf)
+    return 2;
+  else if (0xe0 <= data && data <= 0xf0)

[Takashi Toyoshima, 2013/11/19 01:45:31]

latter condition should be data <= 0xef ?

[yukawa, 2013/11/19 09:41:41]

Nice catch. Fixed.

+    return 3;
+  else if (data == 0xf0)
+    return 0;
+  else if (data == 0xf1)
+    return 2;
+  else if (data == 0xf2)
+    return 3;
+  else if (data == 0xf3)
+    return 2;
+  else if (0xf4 <= data && data <= 0xf6)
+    return 1;
+  else if (data == 0xf7)
+    return 0;
+  else  // 0xf8 <= data && data <= 0xff
+    return 1;
+}
+
+}  // namespace
+
+MIDIMessageQueue::MIDIMessageQueue(bool allow_running_status)
+    : allow_running_status_(allow_running_status) {}
+
+MIDIMessageQueue::~MIDIMessageQueue() {}
+
+void MIDIMessageQueue::Add(const std::vector<uint8>& data) {
+  queue_.insert(queue_.end(), data.begin(), data.end());
+}
+
+void MIDIMessageQueue::Add(const uint8* data, size_t length) {
+  queue_.insert(queue_.end(), data, data + length);
+}
+
+void MIDIMessageQueue::Get(std::vector<uint8>* message) {
+  message->clear();
+
+  while (true) {
+    if (queue_.empty())
+      return;
+
+    const uint8 next = queue_.front();
+    queue_.pop_front();
+
+    // "System Real Time Messages" is a special kind of MIDI messages, which can
+    // appear at arbitrary byte position of MIDI stream. Here we reorder
+    // "System Real Time Messages" prior to |next_message_| so that each message
+    // can be clearly separated as a complete MIDI message.
+    if (IsSystemRealTimeMessage(next)) {
+      message->push_back(next);
+      return;
+    }
+
+    if (!next_message_.empty() &&
+        ((next_message_[0] == kSysEx && IsFirstStatusByte(next)) ||
+         (next_message_[0] != kSysEx && !IsDataByte(next)))) {
+      // Status byte appears. Clear the existing incomplete data.

[Takashi Toyoshima, 2013/11/19 12:49:59]

Ah, I see. This comment "Status byte appears" is a little vague. How about
explicitly saying "an invalid data sequence is found".

+      next_message_.clear();
+    }
+
+    if (next_message_.empty()) {
+      if (IsFirstStatusByte(next)) {
+        next_message_.push_back(next);
+      } else {
+        // Invalid data. Ignore |next|.

[Takashi Toyoshima, 2013/11/19 01:45:31]

Can you add some comment here, like "MIDI protocol doesn't provide any error
correction mechanism in physical layers, and incoming messages can be corrupted,
and should be corrected here."

[yukawa, 2013/11/19 09:41:41]

Done.

+      }
+      continue;
+    }
+
+    next_message_.push_back(next);

[Takashi Toyoshima, 2013/11/19 01:45:31]

What happens if the status byte was set by allow_running_status handling at line
126.

[yukawa, 2013/11/19 09:41:41]

Yeah, this is a trick part. In such case, |next_message_| is not empty at line
88. And |next_message_| should already be cleared if the condition at line 90 is
true.
Does this make sense? 

[Takashi Toyoshima, 2013/11/19 12:49:59]

I see. But this code has a state, and a little hard to understand. Some comments
will be helpful.

+
+    DCHECK(!next_message_.empty());

[Takashi Toyoshima, 2013/11/19 01:45:31]

this DCHECK looks too trivial.... but it's ok to keep having :)

[yukawa, 2013/11/19 09:41:41]

Oops. Agreed.

+    const uint8 status_byte = next_message_[0];
+    if (status_byte == kSysEx) {
+      if (next == kEndOfSysEx) {
+        std::swap(*message, next_message_);
+        next_message_.clear();
+        return;
+      }
+      continue;
+    }
+
+    DCHECK(IsDataByte(next));

[Takashi Toyoshima, 2013/11/19 01:45:31]

This should not be DCHECK because as a commented at line 99, broken data can be
received. We should just dispose corrupted messages and restart to parse from
the next message.

[yukawa, 2013/11/19 09:41:41]

See the trick condition at line 88-90.

+    DCHECK_NE(kSysEx, status_byte);
+    const size_t target_len = GetMessageLength(status_byte);
+    if (next_message_.size() < target_len)
+      continue;
+    if (next_message_.size() == target_len) {
+      std::swap(*message, next_message_);
+      next_message_.clear();
+      if (allow_running_status_)
+        next_message_.push_back(status_byte);

[Takashi Toyoshima, 2013/11/19 01:45:31]

I think this code doesn't work.

[yukawa, 2013/11/19 09:41:41]

See the trick condition at line 88-90.

+      return;
+    }
+
+    NOTREACHED();
+  }
+}
+
+}  // namespace media