Bug 198746 - [cairo][SVG] If clipPath has multiple elements, clip-path doesn't work with transform
Summary: [cairo][SVG] If clipPath has multiple elements, clip-path doesn't work with t...
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
: 198779 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-06-10 23:11 PDT by Fujii Hironori
Modified: 2019-06-28 00:27 PDT (History)
7 users (show)

See Also:


Attachments
test case (846 bytes, text/html)
2019-06-10 23:11 PDT, Fujii Hironori
no flags Details
WIP (2.98 KB, patch)
2019-06-12 02:25 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP (3.21 KB, patch)
2019-06-12 05:11 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
WIP (3.18 KB, patch)
2019-06-12 05:15 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (8.91 KB, patch)
2019-06-12 06:04 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (21.25 KB, patch)
2019-06-12 19:05 PDT, Fujii Hironori
Hironori.Fujii: commit-queue-
Details | Formatted Diff | Diff
Patch (9.29 KB, patch)
2019-06-12 19:08 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-10 23:11:39 PDT
Created attachment 371826 [details]
test case

[cairo][SVG] If clipPath has multiple elements, clip-path doesn't work with opacity
Comment 1 Carlos Garcia Campos 2019-06-11 07:35:31 PDT
The problem here is the translation (transform="translate(200,0)") in the second case. I've managed to fix this by using a cairo_pattern_t created for the given cairo_image_t and setting a transformation matrix. But that broke other test cases attached to bug #198701, so it needs some more work. It seems the matrix I'm using only works when current transformation matrix doesn't have a translation already. I'll continue investigating tomorrow.
Comment 2 Fujii Hironori 2019-06-11 23:09:50 PDT
My patch in Bug 198779 comment 2 solves this issue only in non high DPI display, but not fixed in high DPI display. Funny.
It seems there is one more coordinate system bug around us.
Comment 3 Carlos Garcia Campos 2019-06-12 02:25:29 PDT
Created attachment 371938 [details]
WIP

This fixes this test case without breaking the ones attached to bug #198701. It makes the code a lot simpler too. HiDPI is a different issue, we need to apply somehow the device scale factor to the mask surface. This patch only needs to include a test. Fujii why are you using html tests instead of svg?
Comment 4 Fujii Hironori 2019-06-12 03:28:04 PDT
Comment on attachment 371938 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=371938&action=review

LGTM

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:86
> +    cairo_matrix_invert(&matrix);

You can remove this cairo_matrix_invert by negating.
    cairo_matrix_init_translate(&matrix, -rect.x(), -rect.y());
Comment 5 Carlos Garcia Campos 2019-06-12 04:45:40 PDT
(In reply to Fujii Hironori from comment #4)
> Comment on attachment 371938 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371938&action=review
> 
> LGTM

It breaks other tests, though. I think I have a new patch that would also fix bug #198779, running the tests now.

> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:86
> > +    cairo_matrix_invert(&matrix);
> 
> You can remove this cairo_matrix_invert by negating.
>     cairo_matrix_init_translate(&matrix, -rect.x(), -rect.y());

Ok, even simpler indeed.
Comment 6 Carlos Garcia Campos 2019-06-12 05:11:24 PDT
Created attachment 371942 [details]
WIP

This patch fixes this test case and also the HiDPI one with no regressions in layout tests. It just needs a couple of new tests.
Comment 7 Carlos Garcia Campos 2019-06-12 05:15:03 PDT
Created attachment 371944 [details]
WIP

I forgot to remove the matrix_invert call
Comment 8 Carlos Garcia Campos 2019-06-12 06:04:50 PDT
Created attachment 371946 [details]
Patch
Comment 9 Carlos Garcia Campos 2019-06-12 06:41:02 PDT
*** Bug 198779 has been marked as a duplicate of this bug. ***
Comment 10 Carlos Garcia Campos 2019-06-12 06:43:43 PDT
Committed r246350: <https://trac.webkit.org/changeset/246350>
Comment 11 Radar WebKit Bug Importer 2019-06-12 06:44:16 PDT
<rdar://problem/51665805>
Comment 12 Truitt Savell 2019-06-12 09:06:45 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. It looks like EWS caught this timeout on Mac wk1 and Mac debug but was not allowed to finish running. It looks like this test should be corrected. Our policies state we should roll this out until a functioning test can be landed with the patch.
Comment 13 Truitt Savell 2019-06-12 09:18:23 PDT
Additional a failure just occurred on Mac Debug wk2

Diff: 
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r246352%20(3028)/svg/clip-path/clip-hidpi-diffs.html
Comment 14 Truitt Savell 2019-06-12 09:22:02 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 15 Ryan Haddad 2019-06-12 09:40:14 PDT
(In reply to Truitt Savell from comment #12)
> 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. It looks like EWS caught this timeout on Mac
> wk1 and Mac debug but was not allowed to finish running. It looks like this
> test should be corrected. Our policies state we should roll this out until a
> functioning test can be landed with the patch.
This timeout was also seen on Win10:
https://build.webkit.org/results/Apple%20Win%2010%20Release%20(Tests)/r246350%20(2551)/results.html
Comment 16 Michael Catanzaro 2019-06-12 10:17:08 PDT
Reopening due to the rollout.
Comment 17 Fujii Hironori 2019-06-12 19:05:39 PDT
Created attachment 372014 [details]
Patch
Comment 18 Fujii Hironori 2019-06-12 19:08:01 PDT
Created attachment 372015 [details]
Patch

* Fixed timing out issue by using ->.
* Fixed the bug id in ChangeLog
Comment 19 WebKit Commit Bot 2019-06-12 21:10:39 PDT
Comment on attachment 372015 [details]
Patch

Clearing flags on attachment: 372015

Committed r246391: <https://trac.webkit.org/changeset/246391>
Comment 20 WebKit Commit Bot 2019-06-12 21:10:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Fujii Hironori 2019-06-12 21:18:04 PDT
Great work! Good job, KaL. I'm still confused by transform matrix hell, but your solution looks consistent.
Comment 22 Fujii Hironori 2019-06-28 00:27:49 PDT
(In reply to Fujii Hironori from comment #18)
> * Fixed timing out issue by using ->.

Oops, my bad. This is a just syntax error. Filed in Bug 199313.