Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(342)

Issue 93583002: Revert 237558 "Use MIDIMessageQueue/IsValidWebMIDIData for MIDI ..." (Closed)

Created:
7 years ago by Ilya Sherman
Modified:
7 years ago
Reviewers:
yukawa
CC:
chromium-reviews
Visibility:
Public.

Description

Revert 237558 "Use MIDIMessageQueue/IsValidWebMIDIData for MIDI ..." Seems to have caused issues running perf tests. BUG=324160 > Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation > > WebMIDI spec draft: http://www.w3.org/TR/webmidi/ > > WebMIDI API guarantees that MIDIInput::onmessage is called back with a single MIDI message. To guarantee this, this CL introduces MIDIMessageQueue class, which allows you to > - 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, can be placed at the boundary of complete MIDI messages. > - (Optional) Reconstruct complete MIDI messages from data stream that is compressed with "running status". > > This CL also replaces existing System Exclusive message validation logic in MIDIHost::OnSendData with MIDIHost::IsValidWebMIDIData, which can detect SysEx message even when it is concatenated with non-SysEx messages. > > With this change, renderer/blink can be much simpler and free from this kind of data validation. > > BUG=303599, 317355 > TEST=media_unittests --gtest_filter=MIDI*, content_unittests --gtest_filter=MIDI* > > Review URL: https://codereview.chromium.org/68353002 TBR=yukawa@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237660

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -672 lines) Patch
M trunk/src/content/browser/renderer_host/media/midi_host.h View 5 chunks +5 lines, -22 lines 0 comments Download
M trunk/src/content/browser/renderer_host/media/midi_host.cc View 5 chunks +39 lines, -96 lines 0 comments Download
D trunk/src/content/browser/renderer_host/media/midi_host_unittest.cc View 1 chunk +0 lines, -90 lines 0 comments Download
M trunk/src/content/content_tests.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/src/media/media.gyp View 2 chunks +0 lines, -6 lines 0 comments Download
D trunk/src/media/midi/midi_message_queue.h View 1 chunk +0 lines, -72 lines 0 comments Download
D trunk/src/media/midi/midi_message_queue.cc View 1 chunk +0 lines, -119 lines 0 comments Download
D trunk/src/media/midi/midi_message_queue_unittest.cc View 1 chunk +0 lines, -173 lines 0 comments Download
D trunk/src/media/midi/midi_message_util.h View 1 chunk +0 lines, -25 lines 0 comments Download
D trunk/src/media/midi/midi_message_util.cc View 1 chunk +0 lines, -34 lines 0 comments Download
D trunk/src/media/midi/midi_message_util_unittest.cc View 1 chunk +0 lines, -34 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Ilya Sherman
7 years ago (2013-11-28 02:41:39 UTC) #1
Ilya Sherman
Committed patchset #1 manually as r237660.
7 years ago (2013-11-28 02:41:50 UTC) #2
Takashi Toyoshima
any informative data? I don't think this CL can affect performance since the code path ...
7 years ago (2013-11-28 02:58:16 UTC) #3
Ilya Sherman
7 years ago (2013-12-03 00:03:29 UTC) #4
Message was sent while issue was closed.
On 2013/11/28 02:58:16, Takashi Toyoshima (chromium) wrote:
> any informative data?
> I don't think this CL can affect performance since the code path runs iff a
user
> uses the MIDI API, and perf tests do not have such a test, AFAIK.

Yeah, this was an erroneous revert.  Looks like it's already been un-reverted. 
Apologies for the trouble!

Powered by Google App Engine
This is Rietveld 408576698