Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestImageOperationsGolden: arm64, ppc64le, s390x generate slightly different PNGs due to "fused multiply and add" (FMA) instruction #6387

Closed
anthonyfok opened this issue Oct 3, 2019 · 2 comments · Fixed by #6396
Assignees

Comments

@anthonyfok
Copy link
Contributor

@anthonyfok anthonyfok commented Oct 3, 2019

On arm64, ppc64el (ppc64le) and s390x platforms, TestImageOperationsGolden reveals that Hugo (or github.com/disintegration/gift or an underlying library it uses) generates image with different file sizes, as seen on Debian auto-builders and Ubuntu snap builders, and reproducible on e.g. Debian s390x porterbox zelenka.debian.org:

For example:

  • File: gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_84b0614b9f84c94c0773ef49ae868d0b.png
  • want: size: 58776
  • got: size: 58791

The files that differ on s390x are as follows:

  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_300x200_fill_gaussian_smart1_2.png (this one in particular has same file size but different content from byte 16895 on)
  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_84b0614b9f84c94c0773ef49ae868d0b.png
  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_9c5c204a4fc82e861344066bc8d0c7db.png
  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_a0088abf33fdbf6be1651a71e7d4dc33.png
  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_cfc2eacca4b2748852f953954207d615.png
  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_d1ad299f68cb4b3e1eba2ab7633e7857.png
  • gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_dd36fa3cc8ae7cf4d686caf1a171284b.png
  • gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_300x200_fill_gaussian_smart1_2.png
  • gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_4c320010919da2d8b63ed24818b4d8e1.png
  • gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_9d4c2220235b3c2d9fa6506be571560f.png
  • gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_c74bb417b961e09cf1aac2130b7b9b85.png

Further investigation is needed and pending.

What version of Hugo are you using (hugo version)?

0.58.x and 0.59-DEV

Does this issue reproduce with the latest release?

Yes.

Sample full error output

