| Summary: | [cairo][SVG] Putting multiple path elements in clippath causes rendering artifacts | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||||||||||
| Component: | Platform | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | cgarcia, commit-queue, ht990332, magomez, mcatanzaro, tsavell, webkit-bug-importer, zan | ||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=23526 https://bugs.webkit.org/show_bug.cgi?id=198746 |
||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||
| Bug Blocks: | 169001 | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Fujii Hironori
2019-06-09 20:14:48 PDT
Created attachment 371716 [details]
clip-path.html
Created attachment 371717 [details]
clip-path-2.html
Created attachment 371718 [details]
clip-path-2.html
Created attachment 371720 [details]
clip-path-3.html
Created attachment 371722 [details] WIP patch I confirmed this patch solves this issue and YouTube's volume button issue (Bug 169001). Good catch, I'm not a graphics expert, but I don't understand why we need to blit the current target surface into the cairo group right after creating it. I've run the tests with your patch and I didn't get any regression, I got two unexpected passes: svg/gradients/spreadMethodDiagonal3.svg svg/gradients/spreadMethodDiagonal4.svg Thanks for testing. I'm neither a graphics expert, and don't know the reason. I'm guessing it's for alpha brending order. Created attachment 371821 [details]
a test case of clip-path and opacity
Created attachment 371822 [details]
another test case of clip-path and opacity
Created attachment 371824 [details]
test case of clip-path and opcity comparing simgle and multiple clipper objects
It seems to be somehow reusing the content of first tile of the page as the background of the clip path for some reason. Weird. I'll add it to my list. It's clear that your patch fixes several test cases, made two layout tests pass and even more important fixes the youtube volume glitch. And no layout test regressions, so I would just land it. That code came from https://bugs.webkit.org/show_bug.cgi?id=169094 Removing it should break at least fast/hidpi/hidpi-long-page-with-inset-element-expected.html, that was added with the patch for it. Anyway, from what I see we shouldn't just remove the code. There must be something that's missing. (In reply to Miguel Gomez from comment #13) > That code came from https://bugs.webkit.org/show_bug.cgi?id=169094 > > Removing it should break at least > fast/hidpi/hidpi-long-page-with-inset-element-expected.html, that was added > with the patch for it. > > Anyway, from what I see we shouldn't just remove the code. There must be > something that's missing. No, the problem is not the part to work around the pixman limitation, the problem is the part that paints the current target surface into the group right after creating it. And that comes from https://bugs.webkit.org/show_bug.cgi?id=23526 I can confirm fast/hidpi/hidpi-long-page-with-inset-element.html still passes with this patch, Created attachment 371837 [details] patch solving only position issues I created another patch, but this patch solves only position issues, but blending issues. I don't know why this patch doesn't work. It seems that we should remove the blit in despite of the comment in PlatformContextCairo::pushImageMask. Anyway, both my patches don't solve the similar issue of Bug 198746. There is one more bug around us. Created attachment 371838 [details]
Patch
Comment on attachment 371838 [details]
Patch
Thanks!
> I created another patch, but this patch solves only position issues, but > blending issues. Yeah, that makes sense so the original surface is properly painted into the new group. > I don't know why this patch doesn't work. > It seems that we should remove the blit in despite of the comment in > PlatformContextCairo::pushImageMask. I don't think we can remove the blit, as it would change the order in which the elements are painted. Imagine that we are painting object A, when define an image mask, and then paint B and C. With the current blitting, A will be copied into the new group, B will be painted over A and then C will be painted over the result. And then the new group will be painted into the real target. But without the blitting the new group will be empty, so B will be painted on top of nothing, then C on top of B and then the group will be painted in the real target on top of A. That can change the final result. > Anyway, both my patches don't solve the similar issue of Bug 198746. There > is one more bug around us. Yes, there's something we're missing. Let me debug that as well. There might be things still broken, but unless we find them, this patch is fixing more things than what it breaks, so let's land it and continue working on it on top. *** Bug 169001 has been marked as a duplicate of this bug. *** Comment on attachment 371838 [details] Patch Clearing flags on attachment: 371838 Committed r246309: <https://trac.webkit.org/changeset/246309> All reviewed patches have been landed. Closing bug. Amazing, thanks Fujii! Can I ask: how does this relate to https://gitlab.freedesktop.org/cairo/cairo/issues/297 (the GitHub trapezoid bug)? Does it obsolete that issue, or is that a separate problem? (In reply to Michael Catanzaro from comment #26) > Amazing, thanks Fujii! > > Can I ask: how does this relate to > https://gitlab.freedesktop.org/cairo/cairo/issues/297 (the GitHub trapezoid > bug)? Does it obsolete that issue, or is that a separate problem? I think it's unrelated, it was never fixed and it's still broken. svg/clip-path/clip-opacity.html is failing in GTK and WPE. https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/10687 https://build.webkit.org/builders/WPE%20Linux%2064-bit%20Release%20%28Tests%29/builds/13921 Created attachment 371917 [details] test gardening patch I'd like to mark it as ImageOnlyFailure of Bug 168426 if nobody objects. Bug 168426 – [GTK] Some reftest fail with only one or two pixel differences in diff image (In reply to Fujii Hironori from comment #29) > Created attachment 371917 [details] > test gardening patch > > I'd like to mark it as ImageOnlyFailure of Bug 168426 if nobody objects. > > Bug 168426 – [GTK] Some reftest fail with only one or two pixel > differences in diff image Sure, feel free to push it unreviewed. (In reply to Carlos Garcia Campos from comment #30) > Sure, feel free to push it unreviewed. Thanks. Committed r246349: <https://trac.webkit.org/changeset/246349> The new test svg/clip-path/clip-hidpi.svg Added in https://trac.webkit.org/changeset/246350/webkit is timing out. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fclip-path%2Fclip-hidpi.svg This test is effecting Mac wk1 (In reply to Truitt Savell from comment #32) > The new test svg/clip-path/clip-hidpi.svg > > Added in https://trac.webkit.org/changeset/246350/webkit > > is timing out. History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=svg%2Fclip-path%2Fclip-hidpi.svg > > This test is effecting Mac wk1 Oh, that's bug #198746. mac-wk2 was green and I assumed it would be the same for wk1. I have no idea why it times out in wk1, can it be just skipped in wk1? (In reply to Carlos Garcia Campos from comment #33) > Oh, that's bug #198746. mac-wk2 was green and I assumed it would be the same > for wk1. I have no idea why it times out in wk1, can it be just skipped in > wk1? Sorry I posted on the wrong bug, I just followed the bug url from https://trac.webkit.org/changeset/246350/webkit Will move to appropriate bug. (In reply to Truitt Savell from comment #34) > (In reply to Carlos Garcia Campos from comment #33) > > > Oh, that's bug #198746. mac-wk2 was green and I assumed it would be the same > > for wk1. I have no idea why it times out in wk1, can it be just skipped in > > wk1? > > Sorry I posted on the wrong bug, I just followed the bug url from > https://trac.webkit.org/changeset/246350/webkit > > Will move to appropriate bug. Oops, I guess I used the wrong bug number when generating the changelog entries :-P Reverted r246350 for reason: r246350 Introduced a failing and timing out test svg/clip-path/clip-hidpi.svg Committed r246354: <https://trac.webkit.org/changeset/246354> Well this got very confused by the wrong changelog entry. I'm going to correct both changelog entries. The patch that got rolled out was from bug #198746, not this bug. Closing again. Committed r246361: <https://trac.webkit.org/changeset/246361> Also committed r246362: <https://trac.webkit.org/changeset/246362> |
