Bug 54474 - Pixel tests differences on 10.6.6 32bit vs. 64bit
Summary: Pixel tests differences on 10.6.6 32bit vs. 64bit
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 72254
Blocks: 62204
  Show dependency treegraph
 
Reported: 2011-02-15 09:51 PST by Nikolas Zimmermann
Modified: 2014-05-12 06:09 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 (23.10 KB, patch)
2011-02-16 05:21 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-02-15 09:51:57 PST
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.
Comment 1 Nikolas Zimmermann 2011-02-16 05:21:46 PST
Created attachment 82618 [details]
Patch v1

Uploading a first version for a EWS check.
Comment 2 Dirk Schulze 2011-02-16 05:48:14 PST
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/%/
Comment 3 Dirk Schulze 2011-02-16 05:49:31 PST
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.
Comment 4 Nikolas Zimmermann 2011-02-16 07:23:30 PST
Landed in r78704, including a first set of pixel test rebaselines.
Landed second chunk of pixel test updates in r78706.
Comment 5 Nikolas Zimmermann 2011-02-16 07:35:28 PST
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.
Comment 6 WebKit Review Bot 2011-02-16 07:51:35 PST
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
Comment 7 Nikolas Zimmermann 2011-02-16 09:00:16 PST
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...
Comment 8 Nikolas Zimmermann 2011-02-16 09:15:16 PST
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.
Comment 9 Nikolas Zimmermann 2011-02-16 10:00:15 PST
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.
Comment 10 Nikolas Zimmermann 2011-02-16 10:21:07 PST
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.
Comment 11 Martin Robinson 2011-02-16 10:23:35 PST
(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?
Comment 12 Nikolas Zimmermann 2011-02-16 10:44:56 PST
(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
Comment 13 Martin Robinson 2011-02-16 12:18:17 PST
(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.
Comment 14 Martin Robinson 2011-02-16 13:39:25 PST
(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.
Comment 15 WebKit Review Bot 2011-02-16 16:30:48 PST
http://trac.webkit.org/changeset/78722 might have broken GTK Linux 32-bit Release
Comment 16 Nikolas Zimmermann 2011-02-16 23:26:34 PST
(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...
Comment 17 Martin Robinson 2011-02-17 07:28:49 PST
(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.
Comment 18 Martin Robinson 2011-02-17 09:10:03 PST
(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
Comment 19 Darin Adler 2011-07-16 09:17:34 PDT
Comment on attachment 82618 [details]
Patch v1

Clearing review flag since this was reopened.
Comment 20 vanuan 2011-11-10 14:22:55 PST
Might be a duplicate of bug https://bugs.webkit.org/show_bug.cgi?id=62204
Comment 21 vanuan 2011-11-13 11:35:43 PST
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?
Comment 22 Martin Robinson 2011-11-13 20:01:56 PST
(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.
Comment 23 vanuan 2011-11-14 02:12:49 PST
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
Comment 24 Nikolas Zimmermann 2011-11-15 06:15:17 PST
(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.
Comment 25 Nikolas Zimmermann 2011-11-29 02:09:00 PST
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?
Comment 26 Dirk Schulze 2014-05-12 06:09:01 PDT
We don't even support 10.6 anymore IIRC.