Bug 198701 - [cairo][SVG] Putting multiple path elements in clippath causes rendering artifacts
Summary: [cairo][SVG] Putting multiple path elements in clippath causes rendering arti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
: 169001 (view as bug list)
Depends on:
Blocks: 169001
  Show dependency treegraph
 
Reported: 2019-06-09 20:14 PDT by Fujii Hironori
Modified: 2019-06-12 10:26 PDT (History)
8 users (show)

See Also:


Attachments
clip-path.html (696 bytes, text/html)
2019-06-09 20:15 PDT, Fujii Hironori
no flags Details
clip-path-2.html (691 bytes, text/html)
2019-06-09 20:15 PDT, Fujii Hironori
no flags Details
clip-path-2.html (699 bytes, text/html)
2019-06-09 20:19 PDT, Fujii Hironori
no flags Details
clip-path-3.html (768 bytes, text/html)
2019-06-09 22:04 PDT, Fujii Hironori
no flags Details
WIP patch (1002 bytes, patch)
2019-06-09 22:47 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
a test case of clip-path and opacity (469 bytes, text/html)
2019-06-10 22:09 PDT, Fujii Hironori
no flags Details
another test case of clip-path and opacity (421 bytes, text/html)
2019-06-10 22:11 PDT, Fujii Hironori
no flags Details
test case of clip-path and opcity comparing simgle and multiple clipper objects (876 bytes, text/html)
2019-06-10 22:32 PDT, Fujii Hironori
no flags Details
patch solving only position issues (1.10 KB, patch)
2019-06-11 02:43 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (11.95 KB, patch)
2019-06-11 04:04 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
test gardening patch (2.21 KB, patch)
2019-06-11 20:44 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-06-09 20:14:48 PDT
[cairo][SVG] multiple clip-path causes rendering artifacts
Comment 1 Fujii Hironori 2019-06-09 20:15:06 PDT
Created attachment 371716 [details]
clip-path.html
Comment 2 Fujii Hironori 2019-06-09 20:15:24 PDT
Created attachment 371717 [details]
clip-path-2.html
Comment 3 Fujii Hironori 2019-06-09 20:19:27 PDT
Created attachment 371718 [details]
clip-path-2.html
Comment 4 Fujii Hironori 2019-06-09 22:04:27 PDT
Created attachment 371720 [details]
clip-path-3.html
Comment 5 Fujii Hironori 2019-06-09 22:47:49 PDT
Created attachment 371722 [details]
WIP patch

I confirmed this patch solves this issue and YouTube's volume button issue (Bug 169001).
Comment 6 Carlos Garcia Campos 2019-06-10 07:58:08 PDT
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
Comment 7 Fujii Hironori 2019-06-10 20:11:08 PDT
Thanks for testing.
I'm neither a graphics expert, and don't know the reason. 
I'm guessing it's for alpha brending order.
Comment 8 Fujii Hironori 2019-06-10 22:09:46 PDT
Created attachment 371821 [details]
a test case of clip-path and opacity
Comment 9 Fujii Hironori 2019-06-10 22:11:00 PDT
Created attachment 371822 [details]
another test case of clip-path and opacity
Comment 10 Fujii Hironori 2019-06-10 22:32:20 PDT
Created attachment 371824 [details]
test case of clip-path and opcity comparing simgle and multiple clipper objects
Comment 11 Miguel Gomez 2019-06-11 00:19:24 PDT
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.
Comment 12 Carlos Garcia Campos 2019-06-11 00:39:01 PDT
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.
Comment 13 Miguel Gomez 2019-06-11 01:26:27 PDT
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.
Comment 14 Carlos Garcia Campos 2019-06-11 01:40:19 PDT
(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.
Comment 15 Carlos Garcia Campos 2019-06-11 01:41:43 PDT
And that comes from https://bugs.webkit.org/show_bug.cgi?id=23526
Comment 16 Carlos Garcia Campos 2019-06-11 01:44:18 PDT
I can confirm fast/hidpi/hidpi-long-page-with-inset-element.html still passes with this patch,
Comment 17 Fujii Hironori 2019-06-11 02:43:52 PDT
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.
Comment 18 Fujii Hironori 2019-06-11 04:04:52 PDT
Created attachment 371838 [details]
Patch
Comment 19 Carlos Garcia Campos 2019-06-11 04:19:09 PDT
Comment on attachment 371838 [details]
Patch

Thanks!
Comment 20 Miguel Gomez 2019-06-11 04:30:44 PDT
> 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.
Comment 21 Carlos Garcia Campos 2019-06-11 05:17:47 PDT
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.
Comment 22 Carlos Garcia Campos 2019-06-11 05:19:21 PDT
*** Bug 169001 has been marked as a duplicate of this bug. ***
Comment 23 WebKit Commit Bot 2019-06-11 05:50:56 PDT
Comment on attachment 371838 [details]
Patch

Clearing flags on attachment: 371838

Committed r246309: <https://trac.webkit.org/changeset/246309>
Comment 24 WebKit Commit Bot 2019-06-11 05:50:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2019-06-11 05:52:20 PDT
<rdar://problem/51620347>
Comment 26 Michael Catanzaro 2019-06-11 06:12:02 PDT
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?
Comment 27 Carlos Garcia Campos 2019-06-11 07:21:38 PDT
(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.
Comment 29 Fujii Hironori 2019-06-11 20:44:43 PDT
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
Comment 30 Carlos Garcia Campos 2019-06-12 02:58:06 PDT
(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.
Comment 31 Fujii Hironori 2019-06-12 03:41:02 PDT
(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>
Comment 32 Truitt Savell 2019-06-12 08:10:42 PDT
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
Comment 33 Carlos Garcia Campos 2019-06-12 08:44:27 PDT
(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?
Comment 34 Truitt Savell 2019-06-12 08:57:06 PDT
(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.
Comment 35 Carlos Garcia Campos 2019-06-12 09:00:05 PDT
(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
Comment 36 Truitt Savell 2019-06-12 09:21:24 PDT
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>
Comment 37 Michael Catanzaro 2019-06-12 10:17:35 PDT
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.
Comment 38 Michael Catanzaro 2019-06-12 10:22:31 PDT
Committed r246361: <https://trac.webkit.org/changeset/246361>
Comment 39 Michael Catanzaro 2019-06-12 10:26:42 PDT
Also committed r246362: <https://trac.webkit.org/changeset/246362>