Bug 64966 – [svg] SVGResources applied to Text with Incorrect Transformations in non-CG Implementations

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

see the attached demo, both text snippets should have the same gradient, color

Created attachment 101614 [details]
Demo for wrong gradient on transformed text

[Rob Buis]

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.

[Dirk Schulze]

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?

[Martin Robinson]

(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+. :/

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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?

[Dominik Röttsches (drott)]

Created attachment 146304 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146304 [details]
Patch

Attachment 146304 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12924201

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/accelerated-drawing/svg-filters.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
platform/chromium/compositing/lost-compositor-context-permanently.html
http/tests/media/video-play-progress.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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.

[Dominik Röttsches (drott)]

Created attachment 146530 [details]
Patch v2, review comments addressed.

[Dominik Röttsches (drott)]

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.

[Build Bot]

Comment on attachment 146530 [details]
Patch v2, review comments addressed.

Attachment 146530 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12917467

[Dominik Röttsches (drott)]

Created attachment 146535 [details]
Patch v3, review comments addressed. Mac EWS buildfix.

[WebKit Review Bot]

Comment on attachment 146535 [details]
Patch v3, review comments addressed. Mac EWS buildfix.

Attachment 146535 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12929004

New failing tests:
svg/transforms/transformed-text-fill-gradient.html
svg/custom/text-rotated-gradient.svg

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

Created attachment 146811 [details]
Patch v4, vertical gradient to make Chromium EWS pass.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

Created attachment 146834 [details]
Patch v5, gradient test case simplified.

[WebKit Review Bot]

Comment on attachment 146834 [details]
Patch v5, gradient test case simplified.

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

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

CC'ed Dominic Cooney, Chromium WebKit gardener.

[Dominik Röttsches (drott)]

Dirk, do you think, you could give r+ on this one? I can't reach Nikoals in two, three days.

[Nikolas Zimmermann]

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

[Dominik Röttsches (drott)]

Created attachment 147516 [details]
Patch v6, remaining comments addressed, TestExpectations for Chromium edited

[Grzegorz Czajkowski]

Comment on attachment 147516 [details]
Patch v6, remaining comments addressed, TestExpectations for Chromium edited

Please fill reviewer in Source/WebCore/ChangeLog and LayoutTests/ChangeLog.

[Dominik Röttsches (drott)]

Created attachment 147541 [details]
Patch v7, reviewer line fixed.

[WebKit Review Bot]

Comment on attachment 147541 [details]
Patch v7, reviewer line fixed.

Clearing flags on attachment: 147541

Committed r120314: <http://trac.webkit.org/changeset/120314>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen Chenney]

Reopening just for Chromium test expectations updates.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fcustom%2Fjs-late-gradient-creation.svg%2Csvg%2Fcustom%2Fjs-late-pattern-creation.svg

[Stephen Chenney]

Committed r120319: <http://trac.webkit.org/changeset/120319>

[Stephen Chenney]

This is causing crashes:
  svg/batik/text/textDecoration.svg = CRASH
  svg/batik/text/textEffect3.svg = CRASH

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fbatik%2Ftext%2FtextDecoration.svg%20%20svg%2Fbatik%2Ftext%2FtextEffect3.svg

Stack example:
STDERR: ASSERTION FAILED: object->isSVGText() || object->isSVGTextPath()
STDERR: ../../third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp(181) : static bool WebCore::RenderSVGResourceContainer::shouldTransformOnTextPainting(WebCore::RenderObject *, WebCore::AffineTransform &)
STDERR: 1   0x1034b3c
STDERR: 2   0x1042e9c
STDERR: 3   0x104382c
STDERR: 4   0x10c38eb
STDERR: 5   0x10c3ba3
STDERR: 6   0x10c46e4
STDERR: 7   0x10c3486
STDERR: 8   0x10c2fb1
STDERR: 9   0x10c0a2f
STDERR: 10  0x109151b
STDERR: 11  0x21d5e6b
STDERR: 12  0x20cc12f
STDERR: 13  0x20ccc03
STDERR: 14  0x20ca338
STDERR: 15  0x108a5d0
STDERR: 16  0x1082700
STDERR: [1196:1196:90966244269:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x6460ae]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x64dde4]
STDERR: 	0x7fbc5740daf0
STDERR: 	WebCore::RenderSVGResourceContainer::shouldTransformOnTextPainting() [0x1034b4b]
STDERR: 	WebCore::RenderSVGResourcePattern::applyResource() [0x1042e9c]
STDERR: 	WebCore::RenderSVGResourcePattern::applyResource() [0x104382c]
STDERR: 	WebCore::SVGInlineTextBox::acquirePaintingResource() [0x10c38eb]
STDERR: 	WebCore::SVGInlineTextBox::prepareGraphicsContextForTextPainting() [0x10c3ba3]
STDERR: 	WebCore::SVGInlineTextBox::paintTextWithShadows() [0x10c46e4]
STDERR: 	WebCore::SVGInlineTextBox::paintText() [0x10c3486]
STDERR: 	WebCore::SVGInlineTextBox::paint() [0x10c2fb1]
STDERR: 	WebCore::SVGInlineFlowBox::paint() [0x10c0a2f]
STDERR: 	WebCore::SVGRootInlineBox::paint() [0x109151b]
STDERR: 	WebCore::RenderLineBoxList::paint() [0x21d5e6b]
STDERR: 	WebCore::RenderBlock::paintContents() [0x20cc12f]
STDERR: 	WebCore::RenderBlock::paintObject() [0x20ccc03]
STDERR: 	WebCore::RenderBlock::paint() [0x20ca338]
STDERR: 	WebCore::RenderSVGText::paint() [0x108a5d0]
STDERR: 	WebCore::RenderSVGContainer::paint() [0x1082700]

[Stephen Chenney]

Committed r120343: <http://trac.webkit.org/changeset/120343>

[Stephen Chenney]

Please remove the crash expectations when the crash is fixed.

[Dominik Röttsches (drott)]

Crash handled in bug 89104.

[Dominik Röttsches (drott)]

Fixed in http://trac.webkit.org/changeset/120706