commit ef8dda227b55445de090cfc5c59fb633f8a1763d
author Brian Osman <brianosman@google.com> Fri Dec 15 11:50:20 2017 -0500
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Fri Dec 15 19:25:08 2017 +0000
tree 78ee7c2c5d7ae6d2c2e9c16a68d015185681a2e6
parent 2d53d984251a753b9d0fb3adad3be09243cf5c14

Increase accuracy of float -> fixed in ICC code

Add a comment to SkFixed explaining the accuracy issues of the macros.

Bug: skia:
Change-Id: Ibfecb16821fefe87822cc3acd1cf8498df10a492
Reviewed-on: https://skia-review.googlesource.com/85742
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

include/private/SkFixed.h

diff --git a/include/private/SkFixed.h b/include/private/SkFixed.h
index adb0343..05e1de4 100644
--- a/include/private/SkFixed.h
+++ b/include/private/SkFixed.h
@@ -30,6 +30,11 @@
 #define SK_FixedTanPIOver8  (0x6A0A)
 #define SK_FixedRoot2Over2  (0xB505)
 
+// NOTE: SkFixedToFloat is exact. SkFloatToFixed seems to lack a rounding step. For all fixed-point
+// values, this version is as accurate as possible for (fixed -> float -> fixed). Rounding reduces
+// accuracy if the intermediate floats are in the range that only holds integers (adding 0.5f to an
+// odd integer then snaps to nearest even). Using double for the rounding math gives maximum
+// accuracy for (float -> fixed -> float), but that's usually overkill.
 #define SkFixedToFloat(x)   ((x) * 1.52587890625e-5f)
 #define SkFloatToFixed(x)   sk_float_saturate2int((x) * SK_Fixed1)
 

src/core/SkICC.cpp

diff --git a/src/core/SkICC.cpp b/src/core/SkICC.cpp
index 128bce3..cfc66e1 100644
--- a/src/core/SkICC.cpp
+++ b/src/core/SkICC.cpp
@@ -281,25 +281,33 @@
     SkEndian_SwapBE32(kTAG_cprt_Bytes),
 };
 
+// This is like SkFloatToFixed, but rounds to nearest, preserving as much accuracy as possible
+// when going float -> fixed -> float (it has the same accuracy when going fixed -> float -> fixed).
+// The use of double is necessary to accomodate the full potential 32-bit mantissa of the 16.16
+// SkFixed value, and so avoiding rounding problems with float. Also, see the comment in SkFixed.h.
+static SkFixed float_round_to_fixed(float x) {
+    return sk_float_saturate2int((float)floor((double)x * SK_Fixed1 + 0.5));
+}
+
 static void write_xyz_tag(uint32_t* ptr, const SkMatrix44& toXYZ, int col) {
     ptr[0] = SkEndian_SwapBE32(kXYZ_PCSSpace);
     ptr[1] = 0;
-    ptr[2] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(0, col)));
-    ptr[3] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(1, col)));
-    ptr[4] = SkEndian_SwapBE32(SkFloatToFixed(toXYZ.getFloat(2, col)));
+    ptr[2] = SkEndian_SwapBE32(float_round_to_fixed(toXYZ.getFloat(0, col)));
+    ptr[3] = SkEndian_SwapBE32(float_round_to_fixed(toXYZ.getFloat(1, col)));
+    ptr[4] = SkEndian_SwapBE32(float_round_to_fixed(toXYZ.getFloat(2, col)));
 }
 
 static void write_trc_tag(uint32_t* ptr, const SkColorSpaceTransferFn& fn) {
     ptr[0] = SkEndian_SwapBE32(kTAG_ParaCurveType);
     ptr[1] = 0;
     ptr[2] = (uint32_t) (SkEndian_SwapBE16(kGABCDEF_ParaCurveType));
-    ptr[3] = SkEndian_SwapBE32(SkFloatToFixed(fn.fG));
-    ptr[4] = SkEndian_SwapBE32(SkFloatToFixed(fn.fA));
-    ptr[5] = SkEndian_SwapBE32(SkFloatToFixed(fn.fB));
-    ptr[6] = SkEndian_SwapBE32(SkFloatToFixed(fn.fC));
-    ptr[7] = SkEndian_SwapBE32(SkFloatToFixed(fn.fD));
-    ptr[8] = SkEndian_SwapBE32(SkFloatToFixed(fn.fE));
-    ptr[9] = SkEndian_SwapBE32(SkFloatToFixed(fn.fF));
+    ptr[3] = SkEndian_SwapBE32(float_round_to_fixed(fn.fG));
+    ptr[4] = SkEndian_SwapBE32(float_round_to_fixed(fn.fA));
+    ptr[5] = SkEndian_SwapBE32(float_round_to_fixed(fn.fB));
+    ptr[6] = SkEndian_SwapBE32(float_round_to_fixed(fn.fC));
+    ptr[7] = SkEndian_SwapBE32(float_round_to_fixed(fn.fD));
+    ptr[8] = SkEndian_SwapBE32(float_round_to_fixed(fn.fE));
+    ptr[9] = SkEndian_SwapBE32(float_round_to_fixed(fn.fF));
 }
 
 static bool is_3x3(const SkMatrix44& toXYZD50) {