Bug 47467

Created attachment 70393 [details] Patch

Attachment 70393 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 JavaScriptCore/runtime/StringBuilder.h:27: #ifndef header guard has wrong style, please use: StringBuilder_h [build/header_guard] [5] WebCore/svg/SVGPathStringBuilder.h:27: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Okay, I made up my mind, it needs to be further generalized. TextStream<<(float / double) needs the same logic, in order to get consistent values across platforms / architectures. Insipred by the SVG test skip list on platform/win/Skipped - it lists several non-<path> debug output failures, which are due the TextStream::operator<< usage.

(In reply to comment #3) > Okay, I made up my mind, it needs to be further generalized. TextStream<<(float / double) needs the same logic, in order to get consistent values across platforms / architectures. Insipred by the SVG test skip list on platform/win/Skipped - it lists several non-<path> debug output failures, which are due the TextStream::operator<< usage. So the review flag can be cleared? What about 18994, cant this bug be marked as duplicate?

Comment on attachment 70393 [details] Patch Clearing the r-flag, since it looks like you're working on another solution and we can move this patch out of the pending review list. Reset it, if you think this should get reviewed.

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

I've generalized this, will upload a work-of-concept patch, that unifies the way DRT prints floating point numbers.

Created attachment 114917 [details] Patch v1 New attempt at fixing the rounding problems across the Gtk/Win bots. It fixes all 0.0 vs -0.0 issues, several wrong sizes that are reported in the mac dumps 481x360 -> 480x360. This includes a patch from Zoltan to SVGPreserveAspectRatio::getCTM(), to avoid loosing precision, by using double internally. The AffineTransform that getCTM() generates uses double values, that's why this matters!

Attachment 114917 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/JavaScriptCore/wtf/dtoa/utils.h:202: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.

Thanks a lot for fixing these rounding issues! SVG is not really my area of work but this patch looks good, minus the red win EWS :) Have you tested a GTK Debug build too?

