Bug 21205 – Make SVGPattern platform independent

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

We should make use of Patterns in the GraphicsContext and make SVGPattern platform independent.

[Dirk Schulze]

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().

[Dirk Schulze]

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.

[Dirk Schulze]

this patch fails on LayoutTests/svg/custom/stroked-pattern.svg. You'll see the whole circles.

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 24490 [details]
stroked-pattern

This is the result of this patch on the LayoutTest 2 comments above.

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 24543 [details]
stroked-pattern-with-big-image

LayoutTest for big images in patterns, when overflow="visible" is used.

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 25188 [details]
SVGPattern platform independent

forgot to remove unnecessary files.

[Nikolas Zimmermann]

Comment on attachment 25188 [details]
SVGPattern platform independent

r=me. One mac result needs to be udpated, I'll take care of it afterwards.

[Dirk Schulze]

I'll fix a style issue 
for ( ... i--) -> --i 
and run LayoutTests before landing it.

[Dirk Schulze]

Fails on CG on filling or stroking texts with patterns. Needs to be fixed before landing.

[Eric Seidel (no email)]

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.

[Dirk Schulze]

(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.

[Dirk Schulze]

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?

[Dirk Schulze]

Ok. Found the problem for Cg and will update the patch soon.

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 26801 [details]
update of patch

Doesn't touch GraphicsContext.cpp and avoids creating platformpattern multiple times.

[Nikolas Zimmermann]

Comment on attachment 26801 [details]
update of patch

Patch looks excellent, in case there are no regressions, r=me.

[Dirk Schulze]

landed in r40063

[Dirk Schulze]

*** Bug 23275 has been marked as a duplicate of this bug. ***