Web MIDI: data loss on receiving continuous sysex on Chrome 58+ on Windows |
|||||||||||||
Issue descriptionChrome Version: Chrome 58+ OS: Windows What steps will reproduce the problem? (1) Install (or use) a loopback thing (such as http://www.tobias-erichsen.de/software/loopmidi.html) (2) Open up https://jsfiddle.net/toyoshim/wjvjnqzx/ (and open the dev console) (3) See console What is the expected result? No error log What happens instead? Data loss is detected
,
Jun 7 2017
Thanks for the Quick help! Can I just state for the record (if that's able to add any weight in terms of stable branch merging) that this currently very likely breaks production code on at least two rather popular customer facing applications: - https://components.novationmusic.com - https://soundmondo.yamahasynth.com/ both are applications to manage content of hardware synthesizers and are somewhat without alternatives for the users. (I reported the bug originally and I am one of the maintainers of Novation Components)
,
Jun 7 2017
Thank you, Jan Let me share a workaround for advanced users who really need a quick fix. Flip an experimental flag at chrome://flags/#use-winrt-midi-api to Enabled. This will switch to use New Windows 10 API to manage MIDI devices, but there is a known issue that if users use other applications that call legacy MIDI API together. Once this conflict happens, Windows MIDI system may be confused, and will require system reboot to use MIDI system again. I will try to fix the problem as soon as possible, and try merging the fix to release branches. Fix itself is very simple, but I can not promise if my fix can meet the bar for merging to m59 Stable.
,
Jun 7 2017
I forgot to explain, but chrome://flags/#use-winrt-midi-api works only on Windows 10, and we do not have any workaround on other Windows versions such as 7 and 8.
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1814e6af281cd1f00ddf0705c01e2484708b0e4d commit 1814e6af281cd1f00ddf0705c01e2484708b0e4d Author: toyoshim <toyoshim@chromium.org> Date: Wed Jun 07 23:46:03 2017 Web MIDI: restore sysex receiving buffer synchronously The new backend restores sysex receiving buffer asynchronously, but it turned out that this way causes a receiving data loss on receiving continuous sysex messages. The original backend did it synchronously and didn't have such problem. So let me fix the new backend to take the same approach. BUG= 730647 Review-Url: https://codereview.chromium.org/2926143003 Cr-Commit-Position: refs/heads/master@{#477807} [modify] https://crrev.com/1814e6af281cd1f00ddf0705c01e2484708b0e4d/media/midi/midi_manager_win.cc
,
Jun 8 2017
,
Jun 8 2017
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 8 2017
hi toyoshim@ - thanks for the quick bug fix. My recommendation is to wait for M60. It appears that there is a workaround and the impact doesn't seem very large. Since M59 is already being pushed to stable users, the bar is very high for new merges. If you think this is absolutely critical and can not wait until M60, please let us know.
,
Jun 8 2017
hi abdulsyed@ - toyoshim@ told me that this is a possible outcome, but I must say as the original reporter that I am a bit disappointed. I'm not sure what counts as high impact (I assume you judge that by affected users, which is fair, I guess) but we have real users (clients, actually) that use our software every day as professional and hobbyist musicians. Our software is broken for them on Chrome and it is quite an ask for me to point them to a chrome flag that potentially breaks MIDI on their systems on a regular basis and forces them to reboot. My main gripe is that we're only a few days after the last stable release, so we'll have to wait about 6 weeks (that's the current release cycle, right?) to get a fix out to users while at the same time be afraid that the bug spreads to our fallbacks (Opera et al.) and we'll be without any fallback at all for a couple of weeks. Anyway, I totally understand that there are calls to be made and they are difficult and all, and I'm also really happy toyoshim@ was able to fix this so quickly, but I hope you can also understand my frustration about having to wait so long to get a fix that will actually work for my users. (I'll shut up now and let y'all do your job)
,
Jun 8 2017
abdulsyed@ thank you for comments. By the way, why this is not approved automatically even for M-60? Also, for M-59 request, let me explain a little more background to answer questions too, though Jan already provided a precious feedback and great explanations. > It appears that there is a workaround The workaround requires to flip an unsupported experimental flag at chrome://flags, and we already noticed that this flag should be used carefully, because Windows gets confused and stop working until rebooting the operating system. Also it's supported only on Windows 10. So it isn't a solution for typical end users. > the impact doesn't seem very large The number of affected people should be limited. That's right. But for users who has used the service and products Jan mentioned, this problem makes their instruments and services completely useless over a month. So the problem is serious for them. I think this should not be an issue that requires a stable refresh only for this fix, but want to ride on a train together if there are other fixes and chance to ride. The patch itself is very simple one, that removes 3 lines, and adds 1 line. That's all my explanations, and will wait for responsible person's decision.
,
Jun 8 2017
OK great, thank you so much for the feedback. Based on comment10, approving merge for M59 and M60.
,
Jun 8 2017
can you please confirm if this is well tested/verified in canary?
,
Jun 8 2017
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 9 2017
Hi abdulsyed, is the branch for the 60 3112, right? Jan, I can verify Canary working with soundmondo. Could you verify it with components?
,
Jun 9 2017
yes, it's 3112 for m60.
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7829724b2cb160c6d96f17bb8738d4c58c5ecd6f commit 7829724b2cb160c6d96f17bb8738d4c58c5ecd6f Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Fri Jun 09 07:06:35 2017 Web MIDI: restore sysex receiving buffer synchronously The new backend restores sysex receiving buffer asynchronously, but it turned out that this way causes a receiving data loss on receiving continuous sysex messages. The original backend did it synchronously and didn't have such problem. So let me fix the new backend to take the same approach. BUG= 730647 (cherry picked from commit 1814e6af281cd1f00ddf0705c01e2484708b0e4d) Review-Url: https://codereview.chromium.org/2926143003 Cr-Original-Commit-Position: refs/heads/master@{#477807} Change-Id: I493750ac10ab2ece46fa40aa05a3ae296ea1d6f7 Reviewed-on: https://chromium-review.googlesource.com/527546 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#274} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/7829724b2cb160c6d96f17bb8738d4c58c5ecd6f/media/midi/midi_manager_win.cc
,
Jun 9 2017
Can verify Novation Components is working fine with the current win32 canary. Thanks again for the super speedy fix everyone!
,
Jun 9 2017
Thanks for confirming - approving merge for M59. Branch is 3071.
,
Jun 9 2017
Please make sure to merge before Sunday 5pm (pacific time) to make RC for next stable release.
,
Jun 10 2017
I just confirmed soundmondo too, and will submit a merge change soon.
,
Jun 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f6e6469cc6ccb2b6a5d9c843754d76f25a17d133 commit f6e6469cc6ccb2b6a5d9c843754d76f25a17d133 Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Sat Jun 10 03:07:58 2017 Web MIDI: restore sysex receiving buffer synchronously The new backend restores sysex receiving buffer asynchronously, but it turned out that this way causes a receiving data loss on receiving continuous sysex messages. The original backend did it synchronously and didn't have such problem. So let me fix the new backend to take the same approach. BUG= 730647 (cherry picked from commit 1814e6af281cd1f00ddf0705c01e2484708b0e4d) Review-Url: https://codereview.chromium.org/2926143003 Cr-Original-Original-Commit-Position: refs/heads/master@{#477807} Change-Id: I72470e9c91754f6858dcd513c7d4a527411097b4 Reviewed-on: https://chromium-review.googlesource.com/527783 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/branch-heads/3071@{#770} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/f6e6469cc6ccb2b6a5d9c843754d76f25a17d133/media/midi/dynamically_initialized_midi_manager_win.cc
,
Jun 10 2017
Submitted a merge fix, that needed a manual rebase due to a file rename.
,
Jun 14 2017
Tested the issue on Win-10 using chrome beta version #60.0.3112.32. Attached a screen cast for reference. Following are the steps followed to reproduce the issue. ------------ 1. Installed a loopmidi from URL: http://www.tobias-erichsen.de/software/loopmidi.html (2) Opened https://jsfiddle.net/toyoshim/wjvjnqzx/ in chrome browser and opened the dev console. (3)Observed error log in dev console. toyoshim@ - Could you please verify the screen cast and please confirm the expected behavior. Thanks...!!
,
Jun 14 2017
You added a virtual port with the name "loopMIDI Port1", but could you change the name to be "loopMIDI Port" that the test expects? (the last "1" should be removed) In your movie the test fails to find the test device.
,
Jun 14 2017
toyoshim@ - Thanks for clarification...!! Verified the issue on windows 10 using chrome beta version #60.0.3112.32 as per comment #0. Observed No error log in dev console. Hence, the fix is working as expected. Attaching screen shot for reference. Hence, adding the verified labels. Thanks...!!
,
Jun 15 2017
Thanks, the last screen shot proves the test runs correctly!
,
Jun 15 2017
Verified this issue on Windows-10 using chrome latest stable #59.0.3071.104 by following steps mentioned in the original comment and observed no error log in the console as expected. Hence adding TE-Verified label. Thanks!
,
Jun 15 2017
thank you so much! |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by toyoshim@chromium.org
, Jun 7 2017