Bug 198701 – [cairo][SVG] Putting multiple path elements in clippath causes rendering artifacts

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Fujii Hironori]

[cairo][SVG] multiple clip-path causes rendering artifacts

[Fujii Hironori]

Created attachment 371716 [details]
clip-path.html

[Fujii Hironori]

Created attachment 371717 [details]
clip-path-2.html

[Fujii Hironori]

Created attachment 371718 [details]
clip-path-2.html

[Fujii Hironori]

Created attachment 371720 [details]
clip-path-3.html

[Fujii Hironori]

Created attachment 371722 [details]
WIP patch

I confirmed this patch solves this issue and YouTube's volume button issue (Bug 169001).

[Carlos Garcia Campos]

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

[Fujii Hironori]

Thanks for testing.
I'm neither a graphics expert, and don't know the reason. 
I'm guessing it's for alpha brending order.

[Fujii Hironori]

Created attachment 371821 [details]
a test case of clip-path and opacity

[Fujii Hironori]

Created attachment 371822 [details]
another test case of clip-path and opacity

[Fujii Hironori]

Created attachment 371824 [details]
test case of clip-path and opcity comparing simgle and multiple clipper objects

[Miguel Gomez]

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.

[Carlos Garcia Campos]

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.

[Miguel Gomez]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

And that comes from https://bugs.webkit.org/show_bug.cgi?id=23526

[Carlos Garcia Campos]

I can confirm fast/hidpi/hidpi-long-page-with-inset-element.html still passes with this patch,

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 371838 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 371838 [details]
Patch

Thanks!

[Miguel Gomez]

> 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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

*** Bug 169001 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 371838 [details]
Patch

Clearing flags on attachment: 371838

Committed r246309: <https://trac.webkit.org/changeset/246309>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51620347>

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

(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.

[Fujii Hironori]

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

[Fujii Hironori]

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

[Carlos Garcia Campos]

(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.

[Fujii Hironori]

(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>

[Truitt Savell]

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

[Carlos Garcia Campos]

(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?

[Truitt Savell]

(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.

[Carlos Garcia Campos]

(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

[Truitt Savell]

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>

[Michael Catanzaro]

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.

[Michael Catanzaro]

Committed r246361: <https://trac.webkit.org/changeset/246361>

[Michael Catanzaro]

Also committed r246362: <https://trac.webkit.org/changeset/246362>