Nope, t(In reply to comment #10) > Thanks a lot for fixing these rounding issues! > SVG is not really my area of work but this patch looks good, minus the red win EWS :) > > Have you tested a GTK Debug build too? Sorry, I didn't mention it earlier. I have not tested with Gtk, and hope you could try that maybe? I have compared the failures mentioned in the master bug report related to gtk to the new baselines, and they match the gtk "failures" now, so I think the problem should be gone, but I didn't check all tests that are skipped only some of them.

Forgot to say, fixing the win build is a matter of exporting the new symbol for String::number - I'll upload a new patch soon, but it won't affect gtk/mac/qt.

Created attachment 114942 [details] Patch v2 Attempt to fix build/style issues.

(In reply to comment #13) > Created an attachment (id=114942) [details] > Patch v2 > > Attempt to fix build/style issues. Philippe was so kind to build and test on Gtk and send me the results. There are still some differences, and I'm trying to find out what I can do next. Stay tuned.

Comment on attachment 114942 [details] Patch v2 Attachment 114942 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10310473 New failing tests: svg/W3C-SVG-1.1/struct-frag-02-t.svg svg/custom/circular-marker-reference-1.svg svg/W3C-SVG-1.1/fonts-elem-03-b.svg svg/W3C-SVG-1.1/fonts-elem-02-t.svg svg/W3C-SVG-1.1/painting-marker-03-f.svg fast/backgrounds/size/contain-and-cover-zoomed.html svg/W3C-SVG-1.1/coords-viewattr-01-b.svg svg/carto.net/scrollbar.svg svg/custom/circular-marker-reference-3.svg media/track/track-webvtt-tc003-newlines.html svg/W3C-SVG-1.1-SE/text-tspan-02-b.svg svg/W3C-SVG-1.1/fonts-elem-01-t.svg svg/custom/circular-marker-reference-2.svg svg/W3C-SVG-1.1/fonts-elem-04-b.svg svg/W3C-SVG-1.1/struct-frag-03-t.svg

Created attachment 115150 [details] Patch v3 Patch v3 won't build on win yet, the String::number() signature changed, and I need to wait for the win build to show me the right symbol name that I can add to the JSC.def file. I inspected the layout test results that Philippe sent me and found out that the most common source of rounding errors between gtk/mac is the dumping of the renderer positions, see here: - RenderSVGPath {path} at (94,162) size 220x37 [stroke={[type=SOLID] [color=#000000] [stroke width=10.00] [miter limit=17.00]}] [data="M 20 110 L 200 120 L 20 130"] + RenderSVGPath {path} at (94,162) size 220x36 [stroke={[type=SOLID] [color=#000000] [stroke width=10.00] [miter limit=17.00]}] [data="M 20 110 L 200 120 L 20 130"] These values come from the absoluteClippedOverflowRect() - which maps the renderers local repaint rect through a bunch of mapRect() calls up to the root renderer. mapRect() is using FloatQuad::boundingBox(), operating on rounded float points. It returns FloatRect(left, top, right - left, bottom - top), where all values are floats. We should rather use doubles then calculate the subtraction, then round to float. Fixing that changes several test results on mac. Lets see how this affects Gtk. Philippe could you build it again and produce a baseline comparison against mac? Quick'n'dirty: mv LayoutTests/platform/gtk/svg gtk-svg-old; mv LayoutTests/platform/mac/svg/ platform/gtk/svg/

(In reply to comment #16) > Lets see how this affects Gtk. Philippe could you build it again and produce a baseline comparison against mac? I got a new baseline from Philipe, this is a progression, but we're not there yet. Still several rounding differences like: - RenderSVGContainer {g} at (16,556) size 76x14 + RenderSVGContainer {g} at (16,556) size 76x15 RenderSVGText {text} at (10,334) size 45x8 contains 1 chunk(s) RenderSVGInlineText {#text} at (0,0) size 45x8 chunk 1 text run 1 at (10.00,340.00) startOffset 0 endOffset 16 width 45.00: "$Revision: 1.7 $" As you can see the container size differs, which is nothing but the union of its children, so how can this difference be explained? What we're dumping in writePositionAndStyle() in SVGRenderTreeAsText, is the "absoluteClippedOverflowRect". static TextStream& writePositionAndStyle(TextStream& ts, const RenderObject& object) { ts << " " << const_cast<RenderObject&>(object).absoluteClippedOverflowRect(); ... absluteClippedOverflowRect() is an IntRect, and thus not affected by the TextStream::operator<<(float) changes I made, so the remaining rounding error must be somewhere else. What happens when we call absoluteClippedOverflowRect() on a RenderSVGContainer? LayoutRect absoluteClippedOverflowRect() const { return clippedOverflowRectForRepaint(0); } RenderSVGContainer inherits following from RenderSVGModelObject: LayoutRect RenderSVGModelObject::clippedOverflowRectForRepaint(RenderBoxModelObject* repaintContainer) const { return SVGRenderSupport::clippedOverflowRectForRepaint(this, repaintContainer); } Here's the relevant code: LayoutRect SVGRenderSupport::clippedOverflowRectForRepaint(const RenderObject* object, RenderBoxModelObject* repaintContainer) { ... // Pass our local paint rect to computeRectForRepaint() which will // map to parent coords and recurse up the parent chain. LayoutRect repaintRect = enclosingLayoutRect(object->repaintRectInLocalCoordinates()); object->computeRectForRepaint(repaintContainer, repaintRect); return repaintRect; } void SVGRenderSupport::computeRectForRepaint(const RenderObject* object, RenderBoxModelObject* repaintContainer, LayoutRect& repaintRect, bool fixed) { ... // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent repaintRect = object->localToParentTransform().mapRect(repaintRect); object->parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed); } The culprit must be clippedOverflowRectForRepaint(). We're using enclosingIntRect on the repaint rect of the <g> (which is the union of the repaint rects of all children, all stored as FloatRects) and then map that through the parent CTM hierarchy up to RenderSVGRoot, via localToParentTransform(), and calling the parents computeRectForRepaint method. This is error-prone, and should be avoided. I'm now trying a computeFloatRectForRepaint method, smilar to nodeAtFloatPoint vs. nodeAtPoint, that uses more precision inside the SVG tree - float vs int). Let's see how that goes :-)

Comment on attachment 115150 [details] Patch v3 Attachment 115150 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10485434 New failing tests: svg/W3C-SVG-1.1/animate-elem-34-t.svg svg/W3C-SVG-1.1/struct-frag-02-t.svg svg/custom/circular-marker-reference-1.svg svg/W3C-SVG-1.1/fonts-elem-03-b.svg svg/W3C-SVG-1.1/fonts-elem-02-t.svg svg/W3C-SVG-1.1/painting-marker-03-f.svg fast/backgrounds/size/contain-and-cover-zoomed.html svg/W3C-SVG-1.1/coords-viewattr-01-b.svg svg/carto.net/scrollbar.svg svg/W3C-SVG-1.1-SE/text-tspan-02-b.svg svg/W3C-SVG-1.1/fonts-elem-01-t.svg svg/W3C-SVG-1.1/fonts-elem-04-b.svg svg/W3C-SVG-1.1/painting-stroke-07-t.svg svg/W3C-SVG-1.1/struct-frag-03-t.svg

Created attachment 115343 [details] Patch v4 The patch is growing: a noticable number of new baselines is needed on Mac, and included within the patch. I hope this fixes the last category of rounding problems remaining on Gtk: container sizes/positions. If you look through the patch you'll see that on Mac, the container positions remained the same compared to the last patch, but the child positions/sizes changed slightly. I hope this will compensate the rounding problem seen on container sizes/positions on Gtk. It need testing. Unfortunately this doesn't fix the win build yet, I'm missing another symbol and have to wait for EWS.

Created attachment 115358 [details] Patch v5 Another iteration, this time concentrating on text. Thanks again to both ossy & phil for building & testing my patches!

Comment on attachment 115358 [details] Patch v5 Attachment 115358 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10484757

Comment on attachment 115358 [details] Patch v5 Attachment 115358 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10473280

Comment on attachment 115358 [details] Patch v5 Attachment 115358 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10484764

Comment on attachment 115358 [details] Patch v5 Attachment 115358 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10497001

Created attachment 115376 [details] Patch v6 Next try.

Comment on attachment 115376 [details] Patch v6 Attachment 115376 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10487737

Comment on attachment 115376 [details] Patch v6 Attachment 115376 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10310976

Comment on attachment 115376 [details] Patch v6 Attachment 115376 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10483745

Created attachment 115442 [details] Patch v7 New patch for phil/ossy. to try.

Created attachment 115543 [details] Patch v8 Thats what you get from touching lots of test results: merging errors. Updated to ToT, I hope it applies now.

Phillppe built it once again, thank you so much. While I'm confident that 32/64 bit problems should be resolve now, there is following problem remaining affecting most gtk svg tests with text: - RenderSVGText {text} at (290,107) size 20x40 contains 1 chunk(s) - RenderSVGInlineText {#text} at (0,0) size 20x40 + RenderSVGText {text} at (290,107) size 20x41 contains 1 chunk(s) + RenderSVGInlineText {#text} at (0,0) size 20x41 chunk 1 text run 1 at (290.00,140.00) startOffset 0 endOffset 1 width 20.00: "0" Just gdb'ing this with phil with a simple reduction: <svg viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" > <text x="10" y="340" font-family="Arial" font-size="6" fill="black">$Revision: 1.4 $</text> </svg> Breaking here: Breakpoint 1, WebCore::writeSVGInlineTextBox (ts=@0x7fff5fbfdf90, textBox=0x108360758, indent=3) at SVGRenderTreeAsText.cpp:410 That's how it looks like on Mac: (gdb) p textBox->textFragments().at(0) $12 = ('WebCore::SVGTextFragment' &) @0x108862000: { characterOffset = 0, metricsListOffset = 0, length = 16, isTextOnPath = false, x = 10, y = 340, width = 45, height = 6.703125, transform = { m_transform = {1, 0, 0, 1, 0, 0} }, lengthAdjustTransform = { m_transform = {1, 0, 0, 1, 0, 0} } } (gdb) p textBox->textRenderer()->style()->fontMetrics() $15 = (const 'WebCore::FontMetrics' &) @0x1078c900c: { m_unitsPerEm = 2048, m_ascent = 5.43164062, m_descent = 1.27148438, m_lineGap = 0.196289062, m_lineSpacing = 6, m_xHeight = 3.11132812 } Gtk results from phil: philn-tp: $1 = (WebCore::SVGTextFragment &) @0xe1f160: {characterOffset = 0, metricsListOffset = 0, length = 16, isTextOnPath = false, x = 10, y = 340, width = 45, height = 7.80000019, transform = {m_transform = {1, 0, 0, 1, 0, 0}}, lengthAdjustTransform = {m_transform = {1, 0, 0, 1, 0, 0}}} philn-tp: $2 = (const WebCore::FontMetrics &) @0xe15c4c: {m_unitsPerEm = 1000, m_ascent = 6, m_descent = 2, m_lineGap = 0, m_lineSpacing = 8, m_xHeight = 4} Sorry for the formatting, that's over IRC. All integers on Gtk! Let's continue investigating.

In WebCore::SimpleFontData::platformInit, p font_extents $1 = {ascent = 6, descent = 2, height = 6.9999999999999991, max_x_advance = 6.9999999999999991, max_y_advance = 0} And in WebCore::writeSVGInlineTextBox: p textBox->textRenderer()->style()->font() $1 = (const WebCore::Font &) @0xe3d858: {static s_codePath = WebCore::Font::Auto, static s_roundingHackCharacterTable = "\000\000\000\000\000\000\000\000\000\001\001", '\000' <repeats 21 times>, "\001", '\000' <repeats 12 times>, "\001", '\000' <repeats 17 times>, "\001", '\000' <repeats 96 times>, "\001", '\000' <repeats 94 times>, m_fontDescription = {m_familyList = { m_family = "Arial", m_next = {<WTF::RefPtr<WebCore::SharedFontFamily>> = {m_ptr = 0x0}, <No data fields>}}, m_featureSettings = {m_ptr = 0x0}, m_specifiedSize = 6, m_computedSize = 6, m_orientation = 0, m_textOrientation = 0, m_widthVariant = 0, m_italic = 0, m_smallCaps = 0, m_isAbsoluteSize = true, m_weight = 3, m_genericFamily = 0, m_usePrinterFont = false, m_renderingMode = 0, m_keywordSize = 0, m_fontSmoothing = 0, m_textRendering = 0, m_isSpecifiedFont = true, m_script = USCRIPT_COMMON}, m_fontList = {m_ptr = 0xe3d8b0}, m_letterSpacing = 0, m_wordSpacing = 0, m_isPlatformFont = false, m_needsTranscoding = false}

I've just updated my libfreetype6 and fonts packages. It seems I get new results! p textBox->textFragments().at(0) $1 = (WebCore::SVGTextFragment &) @0xe54570: {characterOffset = 0, metricsListOffset = 0, length = 16, isTextOnPath = false, x = 10, y = 340, width = 45, height = 6.5999999, transform = {m_transform = {1, 0, 0, 1, 0, 0}}, lengthAdjustTransform = {m_transform = {1, 0, 0, 1, 0, 0}}} The font metrics are still bad though: (gdb) p textBox->textRenderer()->style()->fontMetrics() [Thread 0x7fffa58f7700 (LWP 606) exited] $2 = (const WebCore::FontMetrics &) @0xe1c53c: {m_unitsPerEm = 1000, m_ascent = 5, m_descent = 1, m_lineGap = 1, m_lineSpacing = 7, m_xHeight = 4}

Created attachment 115811 [details] Patch v9 This is the same patch as version 8, but with a fix for bug 72614 included, which give precise non-integer TextMetrics for Freetype text backends. Its just a matter of setting CAIRO_HINT_METRICS_OFF.

(In reply to comment #34) > Created an attachment (id=115811) [details] > Patch v9 > > This is the same patch as version 8, but with a fix for bug 72614 included, which give precise non-integer TextMetrics for Freetype text backends. Its just a matter of setting CAIRO_HINT_METRICS_OFF. Bug 72614 discusses whats wrong with the Gtk/Cairo/Freetype usage at present and how to fix it easily :-) This fix is included in patch v9.

Created attachment 115830 [details] Patch v10 Okay, we finally have non-integer FontMetrics on Gtk, now I've modified SimpleFontDataFreeType to match the Mac metrics calculations, let's see how it affects Gtk. Note: I couldn't test compilation on Gtk, this is a blind attempt. Ossy, can you do another 32bit vs. 64bit build on Qt as well?

Comment on attachment 115830 [details] Patch v10 Attachment 115830 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10374639

Created attachment 115838 [details] Patch v11 Fix gtk/efl builds.

Comment on attachment 115838 [details] Patch v11 Attachment 115838 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10435014 New failing tests: fast/block/float/overhanging-tall-block.html svg/W3C-I18N/g-dirRTL-ubNone.svg fast/replaced/width100percent-textarea.html platform/chromium-linux/svg/text/text-with-geometric-precision.svg svg/W3C-I18N/g-dirLTR-ubOverride.svg svg/W3C-I18N/g-dirLTR-ubNone.svg fast/backgrounds/size/contain-and-cover-zoomed.html

Created attachment 116484 [details] Updated patch - v1 Now that the String::number() fixes landed, here's a minimal patch that adresses SVG <path> rounding errors, 0.0 vs -0.0 issues, and the rounding problems with the RenderSVGText/RenderSVGInlineText DRT output. Splitting up the patch into multiple pieces here makes no sense, as each of them requires a world rebaseline, and I want to avoid that! I'll upload a seperated patch to update the Mac baseline.

Created attachment 116485 [details] Updated patch - v1 (Mac rebaseline)

A note to all platform maintainers. Once this lands all SVG expected.txt files need an update. I highly encourage you to check wheter your platform specific rebaselines are still needed, as lots of small DRT output problems are fixed, as they output is now platform independant, not using snprintfs anymore etc. A unique 0.0 is enforced, removing the problems win/qt/gtk see with 0.0 vs -0.0.

The baseline optimizer can automagically remove redundant baselines. It's probably worth running that over the svg baselines after this patch lands and folks update their baselines.

Comment on attachment 116484 [details] Updated patch - v1 r=me View in context: https://bugs.webkit.org/attachment.cgi?id=116484&action=review > Source/WebCore/svg/SVGPathStringBuilder.cpp:120 > + DEFINE_STATIC_LOCAL(const String, noString, ("0")); > + DEFINE_STATIC_LOCAL(const String, yesString, ("1")); I think a simple String::number conversion would be enough here. This seems overcomplicate things.

Niko, please contact with the bot managers before landing this patch, it will likely break things.

Created attachment 116494 [details] Updated patch - v1 (Qt rebaseline)

Comment on attachment 116485 [details] Updated patch - v1 (Mac rebaseline) Attachment 116485 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10648327 New failing tests: fast/replaced/width100percent-textarea.html svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg svg/W3C-SVG-1.1-SE/coords-dom-01-f.svg svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg svg/W3C-SVG-1.1/filters-turb-02-f.svg svg/W3C-SVG-1.1-SE/styling-pres-02-f.svg svg/W3C-SVG-1.1-SE/interact-pointer-03-t.svg svg/W3C-SVG-1.1-SE/coords-dom-04-f.svg svg/W3C-SVG-1.1-SE/coords-dom-03-f.svg svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg svg/W3C-SVG-1.1-SE/pservers-pattern-04-f.svg svg/W3C-SVG-1.1-SE/painting-control-04-f.svg svg/W3C-SVG-1.1-SE/text-tref-03-b.svg svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg

Comment on attachment 116484 [details] Updated patch - v1 Attachment 116484 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10664093 New failing tests: fast/block/float/overhanging-tall-block.html svg/W3C-I18N/g-dirLTR-ubNone.svg platform/chromium-linux/svg/text/text-with-geometric-precision.svg svg/W3C-SVG-1.1/animate-elem-04-t.svg svg/W3C-I18N/text-anchor-dirNone-anchorMiddle.svg fast/replaced/width100percent-textarea.html svg/W3C-I18N/text-anchor-dirLTR-anchorStart.svg svg/W3C-SVG-1.1/animate-elem-05-t.svg svg/W3C-I18N/text-anchor-dirNone-anchorStart.svg svg/W3C-I18N/text-anchor-dirLTR-anchorMiddle.svg svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg svg/W3C-I18N/text-anchor-dirNone-anchorEnd.svg svg/W3C-I18N/g-dirLTR-ubOverride.svg svg/W3C-I18N/g-dirRTL-ubNone.svg svg/W3C-I18N/g-dirRTL-ubOverride.svg

I'm going to land this patch now, as discussed with the Gtk/Qt/Chromium gardeners, which are all around on IRC to rebaseline their platforms. Thanks for this great coordination!

Landed in r101342, now waiting for the bots to cycle...

(In reply to comment #50) > Landed in r101342, now waiting for the bots to cycle... Qt rebaselines landed in: http://trac.webkit.org/changeset/101344 http://trac.webkit.org/changeset/101346

Created attachment 116939 [details] failing tests on 64 bit QtWebKit after r101342 I skipped them: http://trac.webkit.org/changeset/101353

Landed Win baseline update in r101358.

Landed lion rebaselines in r101363.

These still appear to be failing on lion: http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101515%20(3080)/svg/W3C-I18N/text-anchor-inherited-dirLTR-anchorStart-pretty-diff.html

(In reply to comment #55) > These still appear to be failing on lion: > http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101515%20(3080)/svg/W3C-I18N/text-anchor-inherited-dirLTR-anchorStart-pretty-diff.html I'm working on a fix for that, stay tuned.

The SVG path d attribute output is fixed, closing this particular bug now.