Bug 54474 – Pixel tests differences on 10.6.6 32bit vs. 64bit

• Component: SVG
• Status: RESOLVED WONTFIX

[Nikolas Zimmermann]

When I generate a SVG pixel test baseline using my vanilla 10.6.6 installation, on my old Core Duo MacBook Pro (32bit), copy it over to my Core 2 Duo MacBook Pro (64bit) there are numerous of differences in the results, they faill in two categories:
a) Just one-pixel diff, not visible, only if you look really close (below 0.01% difference), probably explainable as CGFloat is float on 32bit / double on 64bit, causing very tiny rendering differences at some positions.
b) Whole text fragments are moved a tiny amount in x/y direction, scale seems to vary a bit on several tests, all in all pretty weird.

The goal is to have a baseline, that can be generated on eg. a 32bit SL machine, that passes with tolerance 0 on the 32bit machine, and tolerance 0.01 on the 64bit machine. Currently there are 250+ differences
So there's nothing we can do about a), as CGFloat is typedef'ed to float on 32bit, and double on 64bit.

had to debug quite a lot, to find out what's causing b), patch is coming soon, with more explainations.

[Nikolas Zimmermann]

Created attachment 82618 [details]
Patch v1

Uploading a first version for a EWS check.

[Dirk Schulze]

Comment on attachment 82618 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=82618&action=review

LGTM. r=me. You should land this on weekend. It's more likely that you get the results fixed on weekend than today. But it is up to you.

> Source/WebCore/ChangeLog:16
> +        b) Failures >0.1px (text origin/scale is slightly different, user-visible).

s/px/%/

[Dirk Schulze]

Comment on attachment 82618 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=82618&action=review

> Source/WebCore/svg/SVGSVGElement.cpp:137
> +    FloatPoint origin;
> +    if (!isOutermostSVG())
> +        origin = FloatPoint(x().value(this), y().value(this));
> +
> +    FloatSize size(width().value(this), height().value(this));
> +    return viewBoxToViewTransform(size.width(), size.height()).mapRect(FloatRect(origin, size));

Talked on IRC about it. I think it is better readable to use a FloatRect from the beginning.

[Nikolas Zimmermann]

Landed in r78704, including a first set of pixel test rebaselines.
Landed second chunk of pixel test updates in r78706.

[Nikolas Zimmermann]

Landed third chunk in r78707.
Landing the fourth chunk hangs, I have to leave for 1h, back then landing the rest of the baseline.
Bots should be fine though, as I already landed the text changes.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78704 might have broken Qt Linux Release
The following tests are not passing:
svg/custom/non-scaling-stroke-markers.svg
svg/custom/text-rotated-gradient.svg

[Nikolas Zimmermann]

Ossy updated Qt baselines in r78709.
I landed the fourth chunk in r78711.
Michail landed Chromium baselines in r78712.
I forgot to include the WebCore ChangeLog, fixed in r78713.

Now landing last chunk of pixel test results...

[Nikolas Zimmermann]

Landed the last chunk of pixel test results in r78714.
Landed gtk/win/mac-leopard text result updates in r78716.

Now all bots should be happy again, closing bug.

[Nikolas Zimmermann]

Oops, webkit-patch rebaseline doesn't generate new platforms for win, if the test result isn't already present, but instead rebaselines mac - I mixed up win/mac-leopard results in my last commit.
Fixed in r78720.

[Nikolas Zimmermann]

Updated gtk results using the 64bit debug bot in r78722. I fear gtk exposes another rounding bug.
Reopening bug, eventually we have to skip some tests on gtk, awaiting results.

[Martin Robinson]

