Bug 95197 - Incorrect large-area clipping
Summary: Incorrect large-area clipping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Florin Malita
URL:
Keywords:
Depends on: 95432 95431
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-28 07:10 PDT by Florin Malita
Modified: 2012-08-30 00:52 PDT (History)
7 users (show)

See Also:


Attachments
Incorrect clipping (522 bytes, image/svg+xml)
2012-08-28 07:10 PDT, Florin Malita
no flags Details
Incorrect masking (445 bytes, image/svg+xml)
2012-08-28 07:11 PDT, Florin Malita
no flags Details
Patch (4.74 KB, patch)
2012-08-28 08:21 PDT, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 :)