shiny/iconvg: make SetEllipticalGradient math deterministic
Add explicit rounding in Encoder.SetEllipticalGradient to disable
fused operations. This allows the TestEncodeElliptical case to pass
on GOARCH=arm64. Otherwise, fused multiply-sub instructions are
used on GOARCH=arm64 (but not GOARCH=amd64), which results in a
slightly different real value being computed for a "Set NREG"
instruction (with a tiny delta of 7.450581e-09), and then
encoded into non-identical bytes:
ac Set NREG[NSEL-4] to a real number
-00 0
+03 00 00 b2 -7.450581e-09
Document the IconVG design decision that the encoder aims to
produce byte-identical output on all supported platforms per
suggestion in golang.org/issue/43219#issuecomment-748531069.
This should make it clear for the future—if another similar
problem is detected—that the encoder implementation should
be fixed (rather than relaxing Encode tests to allow for
non-deterministic encoding). It also allows package users
to be aware that the encoder is platform-agnostic and not
have to take additional measures themselves. (If this proves
to be truly unsustainable to maintain, it is still viable to
remove it given the "SUBJECT TO INCOMPATIBLE CHANGES" clause.)
Rename TestRasterizer to TestDecodeAndRasterize since it tests
both decoding and rasterizing. There isn't another test that
tests only the Decode function.
Fix some broken links to the material-design-icons repository.
Fixes golang/go#43219.
Updates golang/go#11811.
Change-Id: I5cf15541a4648e408c9c61689041c678c14441bf
Reviewed-on: https://go-review.googlesource.com/c/exp/+/279294
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Nigel Tao <nigeltao@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
diff --git a/shiny/iconvg/decode_test.go b/shiny/iconvg/decode_test.go
index fae6ca7..c1778e6 100644
--- a/shiny/iconvg/decode_test.go
+++ b/shiny/iconvg/decode_test.go
@@ -155,7 +155,7 @@
continue
}
wantFilename := filepath.FromSlash(tc.filename) + ".ivg.disassembly"
- if overwriteTestdataFiles {
+ if *updateFlag {
if err := ioutil.WriteFile(filepath.FromSlash(wantFilename), got, 0666); err != nil {
t.Errorf("%s: WriteFile: %v", tc.filename, err)
}
@@ -173,7 +173,7 @@
}
}
-func TestRasterizer(t *testing.T) {
+func TestDecodeAndRasterize(t *testing.T) {
for _, tc := range testdataTestCases {
ivgData, err := ioutil.ReadFile(filepath.FromSlash(tc.filename) + ".ivg")
if err != nil {
@@ -218,7 +218,7 @@
wantFilename += "." + variant
}
wantFilename += ".png"
- if overwriteTestdataFiles {
+ if *updateFlag {
if err := encodePNG(filepath.FromSlash(wantFilename), got); err != nil {
t.Errorf("%s %q variant: encodePNG: %v", tc.filename, variant, err)
}
diff --git a/shiny/iconvg/doc.go b/shiny/iconvg/doc.go
index bfa7310..8627814 100644
--- a/shiny/iconvg/doc.go
+++ b/shiny/iconvg/doc.go
@@ -28,6 +28,9 @@
Nonetheless, at typical scales, e.g. up to 4096 × 4096, such differences are
not expected to be perceptible to the naked eye.
+The encoder aims to emit byte-identical output for the same input, independent
+of the platform (and specifically its floating-point hardware).
+
Structure
@@ -474,7 +477,7 @@
The production version of the "action/info" icon from the Material Design icon
set is defined by the following SVG, also available at
-https://github.com/google/material-design-icons/blob/master/action/svg/production/ic_info_48px.svg:
+https://github.com/google/material-design-icons/blob/3.0.2/action/svg/production/ic_info_48px.svg:
<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 48 48">
<path d="M24 4C12.95 4 4 12.95 4 24s8.95 20 20 20 20-8.95 20-20S35.05 4 24 4z
diff --git a/shiny/iconvg/encode.go b/shiny/iconvg/encode.go
index c9c3ac9..6c27f83 100644
--- a/shiny/iconvg/encode.go
+++ b/shiny/iconvg/encode.go
@@ -34,6 +34,9 @@
// The zero value is usable. Calling Reset, which is optional, sets the
// Metadata for the subsequent encoded form. If Reset is not called before
// other Encoder methods, the default metadata is implied.
+//
+// It aims to emit byte-identical Bytes output for the same input, independent
+// of the platform (and specifically its floating-point hardware).
type Encoder struct {
// HighResolutionCoordinates is whether the encoder should encode
// coordinate numbers for subsequent paths at the best possible resolution
@@ -410,16 +413,21 @@
// axis vectors (rx, ry) and (sx, sy) such that (cx+rx, cy+ry) and (cx+sx,
// cy+sy) are on the ellipse.
func (e *Encoder) SetEllipticalGradient(cBase, nBase uint8, cx, cy, rx, ry, sx, sy float32, spread GradientSpread, stops []GradientStop) {
+ // Explicitly disable FMA in the floating-point calculations below
+ // to get consistent results on all platforms, and in turn produce
+ // a byte-identical encoding.
+ // See https://golang.org/ref/spec#Floating_point_operators and issue 43219.
+
// See the package documentation's appendix for a derivation of the
// transformation matrix.
- invRSSR := 1 / (rx*sy - sx*ry)
+ invRSSR := 1 / (float32(rx*sy) - float32(sx*ry))
ma := +sy * invRSSR
mb := -sx * invRSSR
- mc := -ma*cx - mb*cy
+ mc := -float32(ma*cx) - float32(mb*cy)
md := -ry * invRSSR
me := +rx * invRSSR
- mf := -md*cx - me*cy
+ mf := -float32(md*cx) - float32(me*cy)
e.SetGradient(cBase, nBase, true, f32.Aff3{
ma, mb, mc,
diff --git a/shiny/iconvg/encode_test.go b/shiny/iconvg/encode_test.go
index 8091447..8c2592c 100644
--- a/shiny/iconvg/encode_test.go
+++ b/shiny/iconvg/encode_test.go
@@ -6,35 +6,28 @@
import (
"bytes"
+ "flag"
"image/color"
"io/ioutil"
"math"
"path/filepath"
+ "runtime"
"strconv"
"testing"
"golang.org/x/image/math/f32"
)
-// overwriteTestdataFiles is temporarily set to true when adding new
-// testdataTestCases.
-const overwriteTestdataFiles = false
-
-// TestOverwriteTestdataFilesIsFalse tests that any change to
-// overwriteTestdataFiles is only temporary. Programmers are assumed to run "go
-// test" before sending out for code review or committing code.
-func TestOverwriteTestdataFilesIsFalse(t *testing.T) {
- if overwriteTestdataFiles {
- t.Errorf("overwriteTestdataFiles is true; do not commit code changes")
- }
-}
+// updateFlag controls whether to overwrite testdata files during tests.
+// This can be useful when adding new testdataTestCases.
+var updateFlag = flag.Bool("update", false, "Overwrite testdata files.")
func testEncode(t *testing.T, e *Encoder, wantFilename string) {
got, err := e.Bytes()
if err != nil {
t.Fatalf("encoding: %v", err)
}
- if overwriteTestdataFiles {
+ if *updateFlag {
if err := ioutil.WriteFile(filepath.FromSlash(wantFilename), got, 0666); err != nil {
t.Fatalf("WriteFile: %v", err)
}
@@ -45,7 +38,16 @@
t.Fatalf("ReadFile: %v", err)
}
if !bytes.Equal(got, want) {
- t.Fatalf("\ngot %d bytes:\n% x\nwant %d bytes:\n% x", len(got), got, len(want), want)
+ // The IconVG encoder is expected to be completely deterministic across all
+ // platforms and Go compilers, so check that we get exactly the right bytes.
+ //
+ // If we get slightly different bytes on some supported platform (for example,
+ // a new GOOS/GOARCH port, or a different but spec-compliant Go compiler) due
+ // to non-determinism in floating-point math, the encoder needs to be fixed.
+ //
+ // See golang.org/issue/43219#issuecomment-748531069.
+ t.Fatalf("\ngot %d bytes (on GOOS=%s GOARCH=%s, using compiler %q):\n% x\nwant %d bytes:\n% x",
+ len(got), runtime.GOOS, runtime.GOARCH, runtime.Compiler, got, len(want), want)
}
}
@@ -144,7 +146,7 @@
stops []GradientStop
}{{
-// The 0th element is unused.
+ // The 0th element is unused.
}, {
radial: true,
cx: -102.14,
diff --git a/shiny/iconvg/testdata/README b/shiny/iconvg/testdata/README
index 22b9579..ce53d8c 100644
--- a/shiny/iconvg/testdata/README
+++ b/shiny/iconvg/testdata/README
@@ -1,6 +1,6 @@
action-info.svg comes from the Material Design icon set. See
action/svg/production/ic_info_48px.svg in the
-github.com/google/material-design-icons repository.
+github.com/google/material-design-icons repository at the tag 3.0.2.
action-info.{lo,hi}res.ivg are low- and high-resolution IconVG versions of that
SVG file. Low resolution means that coordinates are quantized to 1/64th of a
