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

Florin Malita
Reported 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.
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
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Florin Malita
Comment 1 2012-08-28 07:11:57 PDT
Created attachment 160969 [details] Incorrect masking
Florin Malita
Comment 2 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.
Florin Malita
Comment 3 2012-08-28 08:21:46 PDT
Created attachment 160988 [details] Patch
Nikolas Zimmermann
Comment 4 2012-08-29 02:55:31 PDT
Comment on attachment 160988 [details] Patch r=me, this was obviously wrong.
WebKit Review Bot
Comment 5 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>
WebKit Review Bot
Comment 6 2012-08-29 05:24:29 PDT
All reviewed patches have been landed. Closing bug.
Jessie Berlin
Comment 7 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.
Florin Malita
Comment 8 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.
Florin Malita
Comment 9 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.
Jessie Berlin
Comment 10 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 :)
Note You need to log in before you can comment on or make changes to this bug.