--- FAIL: TestImageOperationsGolden (7.48s)
    ...
    quicktest.go:288: 
        error:
          values are not deep equal
        diff (-got +want):
            (*os.fileStat)(
          - 	&{
          - 		name: "gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_84b0614b9f84c94c0773ef49ae868d0b.png",
          - 		size: 58791,
          - 		mode: 436,
          - 		modTime: time.Time{
          - 			wall: 0x2a36c6d5,
          - 			ext:  63705700464,
          - 			loc: &time.Location{
          - 				name:       "Local",
          - 				zone:       []time.zone{{name: "UTC"}},
          - 				tx:         []time.zoneTrans{{when: -9223372036854775808}},
          - 				cacheStart: -9223372036854775808,
          - 				cacheEnd:   9223372036854775807,
          - 				cacheZone:  ⟪0x400053e220⟫,
          - 			},
          - 		},
          - 		sys: syscall.Stat_t{
          - 			Dev:     0x29,
          - 			Ino:     0x23341e,
          - 			Mode:    0x81b4,
          - 			Nlink:   0x01,
          - 			Uid:     0x0b88,
          - 			Gid:     0x03f1,
          - 			Size:    58791,
          - 			Blksize: 4096,
          - 			Blocks:  120,
          - 			Atim:    syscall.Timespec{Sec: 1570103664, Nsec: 708232917},
          - 			Mtim:    syscall.Timespec{Sec: 1570103664, Nsec: 708232917},
          - 			Ctim:    syscall.Timespec{Sec: 1570103664, Nsec: 708232917},
          - 		},
          - 	},
          + 	&{
          + 		name: "gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_84b0614b9f84c94c0773ef49ae868d0b.png",
          + 		size: 58776,
          + 		mode: 420,
          + 		modTime: time.Time{
          + 			wall: 0x255280f2,
          + 			ext:  63705700209,
          + 			loc: &time.Location{
          + 				name:       "Local",
          + 				zone:       []time.zone{{name: "UTC"}},
          + 				tx:         []time.zoneTrans{{when: -9223372036854775808}},
          + 				cacheStart: -9223372036854775808,
          + 				cacheEnd:   9223372036854775807,
          + 				cacheZone:  ⟪0x400053e220⟫,
          + 			},
          + 		},
          + 		sys: syscall.Stat_t{
          + 			Dev:     0x29,
          + 			Ino:     0x22d5ce,
          + 			Mode:    0x81a4,
          + 			Nlink:   0x01,
          + 			Uid:     0x0b88,
          + 			Gid:     0x03f1,
          + 			Size:    58776,
          + 			Blksize: 4096,
          + 			Blocks:  120,
          + 			Atim:    syscall.Timespec{Sec: 1570103409, Nsec: 626163954},
          + 			Mtim:    syscall.Timespec{Sec: 1570103409, Nsec: 626163954},
          + 			Ctim:    syscall.Timespec{Sec: 1570103409, Nsec: 626163954},
          + 		},
          + 	},
            )
        got:
          &os.fileStat{
              name:    "gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_84b0614b9f84c94c0773ef49ae868d0b.png",
              size:    58791,
              mode:    0x1b4,
              modTime: time.Time{
                  wall: 0x2a36c6d5,
                  ext:  63705700464,
                  loc:  &time.Location{
                      name: "Local",
                      zone: {
                          {name:"UTC", offset:0, isDST:false},
                      },
                      tx: {
                          {when:-9223372036854775808, index:0x0, isstd:false, isutc:false},
                      },
                      cacheStart: -9223372036854775808,
                      cacheEnd:   9223372036854775807,
                      cacheZone:  &time.zone{(CYCLIC REFERENCE)},
                  },
              },
              sys: syscall.Stat_t{
                  Dev:               0x29,
                  Ino:               0x23341e,
                  Mode:              0x81b4,
                  Nlink:             0x1,
                  Uid:               0xb88,
                  Gid:               0x3f1,
                  Rdev:              0x0,
                  X__pad1:           0x0,
                  Size:              58791,
                  Blksize:           4096,
                  X__pad2:           0,
                  Blocks:            120,
                  Atim:              syscall.Timespec{Sec:1570103664, Nsec:708232917},
                  Mtim:              syscall.Timespec{Sec:1570103664, Nsec:708232917},
                  Ctim:              syscall.Timespec{Sec:1570103664, Nsec:708232917},
                  X__glibc_reserved: {0, 0},
              },
          }
        want:
          &os.fileStat{
              name:    "gohugoio24_huc57dd738f4724f4b341121e66fd85555_267952_84b0614b9f84c94c0773ef49ae868d0b.png",
              size:    58776,
              mode:    0x1a4,
              modTime: time.Time{
                  wall: 0x255280f2,
                  ext:  63705700209,
                  loc:  &time.Location{
                      name: "Local",
                      zone: {
                          {name:"UTC", offset:0, isDST:false},
                      },
                      tx: {
                          {when:-9223372036854775808, index:0x0, isstd:false, isutc:false},
                      },
                      cacheStart: -9223372036854775808,
                      cacheEnd:   9223372036854775807,
                      cacheZone:  &time.zone{(CYCLIC REFERENCE)},
                  },
              },
              sys: syscall.Stat_t{
                  Dev:               0x29,
                  Ino:               0x22d5ce,
                  Mode:              0x81a4,
                  Nlink:             0x1,
                  Uid:               0xb88,
                  Gid:               0x3f1,
                  Rdev:              0x0,
                  X__pad1:           0x0,
                  Size:              58776,
                  Blksize:           4096,
                  X__pad2:           0,
                  Blocks:            120,
                  Atim:              syscall.Timespec{Sec:1570103409, Nsec:626163954},
                  Mtim:              syscall.Timespec{Sec:1570103409, Nsec:626163954},
                  Ctim:              syscall.Timespec{Sec:1570103409, Nsec:626163954},
                  X__glibc_reserved: {0, 0},
              },
          }
        stack:
          /<<PKGBUILDDIR>>/obj-aarch64-linux-gnu/src/github.com/gohugoio/hugo/resources/image_test.go:543
            c.Assert(fi1, eq, fi2)
        
FAIL
FAIL	github.com/gohugoio/hugo/resources	48.819s

Archived build logs in case they go away in the future:

@anthonyfok anthonyfok self-assigned this Oct 3, 2019
@bep