(In reply to comment #10)
> Updated gtk results using the 64bit debug bot in r78722. I fear gtk exposes another rounding bug.
> Reopening bug, eventually we have to skip some tests on gtk, awaiting results.

Do you mind giving a bit more information about the rounding bug you've uncovered on GTK+ and where it might reside?

[Nikolas Zimmermann]

(In reply to comment #11)
> (In reply to comment #10)
> > Updated gtk results using the 64bit debug bot in r78722. I fear gtk exposes another rounding bug.
> > Reopening bug, eventually we have to skip some tests on gtk, awaiting results.
> 
> Do you mind giving a bit more information about the rounding bug you've uncovered on GTK+ and where it might reside?

For the record, talking with mrobinsons on IRC:
[19:42] WildFox: I just had two issues with text results
[19:42] WildFox: but the reason is pretty simple to find
[19:42] WildFox: width=12, can be stored in a FloatRect, as 11.99999996 or 12.00000004, whenever we use enclosingIntRect() we have a problem, as 'width' is ceil'ed, that means ceil(11.999..) = 12, ceil(12.0004) = 13
[19:43] WildFox: that's mostly the reason for differ'ing textual dumps

[Martin Robinson]

(12:08:54 PM) WildFox: mrobinson: could you post your findings as well on the br
(12:09:25 PM) mrobinson: Sure.
(12:09:48 PM) WildFox: maybe mention that you'll see the same problem as I saw before

I debugged this locally and found the following issue on my 64-bit GTK+ setup:

WebCore::RenderSVGRoot::clippedOverflowRectForRepaint calls
WebCore::SVGRenderSupport::clippedOverflowRectForRepaint calls
WebCore::AffineTransform::mapRect
@ line 285: repaintRect = localToBorderBoxTransform().mapRect(repaintRect); 
AffineTransformation::mapRect(IntRect) calls @ line 306
return enclosingIntRect(mapRect(FloatRect(rect)));

The return value of mapRect(FloatRect(rect)) is
(const WebCore::FloatRect &) @0x7fffffffd230: {m_location = {m_x = 99.9999847, m_y = 0}, m_size = {m_width = 300, m_height = 300}}

And thus the enclosingIntRect is:
 {m_location = {m_x = 99, m_y = 0}, m_size = {m_width = 301, m_height = 300}}

And that's the root of the problem.

[Martin Robinson]

(In reply to comment #13)
> And that's the root of the problem.

My statement here is incorrect. The root of the problem seems to be 
SVGPreserveAspectRatio::getCTM. It call transform.translate (which accepts double paramters) with the float arguments.. As a proof of concept, this change seems to fix the 64-bit rounding issue with 
LayoutTests/svg/W3C-SVG-1.1/struct-frag-03-t.svg.


diff --git a/Source/WebCore/svg/SVGPreserveAspectRatio.cpp b/Source/WebCore/svg/SVGPreserveAspectRatio.cpp
index 2ebcc1f..82b9bb9 100644
--- a/Source/WebCore/svg/SVGPreserveAspectRatio.cpp
+++ b/Source/WebCore/svg/SVGPreserveAspectRatio.cpp
@@ -262,13 +262,19 @@ AffineTransform SVGPreserveAspectRatio::getCTM(float logicX, float logicY, float
         return transform;
     }
 
+    double logicXD = logicX;
+    double logicWidthD = logicWidth;
+    double logicHeightD = logicHeight;
+    double physWidthD = physWidth;
+    double physHeightD = physHeight;
+
     if ((logicalRatio < physRatio && (m_meetOrSlice == SVG_MEETORSLICE_MEET)) || (logicalRatio >= physRatio && (m_meetOrSlice == SVG_MEETORSLICE_SLICE))) {
         transform.scaleNonUniform(physHeight / logicHeight, physHeight / logicHeight);
 
         if (m_align == SVG_PRESERVEASPECTRATIO_XMINYMIN || m_align == SVG_PRESERVEASPECTRATIO_XMINYMID || m_align == SVG_PRESERVEASPECTRATIO_XMINYMAX)
             transform.translate(-logicX, -logicY);
         else if (m_align == SVG_PRESERVEASPECTRATIO_XMIDYMIN || m_align == SVG_PRESERVEASPECTRATIO_XMIDYMID || m_align == SVG_PRESERVEASPECTRATIO_XMIDYMAX)
-            transform.translate(-logicX - (logicWidth - physWidth * logicHeight / physHeight) / 2, -logicY);
+            transform.translate(-logicXD - (logicWidthD - physWidthD * logicHeightD / physHeightD) / 2, -logicY);
         else
             transform.translate(-logicX - (logicWidth - physWidth * logicHeight / physHeight), -logicY);

Perhaps the solution here is for SVGPreserveAspectRatio::getCTM to accept double parameters.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78722 might have broken GTK Linux 32-bit Release

[Nikolas Zimmermann]

(In reply to comment #14)
> (In reply to comment #13)
> > And that's the root of the problem.
> 
> My statement here is incorrect. The root of the problem seems to be 
> SVGPreserveAspectRatio::getCTM. It call transform.translate (which accepts double paramters) with the float arguments.. As a proof of concept, this change seems to fix the 64-bit rounding issue with 
> LayoutTests/svg/W3C-SVG-1.1/struct-frag-03-t.svg.
> 
> 
> diff --git a/Source/WebCore/svg/SVGPreserveAspectRatio.cpp b/Source/WebCore/svg/SVGPreserveAspectRatio.cpp
> index 2ebcc1f..82b9bb9 100644
> --- a/Source/WebCore/svg/SVGPreserveAspectRatio.cpp
> +++ b/Source/WebCore/svg/SVGPreserveAspectRatio.cpp
> @@ -262,13 +262,19 @@ AffineTransform SVGPreserveAspectRatio::getCTM(float logicX, float logicY, float
>          return transform;
>      }
> 
> +    double logicXD = logicX;
> +    double logicWidthD = logicWidth;
> +    double logicHeightD = logicHeight;
> +    double physWidthD = physWidth;
> +    double physHeightD = physHeight;
> +
>      if ((logicalRatio < physRatio && (m_meetOrSlice == SVG_MEETORSLICE_MEET)) || (logicalRatio >= physRatio && (m_meetOrSlice == SVG_MEETORSLICE_SLICE))) {
>          transform.scaleNonUniform(physHeight / logicHeight, physHeight / logicHeight);
> 
>          if (m_align == SVG_PRESERVEASPECTRATIO_XMINYMIN || m_align == SVG_PRESERVEASPECTRATIO_XMINYMID || m_align == SVG_PRESERVEASPECTRATIO_XMINYMAX)
>              transform.translate(-logicX, -logicY);
>          else if (m_align == SVG_PRESERVEASPECTRATIO_XMIDYMIN || m_align == SVG_PRESERVEASPECTRATIO_XMIDYMID || m_align == SVG_PRESERVEASPECTRATIO_XMIDYMAX)
> -            transform.translate(-logicX - (logicWidth - physWidth * logicHeight / physHeight) / 2, -logicY);
> +            transform.translate(-logicXD - (logicWidthD - physWidthD * logicHeightD / physHeightD) / 2, -logicY);
>          else
>              transform.translate(-logicX - (logicWidth - physWidth * logicHeight / physHeight), -logicY);
> 
> Perhaps the solution here is for SVGPreserveAspectRatio::getCTM to accept double parameters.
Look at my patch, I reverted just _that_. It was causing trouble for me between mac machines. Internally we use float everywhere in SVG, except for AffineTransform which is double based...

[Martin Robinson]

(In reply to comment #16)
> > Perhaps the solution here is for SVGPreserveAspectRatio::getCTM to accept double parameters.
> Look at my patch, I reverted just _that_. It was causing trouble for me between mac machines. Internally we use float everywhere in SVG, except for AffineTransform which is double based...

Right. I discovered this shortly after while talking to Dirk. The change seems suspicious as it throws away precision in this situation. I'm not surprised it introduced rounding errors. Perhaps there is some other way to achieve what you want for Mac, without affecting other platform detrimentally. Perhaps AffineTransformation could also accept floats.

[Martin Robinson]

(In reply to comment #17)
> (In reply to comment #16)
> > > Perhaps the solution here is for SVGPreserveAspectRatio::getCTM to accept double parameters.
> > Look at my patch, I reverted just _that_. It was causing trouble for me between mac machines. Internally we use float everywhere in SVG, except for AffineTransform which is double based...
> 
> Right. I discovered this shortly after while talking to Dirk. The change seems suspicious as it throws away precision in this situation. I'm not surprised it introduced rounding errors. Perhaps there is some other way to achieve what you want for Mac, without affecting other platform detrimentally. Perhaps AffineTransformation could also accept floats.

It seems specializing float operations within AffineTransformation can also fix this issue.


diff --git a/Source/WebCore/platform/graphics/transforms/AffineTransform.cpp b/Source/WebCore/platform/graphics/transforms/AffineTransform.cpp
index a1ffa30..f8c52b5 100644
--- a/Source/WebCore/platform/graphics/transforms/AffineTransform.cpp
+++ b/Source/WebCore/platform/graphics/transforms/AffineTransform.cpp
@@ -295,10 +295,10 @@ IntPoint AffineTransform::mapPoint(const IntPoint& point) const
 
 FloatPoint AffineTransform::mapPoint(const FloatPoint& point) const
 {
-    double x2, y2;
-    map(point.x(), point.y(), x2, y2);
+    float x2 = (narrowPrecisionToFloat(m_transform[0]) * point.x() + narrowPrecisionToFloat(m_transform[2]) * point.y() + narrowPrecisionToFloat(m_transform[4]));
+    float y2 = (narrowPrecisionToFloat(m_transform[1]) * point.x() + narrowPrecisionToFloat(m_transform[3]) * point.y() + narrowPrecisionToFloat(m_transform[5]));
 
-    return FloatPoint(narrowPrecisionToFloat(x2), narrowPrecisionToFloat(y2));
+    return FloatPoint(x2, y2);
 }
 
 IntRect AffineTransform::mapRect(const IntRect &rect) const

[Darin Adler]

Comment on attachment 82618 [details]
Patch v1

Clearing review flag since this was reopened.

Might be a duplicate of bug https://bugs.webkit.org/show_bug.cgi?id=62204

I've managed to have all rounding related tests pass with 32bit gtk build by using these additional flags:


Index: GNUmakefile.am
===================================================================
--- GNUmakefile.am	(revision 99472)
+++ GNUmakefile.am	(working copy)
@@ -116,7 +116,8 @@
 	-Wformat -Wformat-security -Wno-format-y2k -Wundef \
 	-Wmissing-format-attribute -Wpointer-arith -Wwrite-strings \
 	-Wno-unused-parameter -Wno-parentheses \
-	-fno-exceptions -DENABLE_GLIB_SUPPORT=1
+	-fno-exceptions -DENABLE_GLIB_SUPPORT=1 \
+	-march=pentium4 -msse2 -mfpmath=sse
 
 
 global_cxxflags += \


The description of these flags and why they are needed is here: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/build/common.gypi&q=file:build/common.gypi&exact_package=chromium&l=1718

Should I create a patch? What do you think about this approach?

[Martin Robinson]

(In reply to comment #21)
> I've managed to have all rounding related tests pass with 32bit gtk build by using these additional flags:
> 
> 
> Index: GNUmakefile.am
> ===================================================================
> --- GNUmakefile.am    (revision 99472)
> +++ GNUmakefile.am    (working copy)
> @@ -116,7 +116,8 @@
>      -Wformat -Wformat-security -Wno-format-y2k -Wundef \
>      -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings \
>      -Wno-unused-parameter -Wno-parentheses \
> -    -fno-exceptions -DENABLE_GLIB_SUPPORT=1
> +    -fno-exceptions -DENABLE_GLIB_SUPPORT=1 \
> +    -march=pentium4 -msse2 -mfpmath=sse
> 
> 
>  global_cxxflags += \
> 

Awesome! Do you mind opening a new bug that addresses GTK+ specifically. I'd prefer that this work-around only be applied when building via build-webkit, because I'm not sure if it has any performance implications otherwise. You can ensure that it only affects build-webkit, by modifying Tools/Scripts/webkitdirs.pm.

I'm not sure how to do that. Can you help me with it?

Opened a new bug:
https://bugs.webkit.org/show_bug.cgi?id=72254

[Nikolas Zimmermann]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > > Perhaps the solution here is for SVGPreserveAspectRatio::getCTM to accept double parameters.
> > > Look at my patch, I reverted just _that_. It was causing trouble for me between mac machines. Internally we use float everywhere in SVG, except for AffineTransform which is double based...
> > 
> > Right. I discovered this shortly after while talking to Dirk. The change seems suspicious as it throws away precision in this situation. I'm not surprised it introduced rounding errors. Perhaps there is some other way to achieve what you want for Mac, without affecting other platform detrimentally. Perhaps AffineTransformation could also accept floats.
> 
> It seems specializing float operations within AffineTransformation can also fix this issue.
I came to another conclusion. mapPoint is only used directly in the SVG rendering code for hit testing, for anything else mapRect is used (computeRectForRepaint, etc.) which is used to dump the position of SVG renderers.

mapRect currently uses FloatQuad. The four corners are rounded individually, and FloatRect(left, top, right - left, top - bottom) is returned, but each of the corners is rounded on their own.
I'm now trying to preserver precision as long as possible (calculating the - in doubles now, instead of floats). This affects several test results on Mac, I hope they are progressions for Gtk, see bug 47467 for the recent work towards fixing this.

[Nikolas Zimmermann]

I've heard that the Lion baseline is supposed to be sharable - at least Timothy & Dirk could share a baseline that passed with tolerance 0 on both their machines which differed.

Does anyone have Lion machines to test this?

[Dirk Schulze]

We don't even support 10.6 anymore IIRC.