213424 2020-06-19 22:09:08 -0700 compositing/video/video-border-radius-clipping.html was a flaky failure after r263223 2020-06-22 19:34:15 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ggaren ggaren ap darin eric.carlson ews-watchlist glenn jer.noble philipj sergio simon.fraser webkit-bug-importer oldest_to_newest 1664703 0 ggaren 2020-06-19 22:09:08 -0700 compositing/video/video-border-radius-clipping.html was a flaky failure after r263223 1664704 1 402377 ggaren 2020-06-19 22:48:11 -0700 Created attachment 402377 Patch 1664758 2 402377 ap 2020-06-20 11:22:06 -0700 Comment on attachment 402377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402377&action=review > LayoutTests/ChangeLog:13 > + I took a screen recording of a few hundred loads of this test @ r263222, > + and it looks like it was always possible for canplaythrough and seeked > + to fire before the video had rendered its first frame. So, this is a > + test issue. canplaythrough is indeed obviously unrelated to rendering the first frame, but dispatching seeked without rendering sounds like an actual bug, not a test issue. It this really allowed? 1664767 3 ggaren 2020-06-20 12:21:08 -0700 > > LayoutTests/ChangeLog:13 > > + I took a screen recording of a few hundred loads of this test @ r263222, > > + and it looks like it was always possible for canplaythrough and seeked > > + to fire before the video had rendered its first frame. So, this is a > > + test issue. > > canplaythrough is indeed obviously unrelated to rendering the first frame, > but dispatching seeked without rendering sounds like an actual bug, not a > test issue. It this really allowed? You're asking the wrong question. This is a test of border radius. When we analyze a change to this test, we should ask whether the test still correctly measures border radius. That said, my reading is that it is allowed for the seeked event to fire before a video frame has rendered. The specification never speaks of what's rendered onscreen. For seeking, it says, "Wait until the user agent has established whether or not the media data for the new playback position is available, and, if it is, until it has decoded enough data to play back that position." So, seeked is fired when the seeked position has decoded, not when it has rendered onscreen. 1664771 4 ap 2020-06-20 13:03:36 -0700 Well, there are two questions here. One is the one you think is right, whether the test still correctly measures border radius. Another is whether the test discovered an actual bug, introduced or triggered by changes in r263223. I did note that you observed something similar with r263222, but that doesn't explain why the test regressed. Assuming that your goal is to re-land r263223, we need an answer to the second question. Sounds like you think that the test always relied on something unspecified, so the change is OK and won't break websites. Let's have media experts confirm your understanding. 1664788 5 ggaren 2020-06-20 14:49:44 -0700 > I did note that you observed something similar with r263222, but > that doesn't explain why the test regressed. Actually, it does. The timing of a screenshot vs a media element's events is flaky. It was flaky in December (r253310), it was flaky in r263222, and it was flaky in r263223. Flaky tests sometimes cause bot failures; that does not indicate a regression in behavior. > Assuming that your goal is to re-land r263223, we need an answer to the > second question. Sounds like you think that the test always relied on > something unspecified, so the change is OK and won't break websites. Let's > have media experts confirm your understanding. Websites don't take screenshots. 1664791 6 ap 2020-06-20 16:16:08 -0700 No, general flakiness does not explain the observed regression. You could say that it’s not worth investigating extra flakiness when the whole area is flaky, and that would be fairly convincing. But claiming that you have an explanation at this point is untrue. We’ve seen plenty of processing done by websites via capturing video frames. So not sure what you are talking about with regards to websites not taking screenshots. Literal screenshots, they do not, but rendering timing bugs are not limited to literal screenshots. 1664877 7 402377 darin 2020-06-21 13:13:01 -0700 Comment on attachment 402377 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402377&action=review >> LayoutTests/ChangeLog:13 >> + test issue. > > canplaythrough is indeed obviously unrelated to rendering the first frame, but dispatching seeked without rendering sounds like an actual bug, not a test issue. It this really allowed? I suggest investigating that further with a purpose-built test, rather than testing it accidentally with this test. 1664879 8 ews-feeder 2020-06-21 13:20:27 -0700 Committed r263332: <https://trac.webkit.org/changeset/263332> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402377. 1664880 9 webkit-bug-importer 2020-06-21 13:21:15 -0700 <rdar://problem/64578547> 1665055 10 ap 2020-06-22 09:08:54 -0700 > I suggest investigating that further with a purpose-built test, rather than testing it accidentally with this test. I consider re-landing r263223 blocked until this investigation is complete. Rewriting a test to swipe a regression under the carpet does not resolve the regression. 1665081 11 ggaren 2020-06-22 10:09:47 -0700 (In reply to Alexey Proskuryakov from comment #10) > > I suggest investigating that further with a purpose-built test, rather than testing it accidentally with this test. > > I consider re-landing r263223 blocked until this investigation is complete. My investigation is complete. It shows that the canplaythrough and seeked events, by pre-existing behavior and by specification, do not synchronize with on-screen rendering. If you see something more to investigate here, you're welcome to do so. I'm not going to withhold a patch that has been reviewed and that passes all tests. If you'd like to propose a new standard for patches, that says they must pass review, and all tests, and personal approval by Alexey Proskuryakov, and all tests that could possibly have been written even though they can't be specifically described and they may not relate to any specified or necessary behavior, please send email to webkit-dev and we can consider that policy change as a team. 1665087 12 ap 2020-06-22 10:17:42 -0700 There is no new standard here. Your patch broke tests, you don't know why the change is OK, so you just changed the tests until they passed. Now you are just escalating to personal attacks instead of civil discussion. 1665110 13 darin 2020-06-22 12:25:54 -0700 I agree that the test detected a change in behavior. But I am thinking it did not detect a change in behavior that we should care about. Instead they detected a way to accidentally depend on race conditions. Certainly website developers could make the same kinds of mistakes and there’s a reason to want tests that check these sorts of things. I’m not sure how to write those tests, though, since the code wouldn’t and shouldn’t try to time things consistently. We could probe this further, but I’m not sure there’s reason to believe this is worthwhile. 1665250 14 ap 2020-06-22 18:38:08 -0700 The test detected the race getting more likely to occur. My point is that no one checked whether there is a race here in other browsers. And Geoff not finding it in the previously unfamiliar spec is weak evidence that the racy behavior is OK. This is why I was suggesting to have media experts weigh in before it got personal. If something works 100% in other browsers, and mostly works in shipping Safari, saying that we are free to make it fail more in Safari makes no sense. As for this being worthwhile to probe further - I think that non-urgent refactoring that makes observed behavior worse is a questionable trade-off. 1665264 15 ggaren 2020-06-22 19:34:15 -0700 Not sure what you mean by "personal attacks" and "got personal". I wish you would directly address my reasonable technical and project comments, rather than dismissing them as "personal" and not "civil". 402377 2020-06-19 22:48:11 -0700 2020-06-21 13:20:28 -0700 Patch bug-213424-20200619224810.patch text/plain 3268 ggaren SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9D aGFuZ2VMb2cJKHJldmlzaW9uIDI2MzMxNikKKysrIExheW91dFRlc3RzL0NoYW5nZUxvZwkod29y a2luZyBjb3B5KQpAQCAtMSwzICsxLDMxIEBACisyMDIwLTA2LTE5ICBHZW9mZnJleSBHYXJlbiAg PGdnYXJlbkBhcHBsZS5jb20+CisKKyAgICAgICAgY29tcG9zaXRpbmcvdmlkZW8vdmlkZW8tYm9y ZGVyLXJhZGl1cy1jbGlwcGluZy5odG1sIHdhcyBhIGZsYWt5IGZhaWx1cmUgYWZ0ZXIgcjI2MzIy MworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjEzNDI0 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIGxh c3QgYXR0ZW1wdCB0byB1bmZsYWtlIHRoaXMgdGVzdCB3YXMgcjI1MzMxMC4gTGV0J3MgdHJ5IGFn YWluLgorCisgICAgICAgIEkgdG9vayBhIHNjcmVlbiByZWNvcmRpbmcgb2YgYSBmZXcgaHVuZHJl ZCBsb2FkcyBvZiB0aGlzIHRlc3QgQCByMjYzMjIyLAorICAgICAgICBhbmQgaXQgbG9va3MgbGlr ZSBpdCB3YXMgYWx3YXlzIHBvc3NpYmxlIGZvciBjYW5wbGF5dGhyb3VnaCBhbmQgc2Vla2VkCisg ICAgICAgIHRvIGZpcmUgYmVmb3JlIHRoZSB2aWRlbyBoYWQgcmVuZGVyZWQgaXRzIGZpcnN0IGZy YW1lLiBTbywgdGhpcyBpcyBhCisgICAgICAgIHRlc3QgaXNzdWUuCisKKyAgICAgICAgVGhlIHNj cmVlbiByZWNvcmRpbmcgc2VlbXMgdG8gc2hvdyB0aGF0IHdhaXRpbmcgZm9yCisgICAgICAgIHJl cXVlc3RBbmltYXRpb25GcmFtZSBlbnN1cmVzIHRoYXQgdGhlIHZpZGVvIHJlbmRlcnMgaXRzIGZp cnN0IGZyYW1lLgorICAgICAgICBUaGlzIG1pZ2h0IGJlIGZvciBhIHByaW5jaXBsZWQgc3lzdGVt IHZzeW5jIHJlYXNvbiwgb3IganVzdCBhIGx1Y2t5CisgICAgICAgIGRlbGF5LiBFaXRoZXIgd2F5 LCB0aGUgdGVzdCBzZWVtcyBsZXNzIGZsYWt5IG5vdy4KKworICAgICAgICBUaGlzIG1pZ2h0IG1l YW4gdGhhdCB3ZSBjYW4gcmVtb3ZlIHRoZSBwcmV2aW91cyBzZWVrZWQgZXZlbnQgd29ya2Fyb3Vu ZCwKKyAgICAgICAgYnV0IEkgY2hvc2Ugbm90IHRvIHRlbXB0IGZhdGUsIGFuZCBsZWZ0IGl0IGlu IHBsYWNlLgorCisgICAgICAgICogY29tcG9zaXRpbmcvdmlkZW8vdmlkZW8tYm9yZGVyLXJhZGl1 cy1jbGlwcGluZy1leHBlY3RlZC5odG1sOgorICAgICAgICAqIGNvbXBvc2l0aW5nL3ZpZGVvL3Zp ZGVvLWJvcmRlci1yYWRpdXMtY2xpcHBpbmcuaHRtbDoKKyAgICAgICAgKiBtZWRpYS92aWRlby10 ZXN0LmpzOgorICAgICAgICAoX2V2ZW50Q2FsbGJhY2spOgorICAgICAgICAod2FpdEZvckV2ZW50 KToKKwogMjAyMC0wNi0xOSAgTXlsZXMgQy4gTWF4ZmllbGQgIDxtbWF4ZmllbGRAYXBwbGUuY29t PgogCiAgICAgICAgIGZhc3QvdGV4dC9pbnRlcm5hdGlvbmFsL3N5c3RlbS1sYW5ndWFnZS9uYXZp Z2F0b3ItbGFuZ3VhZ2UvbmF2aWdhdG9yLWxhbmd1YWdlIHRlc3RzIGFyZSBmYWlsaW5nIG9uIENh dGFsaW5hIGFuZCBpT1MKSW5kZXg6IExheW91dFRlc3RzL2NvbXBvc2l0aW5nL3ZpZGVvL3ZpZGVv LWJvcmRlci1yYWRpdXMtY2xpcHBpbmctZXhwZWN0ZWQuaHRtbAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlv dXRUZXN0cy9jb21wb3NpdGluZy92aWRlby92aWRlby1ib3JkZXItcmFkaXVzLWNsaXBwaW5nLWV4 cGVjdGVkLmh0bWwJKHJldmlzaW9uIDI2MzIyNikKKysrIExheW91dFRlc3RzL2NvbXBvc2l0aW5n L3ZpZGVvL3ZpZGVvLWJvcmRlci1yYWRpdXMtY2xpcHBpbmctZXhwZWN0ZWQuaHRtbAkod29ya2lu ZyBjb3B5KQpAQCAtNDIsOCArNDIsOCBAQAogICAgICAgICBmdW5jdGlvbiBkb1Rlc3QoKQogICAg ICAgICB7CiAgICAgICAgICAgICBmaW5kTWVkaWFFbGVtZW50KCk7Ci0gICAgICAgICAgICB3YWl0 Rm9yRXZlbnQoJ2NhbnBsYXl0aHJvdWdoJywgY2FucGxheXRocm91Z2gpOwotICAgICAgICAgICAg d2FpdEZvckV2ZW50KCdzZWVrZWQnLCB0ZXN0Q29tcGxldGUpOworICAgICAgICAgICAgd2FpdEZv ckV2ZW50KCdjYW5wbGF5dGhyb3VnaCcsICgpID0+IHJlcXVlc3RBbmltYXRpb25GcmFtZSgoKSA9 PiB7IGNhbnBsYXl0aHJvdWdoKCk7IH0pKTsKKyAgICAgICAgICAgIHdhaXRGb3JFdmVudCgnc2Vl a2VkJywgKCkgPT4gcmVxdWVzdEFuaW1hdGlvbkZyYW1lKCgpID0+IHsgdGVzdENvbXBsZXRlKCk7 IH0pKTsKICAgICAgICAgICAgIHZpZGVvLnNyYyA9IGZpbmRNZWRpYUZpbGUoJ3ZpZGVvJywgJy4u Ly4uL21lZGlhL2NvbnRlbnQvcHVyZS1jb2xvci1ncmVlbicpOwogICAgICAgICB9CiAgICAgICAg IApJbmRleDogTGF5b3V0VGVzdHMvY29tcG9zaXRpbmcvdmlkZW8vdmlkZW8tYm9yZGVyLXJhZGl1 cy1jbGlwcGluZy5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2NvbXBvc2l0aW5nL3Zp ZGVvL3ZpZGVvLWJvcmRlci1yYWRpdXMtY2xpcHBpbmcuaHRtbAkocmV2aXNpb24gMjYzMjI2KQor KysgTGF5b3V0VGVzdHMvY29tcG9zaXRpbmcvdmlkZW8vdmlkZW8tYm9yZGVyLXJhZGl1cy1jbGlw cGluZy5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC00Myw4ICs0Myw4IEBACiAgICAgICAgIGZ1bmN0 aW9uIGRvVGVzdCgpCiAgICAgICAgIHsKICAgICAgICAgICAgIGZpbmRNZWRpYUVsZW1lbnQoKTsK LSAgICAgICAgICAgIHdhaXRGb3JFdmVudCgnY2FucGxheXRocm91Z2gnLCBjYW5wbGF5dGhyb3Vn aCk7Ci0gICAgICAgICAgICB3YWl0Rm9yRXZlbnQoJ3NlZWtlZCcsIHRlc3RDb21wbGV0ZSk7Cisg ICAgICAgICAgICB3YWl0Rm9yRXZlbnQoJ2NhbnBsYXl0aHJvdWdoJywgKCkgPT4gcmVxdWVzdEFu aW1hdGlvbkZyYW1lKCgpID0+IHsgY2FucGxheXRocm91Z2goKTsgfSkpOworICAgICAgICAgICAg d2FpdEZvckV2ZW50KCdzZWVrZWQnLCAoKSA9PiByZXF1ZXN0QW5pbWF0aW9uRnJhbWUoKCkgPT4g eyB0ZXN0Q29tcGxldGUoKTsgfSkpOwogICAgICAgICAgICAgdmlkZW8uc3JjID0gZmluZE1lZGlh RmlsZSgndmlkZW8nLCAnLi4vLi4vbWVkaWEvY29udGVudC9wdXJlLWNvbG9yLWdyZWVuJyk7CiAg ICAgICAgIH0KICAgICAgICAgCg==