Hi,
(In reply to comment #0)
> see the attached demo, both text snippets should have the same gradient, color
Can you maybe post a screenshot of how it looks for you? On my trunk build both texts look the same.
Cheers,
Rob.
Created attachment 101836[details]
Screenshot of the issue
This is the build I just made from a fresh checkout of the trunk.
I'm on Ubuntu x86_64.
The gradient is very sharp by design, that's not the problem.
For CG the result looks like in FF. I expect a problem with the Cairo port. I'm not sure if I reviewed a patch of Martin or Alex last month that should fix transformations on gradients and patterns for texts. Maybe this special case is not covered. Alex, Martin can you look at this please?
(In reply to comment #4)
> For CG the result looks like in FF. I expect a problem with the Cairo port. I'm not sure if I reviewed a patch of Martin or Alex last month that should fix transformations on gradients and patterns for texts. Maybe this special case is not covered. Alex, Martin can you look at this please?
This is broken for me on both Chromium and WebKitGTK+. :/
Preliminary investigation shows that Skia and Cairo share the same issue and the underlying reason might be an incorrectly computed bounding box for the scaled text. Filling rectangles with the same fill works fine.
Created attachment 145779[details]
Alternative test case
Another text case showing that rectangles work okay, text doesn't. For each line of text, the gradient should match the one in the rectangle, but it ends too early, or in other words, the gradient is horizontally downscaled/squeezed.
The reason is in SVGInlineTextBox::paintTextWithShadows.
This method calls prepareGraphicsContextForTextPainting() which in turns acquires the fill resource which leads to creating a PlatformGradient and setting a gradient transformation matrix that converts between the gradient space and the user space.
However, before drawing the text itself, another "hidden" CTM applied is to the Graphics Context to scale the text into place:
float scalingFactor = textRenderer->scalingFactor();
[...]
newTransform.scale(1 / scalingFactor);
normalizeTransform(newTransform);
context->setCTM(newTransform);
[...]
scaledFont.drawText(context, textRun, textOrigin + extraOffset, startPosition, endPosition);
and then the painting/filling resource is released.
The issue is that the fill gradient doesn't know anything about the additional CTM that's applied to the graphics context. So the gradient is incorrectly scaled down. If I disable the scalingFactor in the function for debugging purposes, the text becomes oversized, but the gradient is correct.
Since I am not familiar with this code (yet), suggestions on how to fix this are very welcome. It seems very tricky to access the Gradient information here to modify its transformation matrix, but it also seems tricky to modify the text scaling code to not use the additional transformation.
One experimental fix is to enable the CG special path for the non-CG ports as well, but I think I have an idea how to fix the gradient transformation matrix.
Created attachment 145993[details]
Equivalent case for patterns
Another test cases which shows that patterns don't scale correctly either - also due to removing the context transform when drawing the text.
Created attachment 146488[details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #10)
> I think we would need to get rid of messing with the transformation matrix of the context when drawing the text - any comments, Nikolas, Dirk?
Hey Dominik,
thanks a lot for your analysis. Unfortunately we can't get rid of the transformation matrix "messing" - it's a special property of SVG text, that's not requires as-is for HTML rendering.
Consider a SVG document with a viewBox (defining the local coordinate system of the document):
<svg width="1000" height="1000" viewBox="0 0 1 1">
<text y="0.1" font-size="0.1">TEST</text>
</svg>
The <svg> element induces a transformation: scale(1000, 1000) to the whole document, to scale up from the local coordinate system 0 0 1 1 to 0 0 1000 1000.
When painting the <text> we do NOT want to draw the text at y=0.1, with a font-size of 0.1 because of numerous of reasons:
- small font-sizes like this aren't supported. It would draw using the minimum font size (eg. 8).
- automatic kerning doesn't work correctly, when drawing a font and scaling it up afterwards.
(to get properly kerned glyphs we need to draw using the final on-screen font size)
- ...
So what we basically need to do is: a) remove the scaling from the CTM, b) paint the text using a final on-screen font-size (here: font-size="100"). b) apply the scaling again to the CTM.
We used to use context->concatCTM(scaleTransform.inverse()) paintText(); context->concatCTM(scaleTranssform) in the past, but that turned out to be numerically unstable on 32bit system at least (ask the Gtk guys for our journey ;-) - so we switched to use setCTM() directly, in order to avoid rounding error accumulation due the inversion of the matrix, and the double concatCTM() calls.
To summarize: I think the painting code for SVG text needs to stay as-is (unless you have a better idea on how to solve this properly). We need to find a fix for non-Cg platforms to respect this transformation in pattern/gradient trafos - and I think that's what your last patch aims at. Lemme check it again.
Comment on attachment 146304[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=146304&action=review
Nice patch! A first round of comments, r- mostly because of chromium redness and some suggestions:
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:189
> +AffineTransform RenderSVGResourceContainer::transformOnTextPainting(RenderObject* object, const AffineTransform& resourceTransform)
We shouldn't return AffineTransforms, but instead pass it by reference as argument.
I'm aware the existing transformOnNonScalingStroke() code path does it this way, but it's deprecated and will be changed soon.
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:201
> + AffineTransform ctm;
> + SVGRenderingContext::calculateTransformationToOutermostSVGCoordinateSystem(object, ctm);
> + float scalingFactor = narrowPrecisionToFloat(sqrt((pow(ctm.xScale(), 2) + pow(ctm.yScale(), 2)) / 2));
This could needs to be shared between here and RenderSVGInlineText in a single place.
Maybe SVGRenderingContext is a good place?
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:206
> + AffineTransform transform;
> + transform.scale(scalingFactor);
> + transform *= resourceTransform;
> + return transform;
You should early exit here for scalingFactor==1, and not build a transformation.
> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:171
> +#if !USE(CG)
I'd avoid to sprinkle around USE(CG) blocks. I'd propose to add a shouldTransformResourceOnTextPainting() method, to be used like this:
AffineTransform userspaceTransform = gradientData->userspaceTransform;
if (isPaintingText) {
AffineTranform additionalTextTransform;
if (shouldTransformResourceOnTextPainting(object, additionalTextTransform))
userspaceTransform.multiply(additionalTextTransform); // Eventually the multiplication must be reversed, didn't check it!
}
gradientData->gradient->setGradientSpaceTransform(userspaceTransform);
shouldTransformResourceOnTextPainting() shouldn't know anything about the actual gradient/pattern space transform, it should be dumb and only return the scaling transform for text.
shouldTransformResourceOnTextPainting() can directly return false, for Cg platforms, and true for anyone else.
Sounds better, eh?
> LayoutTests/svg/transforms/transformed-text-fill-pattern.html:32
> + <text class="ahemblock" y="50" fill="url(#hatch)">AAAAA</text>
Very clever testcases! I hope this actually works in the wild :-) (aka. cr-linux EWS that is currently red).
There's another way to exercise the scaling-code path, which is probably easier to test:
foo.svg;
<svg viewBox="0 0 1 1" width="1000" height="1000">
<text y="0.1" font-size="0.1">ABC</text>
</svg>
foo-expected.svg:
<svg width="1000" height="1000">
<text y="100" font-size="100">ABC</text>
</svg>
Ideally this works with any font, as it should render exactly the same, and thus is ideal for reftests.
Thanks Nikolas for the quick and constructive review! Good suggestions.
(In reply to comment #15)
> (From update of attachment 146304[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146304&action=review
>
> Nice patch! A first round of comments, r- mostly because of chromium redness and some suggestions:
>
> > Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:189
> > +AffineTransform RenderSVGResourceContainer::transformOnTextPainting(RenderObject* object, const AffineTransform& resourceTransform)
>
> We shouldn't return AffineTransforms, but instead pass it by reference as argument.
> I'm aware the existing transformOnNonScalingStroke() code path does it this way, but it's deprecated and will be changed soon.
Done, incorporated into the new shouldTransformResourceOnTextPainting function.
>
> > Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:201
> > + AffineTransform ctm;
> > + SVGRenderingContext::calculateTransformationToOutermostSVGCoordinateSystem(object, ctm);
> > + float scalingFactor = narrowPrecisionToFloat(sqrt((pow(ctm.xScale(), 2) + pow(ctm.yScale(), 2)) / 2));
>
> This could needs to be shared between here and RenderSVGInlineText in a single place.
> Maybe SVGRenderingContext is a good place?
Done, factored out into SVGRenderingContext.
> > Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:206
> > + AffineTransform transform;
> > + transform.scale(scalingFactor);
> > + transform *= resourceTransform;
> > + return transform;
>
> You should early exit here for scalingFactor==1, and not build a transformation.
Done, added early exit case.
> > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:171
> > +#if !USE(CG)
>
> I'd avoid to sprinkle around USE(CG) blocks. I'd propose to add a shouldTransformResourceOnTextPainting() method, to be used like this [..]
> Sounds better, eh?
It does, I admittedly felt bad when sprinkling the #ifdef's but didn't feel comfortable changing interfaces either. Thanks for the suggestion.
> > LayoutTests/svg/transforms/transformed-text-fill-pattern.html:32
> > + <text class="ahemblock" y="50" fill="url(#hatch)">AAAAA</text>
>
> Very clever testcases! I hope this actually works in the wild :-) (aka. cr-linux EWS that is currently red).
Tried it for EFL and Qt - and on these they work. Chromium Redness seems unrelated. My assumption is there had been an issue with compositing resulting in a complete failure to output any rendering. (i.e. instead of red/green rectangles what came out was just a black surface.)
> There's another way to exercise the scaling-code path, which is probably easier to test.
Thanks for the additional test method suggestion. If you're okay, I'd still first like to try the reftests since they compare the two different scaling cases, the rectangle one vs. the text one - the case where the scaling is removed from the context vs. the case where the scaling stays in place.
ChangeLogs and some comments updated, rebased to head.
Created attachment 146576[details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 146811[details]
Patch v4, vertical gradient to make Chromium EWS pass.
Attachment 146811[details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12939357
New failing tests:
svg/W3C-SVG-1.1/pservers-grad-08-b.svg
svg/custom/js-late-gradient-and-object-creation.svg
svg/transforms/transformed-text-fill-gradient.html
svg/custom/text-rotated-gradient.svg
Created attachment 146827[details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 146865[details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #26)
> (From update of attachment 146834[details])
> Attachment 146834[details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12948178
>
> New failing tests:
> svg/W3C-SVG-1.1/pservers-grad-08-b.svg
> svg/custom/js-late-gradient-and-object-creation.svg
> svg/custom/text-rotated-gradient.svg
I checked those cases and it looks as if those just require rebaselining due to scaled gradients having slightly updated shades.
Could you do another round of review, Nikolas? As mentioned, I've adressed your feedback from the previous round.
Comment on attachment 146834[details]
Patch v5, gradient test case simplified.
View in context: https://bugs.webkit.org/attachment.cgi?id=146834&action=review
Sorry, seems I forgot to submit them, I actually reviewed this yesterday :(
Anyhow, r=me, but please update platform/chromium/test_expectations.txt so that cr-linux doesn't turn red of you land this.
Mark the failing tests as IMAGE IMAGE+TEXT TEXT etc..
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:27
> +#if !USE(CG)
> +#include....
I think you can always include the file.
> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:183
> +#if USE(CG)
> + UNUSED_PARAM(object);
> + UNUSED_PARAM(resourceTransform);
> + return false;
> +#else
> + ASSERT(object);
Use ASSERT_UNUSED(object, object) to avoid branching UNUSED_PARAM vs. ASSERT for 'object'.
Comment on attachment 147516[details]
Patch v6, remaining comments addressed, TestExpectations for Chromium edited
Please fill reviewer in Source/WebCore/ChangeLog and LayoutTests/ChangeLog.
2011-07-21 11:39 PDT, pogon
2011-07-24 15:22 PDT, pogon
2012-06-05 06:25 PDT, Dominik Röttsches (drott)
2012-06-06 05:03 PDT, Dominik Röttsches (drott)
2012-06-07 09:24 PDT, Dominik Röttsches (drott)
2012-06-07 23:20 PDT, WebKit Review Bot
2012-06-08 03:12 PDT, Dominik Röttsches (drott)
2012-06-08 04:06 PDT, Dominik Röttsches (drott)
2012-06-08 08:29 PDT, WebKit Review Bot
2012-06-11 01:47 PDT, Dominik Röttsches (drott)
2012-06-11 04:21 PDT, WebKit Review Bot
2012-06-11 05:01 PDT, Dominik Röttsches (drott)
2012-06-11 09:24 PDT, WebKit Review Bot
2012-06-14 01:45 PDT, Dominik Röttsches (drott)
2012-06-14 03:13 PDT, Dominik Röttsches (drott)