see the attached demo, both text snippets should have the same gradient, color
Created attachment 101614 [details] Demo for wrong gradient on transformed text
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.
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?
Created attachment 146304 [details] Patch
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
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.
Created attachment 146530 [details] Patch v2, review comments addressed.
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.
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
Created attachment 146535 [details] Patch v3, review comments addressed. Mac EWS buildfix.
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
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
Created attachment 146811 [details] Patch v4, vertical gradient to make Chromium EWS pass.
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 146834 [details] Patch v5, gradient test case simplified.
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
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.
CC'ed Dominic Cooney, Chromium WebKit gardener.
Dirk, do you think, you could give r+ on this one? I can't reach Nikoals in two, three days.
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'.
Created attachment 147516 [details] Patch v6, remaining comments addressed, TestExpectations for Chromium edited
Comment on attachment 147516 [details] Patch v6, remaining comments addressed, TestExpectations for Chromium edited Please fill reviewer in Source/WebCore/ChangeLog and LayoutTests/ChangeLog.
Created attachment 147541 [details] Patch v7, reviewer line fixed.
Comment on attachment 147541 [details] Patch v7, reviewer line fixed. Clearing flags on attachment: 147541 Committed r120314: <http://trac.webkit.org/changeset/120314>
All reviewed patches have been landed. Closing bug.
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
Committed r120319: <http://trac.webkit.org/changeset/120319>
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]
Committed r120343: <http://trac.webkit.org/changeset/120343>
Please remove the crash expectations when the crash is fixed.
Crash handled in bug 89104.
Fixed in http://trac.webkit.org/changeset/120706