Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1488693006:
Fix flaky WebAudio layout tests (Closed)
|
Created:
5 years ago by hongchan Modified:
5 years ago Reviewers:
Raymond Toy CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWith the suspend/resume feature in OfflineAudioContext, this CL fixes multiple
flaky tests in WebAudio related to the same issue; changing audio graph in the
middle of offline rendering causes the inconsistent result.
webaudio/audiochannelmerger-disconnect.html
webaudio/stereopannernode-no-glitch.html
webaudio/audiobuffersource-late-start.html
webaudio/audiochannelmerger-disconnect.html
BUG=469767, 519005, 520203, 520617
Committed: https://crrev.com/f22656aca1120584c3fb96580def7833c3cdd170
Cr-Commit-Position: refs/heads/master@{#363229}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed Feedback from rtoy #
Total comments: 7
Patch Set 3 : Feedback (2) #Messages
Total messages: 34 (14 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL.
Description was changed from ========== Fix flaky WebAudio layout tests With the suspend/resume feature in OfflineAudioContext, this CL fixes multiple flaky tests in WebAudio related to the same issue; changing audio graph in the middle of offline rendering causes the inconsistent result. webaudio/audiochannelmerger-disconnect.html webaudio/stereopannernode-no-glitch.html webaudio/audiobuffersource-late-start.html webaudio/audiochannelmerger-disconnect.html BUG=469767,519005,520203,520617 ========== to ========== With the suspend/resume feature in OfflineAudioContext, this CL fixes multiple flaky tests in WebAudio related to the same issue; changing audio graph in the middle of offline rendering causes the inconsistent result. webaudio/audiochannelmerger-disconnect.html webaudio/stereopannernode-no-glitch.html webaudio/audiobuffersource-late-start.html webaudio/audiochannelmerger-disconnect.html BUG=469767,519005,520203,520617 ==========
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488693006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488693006/1
https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:24: Since you only have one task now, I'd put all of these inside defineTask('test-late-start'). No globals needed. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:35: context.suspend(0.01).then(function () { Does it matter that 0.01 gets quantized to a rendering boundary? We probably want to make sure that 0.01 is never quantized to the first rendering quantum. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:52: } Can you use ES6 findIndex() to find the non-zero value? https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html:42: // Schedule the disconnection of |source2| at 1 second. We could make this test much faster if we didn't render so much. Maybe just run the test for a 1/4 second and disconnect at 1/8? https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html:52: // The second channel should contain 1, and 0 after the disconnection. Now that we know exactly when the disconnect happens, we should check that the second channel switches to 0 at exactly the right place. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audionode-disconnect-audioparam.html:57: // Schedule the disconnection at 1.0 second. Can we run the test for a much shorter duration? Say 1/4 sec total with disconnect at 1/8 sec? Does it matter if the disconnect isn't on a rendering quantum? Same issue for the tasks below. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audionode-disconnect-audioparam.html:65: // Expected values are: 1 * 1.5 * 1.5 -> 1 * 1.5 = [2.25, 1.5] We should probably check the values change at exactly the correct time since we now know exactly when the disconnect happens. Same issue for the other task below. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch-expected.txt (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch-expected.txt:9: PASS Channel #0 equals [1.156434465040231,NaN,...] with an element-wise tolerance of 1e-7. This NaN looks bad, as does the ones below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #2 (id:20001) has been deleted
It is possible to cut out the major portion of the stereo panner test, but I won't do it in this CL: also the test will be removed when the dezippering lands anyway. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:24: On 2015/12/01 22:14:37, Raymond Toy wrote: > Since you only have one task now, I'd put all of these inside > defineTask('test-late-start'). No globals needed. Done. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:35: context.suspend(0.01).then(function () { On 2015/12/01 22:14:37, Raymond Toy wrote: > Does it matter that 0.01 gets quantized to a rendering boundary? We probably > want to make sure that 0.01 is never quantized to the first rendering quantum. With the sample rate of 44100, 0.01 second is about 441 samples so it is larger than a single render quantum. I'd rather leave a comment here about this. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:52: } On 2015/12/01 22:14:37, Raymond Toy wrote: > Can you use ES6 findIndex() to find the non-zero value? NEAT! Done. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html:42: // Schedule the disconnection of |source2| at 1 second. On 2015/12/01 22:14:37, Raymond Toy wrote: > We could make this test much faster if we didn't render so much. Maybe just run > the test for a 1/4 second and disconnect at 1/8? I thought you'd ask! I will reduce the render length of all tests to quarter second. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html:52: // The second channel should contain 1, and 0 after the disconnection. On 2015/12/01 22:14:37, Raymond Toy wrote: > Now that we know exactly when the disconnect happens, we should check that the > second channel switches to 0 at exactly the right place. Done. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audionode-disconnect-audioparam.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audionode-disconnect-audioparam.html:57: // Schedule the disconnection at 1.0 second. On 2015/12/01 22:14:37, Raymond Toy wrote: > Can we run the test for a much shorter duration? Say 1/4 sec total with > disconnect at 1/8 sec? > > Does it matter if the disconnect isn't on a rendering quantum? > > Same issue for the tasks below. Done. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audionode-disconnect-audioparam.html:65: // Expected values are: 1 * 1.5 * 1.5 -> 1 * 1.5 = [2.25, 1.5] On 2015/12/01 22:14:37, Raymond Toy wrote: > We should probably check the values change at exactly the correct time since we > now know exactly when the disconnect happens. > > Same issue for the other task below. Done. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch-expected.txt (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch-expected.txt:9: PASS Channel #0 equals [1.156434465040231,NaN,...] with an element-wise tolerance of 1e-7. On 2015/12/01 22:14:37, Raymond Toy wrote: > This NaN looks bad, as does the ones below. Done. It was a stupid mistake. (audiobuffer length of 1)
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488693006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488693006/40001
https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html (right): https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:50: return index; Would it be better to return explicitly true or false for the callback function? https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:58: testPassed('The rendered buffer contains non-zero values after the first sample.'); Should we check that the first nonZeroValueIndex is exactly the right value? When know exactly when source.start(0) is called. https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html (right): https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html:61: testPassed('Transition found between sample #' + start + ' and #' + end + '.'); I know this isn't part of this CL, but is there any way to know what the actual start and end values should be?
https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/audiochannelmerger-disconnect.html:42: // Schedule the disconnection of |source2| at 1 second. On 2015/12/02 18:58:31, hoch wrote: > On 2015/12/01 22:14:37, Raymond Toy wrote: > > We could make this test much faster if we didn't render so much. Maybe just > run > > the test for a 1/4 second and disconnect at 1/8? > > I thought you'd ask! I will reduce the render length of all tests to quarter > second. Seems to me that you did 1/2 sec for the tests. That's ok though. https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch-expected.txt (right): https://codereview.chromium.org/1488693006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch-expected.txt:9: PASS Channel #0 equals [1.156434465040231,NaN,...] with an element-wise tolerance of 1e-7. On 2015/12/02 18:58:32, hoch wrote: > On 2015/12/01 22:14:37, Raymond Toy wrote: > > This NaN looks bad, as does the ones below. > > Done. It was a stupid mistake. (audiobuffer length of 1) We need to fix that. NaN does not meet an element-wise tolerance of anything. :-) A different CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html (right): https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:50: return index; On 2015/12/02 19:33:41, Raymond Toy wrote: > Would it be better to return explicitly true or false for the callback function? Done. https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audiobuffersource-late-start.html:58: testPassed('The rendered buffer contains non-zero values after the first sample.'); On 2015/12/02 19:33:41, Raymond Toy wrote: > Should we check that the first nonZeroValueIndex is exactly the right value? > When know exactly when source.start(0) is called. Done. https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html (right): https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html:61: testPassed('Transition found between sample #' + start + ' and #' + end + '.'); On 2015/12/02 19:33:41, Raymond Toy wrote: > I know this isn't part of this CL, but is there any way to know what the actual > start and end values should be? 1. This extractPanningTransition() method is actually not necessary because we have suspend/resume. We know when the transition starts/ends so there no need to find the transition manually. 2. However, if you really want to find out the start/end value, we can just cache two values from two while statement.
lgtm with nit. You can decide what to do with the start/end transition stuff. https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html (right): https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html:61: testPassed('Transition found between sample #' + start + ' and #' + end + '.'); On 2015/12/03 19:28:31, hoch wrote: > On 2015/12/02 19:33:41, Raymond Toy wrote: > > I know this isn't part of this CL, but is there any way to know what the > actual > > start and end values should be? > > 1. This extractPanningTransition() method is actually not necessary because we > have suspend/resume. We know when the transition starts/ends so there no need to > find the transition manually. > > 2. However, if you really want to find out the start/end value, we can just > cache two values from two while statement. Well, the test always passes so it seems not really useful except as an informational message. (It would be nice if js-test had a testInfo method.) Ideally, if we could a priori determine what start and end are supposed to be and then verify that the actual data had these values, we would make the test more useful. As it is now, the test just tells you what was found, but we don't actually know if that's correct. So this test will only tell us if the start and end values ever change. Which might be good enough?
On 2015/12/03 19:50:54, Raymond Toy wrote: > lgtm with nit. You can decide what to do with the start/end transition stuff. > > https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html > (right): > > https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html:61: > testPassed('Transition found between sample #' + start + ' and #' + end + '.'); > On 2015/12/03 19:28:31, hoch wrote: > > On 2015/12/02 19:33:41, Raymond Toy wrote: > > > I know this isn't part of this CL, but is there any way to know what the > > actual > > > start and end values should be? > > > > 1. This extractPanningTransition() method is actually not necessary because we > > have suspend/resume. We know when the transition starts/ends so there no need > to > > find the transition manually. > > > > 2. However, if you really want to find out the start/end value, we can just > > cache two values from two while statement. > > Well, the test always passes so it seems not really useful except as an > informational message. (It would be nice if js-test had a testInfo method.) > > Ideally, if we could a priori determine what start and end are supposed to be > and then verify that the actual data had these values, we would make the test > more useful. As it is now, the test just tells you what was found, but we don't > actually know if that's correct. So this test will only tell us if the start > and end values ever change. Which might be good enough? I think it is about salvaging this test. Actually, once dezippering is gone the only way to avoid glitch is using AudioParam methods. So if you want to keep this test, we need to rethink the purpose or the goal of it.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488693006/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488693006/50001
On 2015/12/03 20:49:16, hoch wrote: > On 2015/12/03 19:50:54, Raymond Toy wrote: > > lgtm with nit. You can decide what to do with the start/end transition stuff. > > > > > https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html > > (right): > > > > > https://codereview.chromium.org/1488693006/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/webaudio/stereopannernode-no-glitch.html:61: > > testPassed('Transition found between sample #' + start + ' and #' + end + > '.'); > > On 2015/12/03 19:28:31, hoch wrote: > > > On 2015/12/02 19:33:41, Raymond Toy wrote: > > > > I know this isn't part of this CL, but is there any way to know what the > > > actual > > > > start and end values should be? > > > > > > 1. This extractPanningTransition() method is actually not necessary because > we > > > have suspend/resume. We know when the transition starts/ends so there no > need > > to > > > find the transition manually. > > > > > > 2. However, if you really want to find out the start/end value, we can just > > > cache two values from two while statement. > > > > Well, the test always passes so it seems not really useful except as an > > informational message. (It would be nice if js-test had a testInfo method.) > > > > Ideally, if we could a priori determine what start and end are supposed to be > > and then verify that the actual data had these values, we would make the test > > more useful. As it is now, the test just tells you what was found, but we > don't > > actually know if that's correct. So this test will only tell us if the start > > and end values ever change. Which might be good enough? > > I think it is about salvaging this test. Actually, once dezippering is gone the > only way to avoid glitch is using AudioParam methods. So if you want to keep > this test, we need to rethink the purpose or the goal of it. Just leave it as in then.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488693006/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488693006/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by hongchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488693006/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488693006/50001
Message was sent while issue was closed.
Description was changed from ========== With the suspend/resume feature in OfflineAudioContext, this CL fixes multiple flaky tests in WebAudio related to the same issue; changing audio graph in the middle of offline rendering causes the inconsistent result. webaudio/audiochannelmerger-disconnect.html webaudio/stereopannernode-no-glitch.html webaudio/audiobuffersource-late-start.html webaudio/audiochannelmerger-disconnect.html BUG=469767,519005,520203,520617 ========== to ========== With the suspend/resume feature in OfflineAudioContext, this CL fixes multiple flaky tests in WebAudio related to the same issue; changing audio graph in the middle of offline rendering causes the inconsistent result. webaudio/audiochannelmerger-disconnect.html webaudio/stereopannernode-no-glitch.html webaudio/audiobuffersource-late-start.html webaudio/audiochannelmerger-disconnect.html BUG=469767,519005,520203,520617 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:50001)
Message was sent while issue was closed.
Description was changed from ========== With the suspend/resume feature in OfflineAudioContext, this CL fixes multiple flaky tests in WebAudio related to the same issue; changing audio graph in the middle of offline rendering causes the inconsistent result. webaudio/audiochannelmerger-disconnect.html webaudio/stereopannernode-no-glitch.html webaudio/audiobuffersource-late-start.html webaudio/audiochannelmerger-disconnect.html BUG=469767,519005,520203,520617 ========== to ========== With the suspend/resume feature in OfflineAudioContext, this CL fixes multiple flaky tests in WebAudio related to the same issue; changing audio graph in the middle of offline rendering causes the inconsistent result. webaudio/audiochannelmerger-disconnect.html webaudio/stereopannernode-no-glitch.html webaudio/audiobuffersource-late-start.html webaudio/audiochannelmerger-disconnect.html BUG=469767,519005,520203,520617 Committed: https://crrev.com/f22656aca1120584c3fb96580def7833c3cdd170 Cr-Commit-Position: refs/heads/master@{#363229} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f22656aca1120584c3fb96580def7833c3cdd170 Cr-Commit-Position: refs/heads/master@{#363229} |
