Bug 53378 – [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

svg/clip-path/clip-in-mask.svg has been failing on Windows and Linux.  In particular, it doesn't have the white mark inside the black circle.

See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fclip-path%2Fclip-in-mask.svg&showLargeExpectations=true&showExpectations=true&master=ChromiumWebkit

[Ryosuke Niwa]

Similar issues in
svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg
svg/clip-path/deep-nested-clip-in-mask-panning.svg
svg/clip-path/deep-nested-clip-in-mask.svg
svg/clip-path/nested-clip-in-mask-image-based-clipping.svg
svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg
svg/clip-path/nested-clip-in-mask-path-based-clipping.svg

[Dirk Schulze]

The reason is most likely the implementation of GraphicsContext::clipToImageBuffer for Skia.

[Branimir Lambov]

Created attachment 118212 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 118212 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux
> +        https://bugs.webkit.org/show_bug.cgi?id=53378
> +
> +        Reviewed by NOBODY (OOPS!).

These two should appear before the description.

> LayoutTests/ChangeLog:9
> +        [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux
> +        https://bugs.webkit.org/show_bug.cgi?id=53378
> +
> +        Reviewed by NOBODY (OOPS!).

Ditto.

[WebKit Review Bot]

Comment on attachment 118212 [details]
Patch

Attachment 118212 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10752238

New failing tests:
svg/clip-path/nested-clip-in-mask-path-based-clipping.svg
svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg
svg/clip-path/deep-nested-clip-in-mask-panning.svg
svg/clip-path/nested-clip-in-mask-image-based-clipping.svg
svg/clip-path/clip-in-mask.svg
svg/batik/text/textProperties.svg
svg/W3C-SVG-1.1/filters-example-01-b.svg
svg/clip-path/deep-nested-clip-in-mask.svg
svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg

[Branimir Lambov]

Created attachment 118402 [details]
Patch

[Branimir Lambov]

(In reply to comment #4)
> (From update of attachment 118212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118212&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux
> > +        https://bugs.webkit.org/show_bug.cgi?id=53378
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> These two should appear before the description.
> 

Done.
> > LayoutTests/ChangeLog:9
> > +        [chromium] svg/clip-path/clip-in-mask.svg fails on Windows and Linux
> > +        https://bugs.webkit.org/show_bug.cgi?id=53378
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Ditto.

Done.

[Branimir Lambov]

(In reply to comment #5)
> (From update of attachment 118212 [details])
> Attachment 118212 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10752238
> 
> New failing tests:
> svg/clip-path/nested-clip-in-mask-path-based-clipping.svg
> svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg
> svg/clip-path/deep-nested-clip-in-mask-panning.svg
> svg/clip-path/nested-clip-in-mask-image-based-clipping.svg
> svg/clip-path/clip-in-mask.svg
> svg/batik/text/textProperties.svg
> svg/W3C-SVG-1.1/filters-example-01-b.svg
> svg/clip-path/deep-nested-clip-in-mask.svg
> svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg

I intend to run webkit-patch rebaseline on these tests after the patch is landed.

[James Robinson]

(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 118212 [details] [details])
> > Attachment 118212 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/10752238
> > 
> > New failing tests:
> > svg/clip-path/nested-clip-in-mask-path-based-clipping.svg
> > svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg
> > svg/clip-path/deep-nested-clip-in-mask-panning.svg
> > svg/clip-path/nested-clip-in-mask-image-based-clipping.svg
> > svg/clip-path/clip-in-mask.svg
> > svg/batik/text/textProperties.svg
> > svg/W3C-SVG-1.1/filters-example-01-b.svg
> > svg/clip-path/deep-nested-clip-in-mask.svg
> > svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg
> 
> I intend to run webkit-patch rebaseline on these tests after the patch is landed.

You need to mark these as expected failures in test_expectations.txt in that case.

[Ryosuke Niwa]

(In reply to comment #9)
> You need to mark these as expected failures in test_expectations.txt in that case.

I think it's okay not to include it in the patch since doing so will likely cause a patch conflict.

[James Robinson]

I disagree, if you put the new lines somewhere other than the very end of test_expectations.txt the odds of a patch conflict are low. Landing without the test_expectations.txt is just giving the gardener an avoidable red tree and creating a lot of extra work.

[Ryosuke Niwa]

(In reply to comment #11)
> I disagree, if you put the new lines somewhere other than the very end of test_expectations.txt the odds of a patch conflict are low. Landing without the test_expectations.txt is just giving the gardener an avoidable red tree and creating a lot of extra work.

The author can land & add expectations in one patch when he/she lands a patch. In my experience, adding per-file expectation for test_expectations.txt in a patch posted on Bugzilla causes nothing but trouble because there is a big delay between the time you post the patch on Bugzilla and it gets landed. It's much better to add lines before landing the patch after the patch has been reviewed.

[Ryosuke Niwa]

Comment on attachment 118402 [details]
Patch

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

> LayoutTests/platform/chromium/test_expectations.txt:-842
> -// New in WK r65880, some probably just need rebaselining, others have real diffs
> -BUGWK53378 LINUX WIN : svg/clip-path/clip-in-mask.svg = IMAGE
> -BUGWK53378 LINUX WIN : svg/clip-path/deep-nested-clip-in-mask-different-unitTypes.svg = IMAGE+TEXT
> -BUGWK53378 LINUX WIN : svg/clip-path/deep-nested-clip-in-mask-panning.svg = IMAGE+TEXT
> -BUGWK53378 LINUX WIN : svg/clip-path/deep-nested-clip-in-mask.svg = IMAGE+TEXT
> -BUGWK53378 LINUX WIN : svg/clip-path/nested-clip-in-mask-image-based-clipping.svg = IMAGE
> -BUGWK53378 LINUX WIN : svg/clip-path/nested-clip-in-mask-path-and-image-based-clipping.svg = IMAGE
> -BUGWK53378 LINUX WIN : svg/clip-path/nested-clip-in-mask-path-based-clipping.svg = IMAGE
> -// Rebaselined in WK r65874, Linux has missing lines
> -

Having said that, it seems bad to remove these lines pre-emptively. You should land this patch and then remove these lines as you confirm your patch fixed the bug.

[Branimir Lambov]

Created attachment 118554 [details]
Patch

[Tony Chang]

Comment on attachment 118554 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Fixes a problem in applying clip layers on Skia that was causing
> +        all complex clipping (including text and/or masks) to fail.

Can you explain what the bug was and how this fixes it?  I don't know this code very well and it's hard for me to tell if the new code is correct.

[Branimir Lambov]

Comment on attachment 118554 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        all complex clipping (including text and/or masks) to fail.
> 
> Can you explain what the bug was and how this fixes it?  I don't know this code very well and it's hard for me to tell if the new code is correct.

The bug was in Skia's clipping code's handling of coordinate transformations. The method beginLayerClippedToImage was taking rectangle coordinates in one local coordinate space, but it was applying them in a different one because of the delay between the time it is called and the actual application occurs in applyClipFromImage. The fix translates the coordinates passed to beginLayerClippedToImage to absolute ones, so that they are not affected by any change in the transform matrix, and makes sure that applyClipFromImage clears the matrix before applying the clip layer to correctly apply the absolute coordinates.

[Tony Chang]

Comment on attachment 118554 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        all complex clipping (including text and/or masks) to fail.
>> 
>> Can you explain what the bug was and how this fixes it?  I don't know this code very well and it's hard for me to tell if the new code is correct.
> 
> The bug was in Skia's clipping code's handling of coordinate transformations. The method beginLayerClippedToImage was taking rectangle coordinates in one local coordinate space, but it was applying them in a different one because of the delay between the time it is called and the actual application occurs in applyClipFromImage. The fix translates the coordinates passed to beginLayerClippedToImage to absolute ones, so that they are not affected by any change in the transform matrix, and makes sure that applyClipFromImage clears the matrix before applying the clip layer to correctly apply the absolute coordinates.

Please include this in the ChangeLog, otherwise, this patch seems fine.

[Branimir Lambov]

Created attachment 118641 [details]
Patch

[WebKit Review Bot]

Comment on attachment 118641 [details]
Patch

Clearing flags on attachment: 118641

Committed r103091: <http://trac.webkit.org/changeset/103091>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Committed r103099: <http://trac.webkit.org/changeset/103099>

[Adrienne Walker]

Committed r103111: <http://trac.webkit.org/changeset/103111>

[Stephen Chenney]

Committed r114407: <http://trac.webkit.org/changeset/114407>