Bug 21205 - Make SVGPattern platform independent
Summary: Make SVGPattern platform independent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
: 23275 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-09-28 23:33 PDT by Dirk Schulze
Modified: 2009-01-21 23:22 PST (History)
1 user (show)

See Also:


Attachments
SVGPattern cleanUp (17.65 KB, patch)
2008-09-28 23:39 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVG-pattern (11.73 KB, patch)
2008-10-13 12:08 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVGPattern (16.28 KB, patch)
2008-10-16 12:50 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVGPattern (14.24 KB, patch)
2008-10-18 06:27 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
stroked-pattern (265.59 KB, image/png)
2008-10-18 10:32 PDT, Dirk Schulze
no flags Details
SVGPattern (14.54 KB, patch)
2008-10-19 04:16 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVGPattern (14.89 KB, patch)
2008-10-21 13:08 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
stroked-pattern-with-big-image (990 bytes, image/svg+xml)
2008-10-21 13:18 PDT, Dirk Schulze
no flags Details
SVGPattern platform independent (26.81 KB, patch)
2008-11-11 12:35 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
SVGPattern platform independent (25.21 KB, patch)
2008-11-15 11:21 PST, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff
update of patch (168.76 KB, patch)
2009-01-16 08:44 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
update of patch (167.88 KB, patch)
2009-01-16 11:41 PST, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2008-09-28 23:33:56 PDT
We should make use of Patterns in the GraphicsContext and make SVGPattern platform independent.
Comment 1 Dirk Schulze 2008-09-28 23:39:29 PDT
Created attachment 23901 [details]
SVGPattern cleanUp

This is a first concept and works for Qt and Cairo. It should work for Cg as well, but I can't test it. That's why I bounded it to Cairo and Qt.

To be fixed: I had to change setStrokePattern() and setFillPattern() to give over a transform matrix. This matrix is saved in the state of the context.
We should avoid this with the use of concatCTM().
Comment 2 Dirk Schulze 2008-10-13 12:08:05 PDT
Created attachment 24320 [details]
SVG-pattern

This patch needs https://bugs.webkit.org/show_bug.cgi?id=21555 to work for cairo. Qt doesn't handle the context like Cg or Cairo. That's why the patch only touch the Cg/Cairo code.
Comment 3 Dirk Schulze 2008-10-15 12:55:13 PDT
this patch fails on LayoutTests/svg/custom/stroked-pattern.svg. You'll see the whole circles.
Comment 4 Dirk Schulze 2008-10-16 12:50:56 PDT
Created attachment 24404 [details]
SVGPattern

This patch solves the problem with the LayoutTest above. 
But it looks a bit different as the pixel test. This is because I used a new imageBuffer to create a cell of the pattern manualy. ImageBuffer's need an IntRect but this falsified the resolution a bit.

Second: The pattern looks the same on the top, like every other line. Don't know if this is a feature or a bug.
Comment 5 Dirk Schulze 2008-10-18 06:27:40 PDT
Created attachment 24489 [details]
SVGPattern

A bit clean up. Only problem now: There are 11,5 circles from right to left instead of 11 and 2 half circles (on the left and right side).
I must test the patch on Cg again.
Comment 6 Dirk Schulze 2008-10-18 10:32:39 PDT
Created attachment 24490 [details]
stroked-pattern

This is the result of this patch on the LayoutTest 2 comments above.
Comment 7 Dirk Schulze 2008-10-19 04:16:36 PDT
Created attachment 24506 [details]
SVGPattern

The problem of the wrong width of a pattern cell is caused by enclosingIntRect().
enclosingIntRect transforms a FloatRect to a IntRect.
But it uses

    int r = static_cast<int>(ceilf(rect.right()));
    int b = static_cast<int>(ceilf(rect.bottom()));

for the transformation of witdth/height from float to int.
That means 15.0 -> 16 and 30.0 -> 31

I cast the width and height my self for the moment.
This solves the the wrong rendering on this LayoutTest.

Last problems: is style->opacity() still needed? Wrong transformations on Qt.
Comment 8 Dirk Schulze 2008-10-21 13:08:19 PDT
Created attachment 24542 [details]
SVGPattern