This comment has been minimized.

Copy link
Member

@bep bep commented Oct 3, 2019

I'm totally guessing here, but I assume this is the PNG header that is slightly different (maybe it has some platform info encoded?). If that is the case, we should consider making the tests more lenient.

@anthonyfok

This comment has been minimized.

Copy link
Contributor Author

@anthonyfok anthonyfok commented Oct 4, 2019

Thank you for the idea regarding potential difference in PNG header, which prompted me to experiment with various PNG comparison tools such as pngmeta, pngcheck, etc., and eventually, perceptualdiff (http://pdiff.sourceforge.net/, https://github.com/myint/perceptualdiff). According to this tool, all images are visually identical except these four:

gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_300x200_fill_gaussian_smart1_2.png
FAIL: Images are visibly different
1848 pixels are different
gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_4c320010919da2d8b63ed24818b4d8e1.png
FAIL: Images are visibly different
1233 pixels are different
gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_9d4c2220235b3c2d9fa6506be571560f.png
FAIL: Images are visibly different
1418 pixels are different
gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_c74bb417b961e09cf1aac2130b7b9b85.png
FAIL: Images are visibly different
10953 pixels are different

This is how the last image from above look like:

amd64:
gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_c74bb417b961e09cf1aac2130b7b9b85

arm64, ppc64el (ppc64le), s390x:
gohugoio8_hu7f72c00afdf7634587afaa5eff2a25b2_73538_c74bb417b961e09cf1aac2130b7b9b85

Difference:
perceptualdiff

Feeling perplexed, I tried my luck by browsing https://github.com/disintegration/gift more carefully, and noticed the message "fix tests on arm64, ppc64le, s390x" beside colors_test.go, which led me to disintegration/gift#20 where Grigory (@disintegration) explained:

The Go compiler now implements a fused multiply and add (FMA) instruction on some architectures, which leads to floating-point rounding differences compared to amd64.

https://golang.org/ref/spec#Floating_point_operators

I've modified the golden tests by adding a tolerance when comparing two images on architectures other than amd64. As for for brightness test, I've adjusted the input to avoid rounding differences.

and to disintegration/gift@a999ff8 where Grigory solved with a new goldenEqual function:

// goldenEqual compares two NRGBA images. It is used in golden tests only.
// All the golden images are generated on amd64 architecture. Due to differences
// in floating-point rounding on different architectures, we need to add some
// level of tolerance when comparing images on architectures other than amd64.
// See https://golang.org/ref/spec#Floating_point_operators for information on
// fused multiply and add (FMA) instruction.

which I guess could be adapted for Hugo to handle most of the golden images, maybe except some of the 8-bit colour images where tiny differences become visible differences due to dithering.

Aside: It seems golang.org/x/image/vector ran into the same issue two years ago, see https://groups.google.com/forum/#!topic/golang-dev/Sti0bl2xUXQ

Another aside: It appears that ppc64le and s390x have the same FMA implementation as they produce identical PNG images, whereas arm64's FMA results in slightly different results in 4 PNG images.

@anthonyfok anthonyfok changed the title TestImageOperationsGolden: arm64, ppc64le, s390x generate images of different file size TestImageOperationsGolden: arm64, ppc64le, s390x generate slightly different PNGs due to "fused multiply and add" (FMA) instruction Oct 4, 2019
anthonyfok added a commit to anthonyfok/hugo that referenced this issue Oct 7, 2019
In TestImageOperationsGolden, tolerate slight floating-point rounding
differences due to the use or non-use of "fused multiply and add" (FMA)
instruction on different architectures.

Special thanks to @disintegration for the solution in goldenEqual();
see disintegration/gift#20

Fixes gohugoio#6387
@anthonyfok anthonyfok removed the InProgress label Oct 7, 2019
@bep bep closed this in #6396 Oct 7, 2019
bep added a commit that referenced this issue Oct 7, 2019
In TestImageOperationsGolden, tolerate slight floating-point rounding
differences due to the use or non-use of "fused multiply and add" (FMA)
instruction on different architectures.

Special thanks to @disintegration for the solution in goldenEqual();
see disintegration/gift#20

Fixes #6387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.