Bug 95197

Summary: Incorrect large-area clipping
Product: WebKit Reporter: Florin Malita <fmalita>
Component: SVGAssignee: Florin Malita <fmalita>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jberlin, krit, pdr, schenney, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 95431, 95432    
Bug Blocks:    
Attachments:
Description Flags
Incorrect clipping
none
Incorrect masking
none
Patch none

Description Florin Malita 2012-08-28 07:10:07 PDT
Created attachment 160968 [details]
Incorrect clipping

CR issue: http://code.google.com/p/chromium/issues/detail?id=144630

This bug manifests when the following conditions are met:

a) the clipping is handled via masking (forcing it via clip-path on a clip-path in the test)
and
b) the clip area is larger than 4096 (absolute size) in one dimension
and
c) the path repaint rect has a non-zero offset

During mask ImageBuffer allocation the size is limited to 4096x4096 max. This introduces a non-trivial scaling component, which needs to be accounted for in the translation also.

Note that this also affects masking, which uses the came ImageBuffer allocation logic.
Comment 1 Florin Malita 2012-08-28 07:11:57 PDT
Created attachment 160969 [details]
Incorrect masking
Comment 2 Florin Malita 2012-08-28 07:20:43 PDT
(In reply to comment #0)
> During mask ImageBuffer allocation the size is limited to 4096x4096 max. This introduces a non-trivial scaling component, which needs to be accounted for in the translation also.

I think the order of matrix ops in SVGRenderingContext::createImageBuffer() is wrong:

* imageContext->translate()
* imageContext->concatCTM)()
* imageContext->scale()

Since this is a viewport transformation (where the viewport is the image buffer and world coords == abs coords), the scaling transform should be pushed before translation: P' = S(T(P)) = S * T * P

Some quick testing validates this, patch coming soon.
Comment 3 Florin Malita 2012-08-28 08:21:46 PDT
Created attachment 160988 [details]
Patch
Comment 4 Nikolas Zimmermann 2012-08-29 02:55:31 PDT
Comment on attachment 160988 [details]
Patch

r=me, this was obviously wrong.
Comment 5 WebKit Review Bot 2012-08-29 05:24:26 PDT
Comment on attachment 160988 [details]
Patch

Clearing flags on attachment: 160988

Committed r126993: <http://trac.webkit.org/changeset/126993>
Comment 6 WebKit Review Bot 2012-08-29 05:24:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Jessie Berlin 2012-08-29 14:12:10 PDT
This test started failing on Mac at the point it was added:

http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r126993%20(2333)/results.html

Can you please check the results and see if they are expected for Mac? Otherwise I will need to roll this out.
Comment 8 Florin Malita 2012-08-29 14:31:51 PDT
(In reply to comment #7)
> This test started failing on Mac at the point it was added:
> 
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r126993%20(2333)/results.html
> 
> Can you please check the results and see if they are expected for Mac? Otherwise I will need to roll this out.

Yes, the results are good. Unfortunately there's a small subpixel artifact on the edge which makes it different. I'll update it to try to avoid that.
Comment 9 Florin Malita 2012-08-29 15:16:00 PDT
I've tweaked the reftest mask to hopefully take care of it: http://trac.webkit.org/changeset/127056

If that doesn't do it, we may need to switch this test to image results and possibly investigate why that bleeding happens.
Comment 10 Jessie Berlin 2012-08-29 16:38:57 PDT
(In reply to comment #9)
> I've tweaked the reftest mask to hopefully take care of it: http://trac.webkit.org/changeset/127056
> 
> If that doesn't do it, we may need to switch this test to image results and possibly investigate why that bleeding happens.

Looks like r127056 took care of it. Thanks :)