MiDIOutput.send() doesn't handle timestamp correctly
Reported by
maximech...@gmail.com,
Apr 22 2018
|
|||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.117 Safari/537.36 Steps to reproduce the problem: This was previously working, but my browser has just received an update, and it doesn't work correctly anymore. The MIDIOutput.send(msg, timestamp) function doesn't behave correctly when a nonzero timestamp is provided. The MIDI message is never sent. It does send the message immediately if the timestamp is zero or unspecified. I've attached a reduced test case. My specific setup was using an Arturia Drumbrute connected over USB, but you should be able to reproduce this with any MIDI device. You might need to change the MIDI channel from 1 to 0. What is the expected behavior? When a nonzero timestamp is specified, the event should play some time in the future if the timestamp is greater than the value produced by performance.now(). What went wrong? The MIDIOutput.send(msg, timestamp) function doesn't behave correctly when a nonzero timestamp is provided. The MIDI message is never sent. It does send the message immediately if the timestamp is zero or unspecified. Did this work before? Yes 65 Does this work in other browsers? N/A Chrome version: 66.0.3359.117 Channel: stable OS Version: OS X 10.12.6 Flash Version: This API probably needs automated tests?
,
Apr 23 2018
As per comment #0, it seems that the issue is reproducible using a MIDI device set up only which is not available with TE-team. Hence, ccing dev @toyoshim from owners url: https://cs.chromium.org/chromium/src/media/midi/OWNERS?q=Blink%3EWebMID&sq=package:chromium&dr=C&l=11 and requesting to please help us in looking into the issue. Thanks...!!
,
Apr 23 2018
hum... probably this is a regression of https://chromium-review.googlesource.com/c/chromium/src/+/892726 tzik, could you take a look?
,
Apr 23 2018
tzik: https://jsfiddle.net/toyoshim/rbf6b6v1/ this is a simple repro case. The problem seems to be platform independent.
,
Apr 23 2018
+yhirano just in case
,
Apr 23 2018
Thanks toyoshim@ for help based on C#3 and CL poitned out. This seems to have regressed in M-66. Hence, adding M-66 stable blocker for stable refresh, feel free to adjust or remove the blocker lable if this should not be blocking. Change log: https://www.google.com/url?q=https://chromium.googlesource.com/chromium/src/%2Blog/66.0.3337.0..66.0.3338.0?pretty%3Dfuller%26n%3D10000&sa=D&source=hangouts&ust=1524554237435000&usg=AFQjCNH-XU300lYaYe2OrqsctAF5BBCVzw. Note: Not sure about the OS in which the issue is reproducing as it can't be reproducible from TE-end due to unavailable set up. Thanks...!!
,
Apr 23 2018
re#6 The problem is platform independent, and my repro at #4 will work on Linux even without any real MIDI devices.
,
Apr 23 2018
tzik@ is looking.
,
Apr 24 2018
The previous one seems the culprit. Fixing... https://chromium-review.googlesource.com/c/chromium/src/+/895129
,
Apr 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e5ce9438849cb3ac042e45f8ee964204634be06 commit 0e5ce9438849cb3ac042e45f8ee964204634be06 Author: tzik <tzik@chromium.org> Date: Tue Apr 24 06:02:27 2018 Fix the time origin of delayed MIDIOutput::send MIDIOutput starts using Performance::timeOrigin() as the time origin of the delayed send request, which is wrong. As timeOrigin() is a web-exposed, it's relative to a different timing to TimeTicks value. We should use Performance::GetTimeOrigin() instread. Bug: 835664 Change-Id: I413ed6387351d55a72150163d09cfa2eb66bca92 Reviewed-on: https://chromium-review.googlesource.com/1025450 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#553012} [modify] https://crrev.com/0e5ce9438849cb3ac042e45f8ee964204634be06/third_party/blink/renderer/modules/webmidi/midi_output.cc
,
Apr 24 2018
The fix at #c10 looks working fine locally.
,
Apr 24 2018
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 25 2018
,
Apr 25 2018
Chatting with tzik@, this is tested locally, safe merge overall, and this is breaking webapps using WebMIDI. Fix is landing in Canary 68.0.3406. Conditionally approving CL for merge to M66 (once it's verified in canary).
,
Apr 25 2018
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 25 2018
Tested the issue on Win-10, mac 10.13.3 and ubuntu 17.10 using chrome version #68.0.3406.0 as per comment #4 and #7. Attached screenshots of Win-10, mac 10.13.3 and ubuntu 17.10 behaviour. Win-10 behaviour ================ Observed that MIDIOutput.send(msg, timestamp) function doesn't behave correctly when a nonzero timestamp is provided on Win-10 using both chrome with fix and chrome without fix. Mac 10.13.3 behaviour ===================== Observed that MIDIOutput.send(msg, timestamp) function doesn't behave correctly when a nonzero timestamp is provided on mac 10.13.3 using both chrome with fix and chrome without fix. ubuntu 17.10 behaviour ====================== Able to reproduce the issue using chrome without fix. But using chrome version #68.0.3406.0, observed that the message is sent immediately if the timestamp is non-zero as expected. Hence, fix is verified on ubuntu 17.10. tzik@ - Could you please check the screen shots and please let us know the different OS behaviour as per comment #7 and please confirm the fix. Thanks...!!
,
Apr 25 2018
tzik@, pls merge to M67 branch 3396 if test verification listed at #16 looks good to you.
,
Apr 26 2018
#c16 looks good. The repro case was for Linux only, as Web MIDI requires a MIDI device, and Linux has a default one.
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e08acd16ff25cc24fe9ed55810ed4395980d1104 commit e08acd16ff25cc24fe9ed55810ed4395980d1104 Author: tzik <tzik@chromium.org> Date: Thu Apr 26 03:23:08 2018 Fix the time origin of delayed MIDIOutput::send MIDIOutput starts using Performance::timeOrigin() as the time origin of the delayed send request, which is wrong. As timeOrigin() is a web-exposed, it's relative to a different timing to TimeTicks value. We should use Performance::GetTimeOrigin() instread. Bug: 835664 Change-Id: I413ed6387351d55a72150163d09cfa2eb66bca92 Reviewed-on: https://chromium-review.googlesource.com/1025450 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#553012}(cherry picked from commit 0e5ce9438849cb3ac042e45f8ee964204634be06) Reviewed-on: https://chromium-review.googlesource.com/1029690 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#318} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/e08acd16ff25cc24fe9ed55810ed4395980d1104/third_party/blink/renderer/modules/webmidi/midi_output.cc
,
Apr 26 2018
I verified that on the latest Chrome Canary on Windows with LoopMIDI. Landing the fix to M66 too.
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/391fec2a18cfc0e1738763eebab996c38f7b42ae commit 391fec2a18cfc0e1738763eebab996c38f7b42ae Author: tzik <tzik@chromium.org> Date: Thu Apr 26 05:15:54 2018 Fix the time origin of delayed MIDIOutput::send MIDIOutput starts using Performance::timeOrigin() as the time origin of the delayed send request, which is wrong. As timeOrigin() is a web-exposed, it's relative to a different timing to TimeTicks value. We should use Performance::GetTimeOrigin() instread. Bug: 835664 Change-Id: Ie1257eb037dcecb897dfc433578d7c5553e1b5b0 Reviewed-on: https://chromium-review.googlesource.com/1025450 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#553012}(cherry picked from commit 0e5ce9438849cb3ac042e45f8ee964204634be06) Reviewed-on: https://chromium-review.googlesource.com/1029555 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#769} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/391fec2a18cfc0e1738763eebab996c38f7b42ae/third_party/WebKit/Source/modules/webmidi/MIDIOutput.cpp
,
Apr 26 2018
Thank you for your quick response. How long should it take until this fix gets pushed to end users in a browser update?
,
Apr 27 2018
This is fixed after 66.0.3359.141, that will probably shipped in 10 days or so.
,
Apr 30 2018
Issue 837993 has been merged into this issue.
,
May 2 2018
Able to reproduce the issue on ubuntu 17.10 using chrome reported version #66.0.3359.117 as per comment #4 and #18. Verified the fix on Ubuntu 17.10 using Chrome beta version #67.0.3396.30 as per the comment #4 and #18. Attaching screen shot for reference. observed that the message is sent immediately if the timestamp is non-zero as expected. Hence, the fix is working as expected. Adding the verified labels after verifying fix on OS-linux.. Thanks...!!
,
May 9 2018
Update OS flags. Because this was a problem in Blink side, it affected all platforms that support Web MIDI. tzik@, than you for all your works. agoode@, thank you for triaging another report.
,
May 10 2018
Able to reproduce the issue on ubuntu 17.10 using chrome reported version #66.0.3359.117 as per comment #4 and #18. But unable to reproduce the issue on OS-win and OS-Linux due to no repro case for those OS. Verified the fix on Ubuntu 17.10 using Chrome version #66.0.3359.170 as per the comment #4 and #18. Attaching screen shot for reference. observed that the message is sent immediately if the timestamp is non-zero as expected. Thanks...!!
,
May 10 2018
I've verified this issue on "Debian GNU/Linux rodete" for Latest Stable RC#66.0.3359.170 and the fix is working as intended. PS: C#18 says that the repro case is applicable for "Linux" only. Thank you! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by susan.boorgula@chromium.org
, Apr 22 2018