This fixes Patterns with a very big Image on overflow="visible". Will upload a Testcase for it.

The style->opacity() is set on SVGRenderSupport with the call beginTransparencyLayer(opacity). It should be needless (and now wrong) to set it in SVGPattern again.
Comment 9 Dirk Schulze 2008-10-21 13:18:48 PDT
Created attachment 24543 [details]
stroked-pattern-with-big-image

LayoutTest for big images in patterns, when overflow="visible" is used.
Comment 10 Dirk Schulze 2008-11-11 12:35:16 PST
Created attachment 25051 [details]
SVGPattern platform independent

This is the real platform independent SVGPattern patch. tronical and I found the reason for the wrong drawing on Qt and will fix it soon.
Comment 11 Dirk Schulze 2008-11-15 11:21:51 PST
Created attachment 25188 [details]
SVGPattern platform independent

forgot to remove unnecessary files.
Comment 12 Nikolas Zimmermann 2008-11-21 06:42:28 PST
Comment on attachment 25188 [details]
SVGPattern platform independent

r=me. One mac result needs to be udpated, I'll take care of it afterwards.
Comment 13 Dirk Schulze 2008-11-21 06:51:17 PST
I'll fix a style issue 
for ( ... i--) -> --i 
and run LayoutTests before landing it.
Comment 14 Dirk Schulze 2008-12-03 12:34:00 PST
Fails on CG on filling or stroking texts with patterns. Needs to be fixed before landing.
Comment 15 Eric Seidel (no email) 2008-12-05 16:20:43 PST
Comment on attachment 25188 [details]
SVGPattern platform independent

This may be wrong:

     // Respect local pattern transformation
65      context->concatCTM(patternTransform());
66  
67      // Apply pattern space transformation
68      context->translate(patternBoundaries().x(), patternBoundaries().y());


 113     context->translate(patternBoundaries().x(), patternBoundaries().y());
 114     context->concatCTM(patternTransform());
 115 

I think that the new code looks more correct, but it concerns me that the old CG code was backwards.
Comment 16 Dirk Schulze 2008-12-05 22:08:15 PST
(In reply to comment #15)

> I think that the new code looks more correct, but it concerns me that the old
> CG code was backwards.

I tried it, but makes no difference.
Comment 17 Dirk Schulze 2009-01-15 11:16:32 PST
What about releasing the pattern in Cg? In SVGPattern we release patterns with the teardown. That means _after_ filling/stroking the path or text with the pattern. With the new code, we release the pattern before we fill/stroke the path or text. Can this cause the trouble with texts on Cg?
Comment 18 Dirk Schulze 2009-01-16 02:34:00 PST
Ok. Found the problem for Cg and will update the patch soon.
Comment 19 Dirk Schulze 2009-01-16 08:44:24 PST
Created attachment 26795 [details]
update of patch

This is an update of the original patch. Applier's should be moved from GraphicsContext.cpp to Font*Cg somehow, otherwise we recreate the platformpattern for Cg if we fill/stroke paths.
It is not enough to create the platformpattern only on GraphicsContext.cpp, because we can modify the context after creating the pattern in canvas. This influences the drawing.
But just let fillPath or strokePath create platformpatterns is not enough too: Texts are not affected by the applier-functions. The platformpattern is not created if we draw texts.
Moving the appliers to drawText would be more complex then adding them to setStrokePattern or setFillPattern.
Comment 20 Dirk Schulze 2009-01-16 11:41:17 PST
Created attachment 26801 [details]
update of patch

Doesn't touch GraphicsContext.cpp and avoids creating platformpattern multiple times.
Comment 21 Nikolas Zimmermann 2009-01-20 11:21:50 PST
Comment on attachment 26801 [details]
update of patch

Patch looks excellent, in case there are no regressions, r=me.
Comment 22 Dirk Schulze 2009-01-20 13:08:50 PST
landed in r40063
Comment 23 Dirk Schulze 2009-01-21 23:22:29 PST
*** Bug 23275 has been marked as a duplicate of this bug. ***