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

image/draw: Possible precision loss for NRGBA images #51893

Closed
nvinuesa opened this issue Mar 23, 2022 · 3 comments
Closed

image/draw: Possible precision loss for NRGBA images #51893

nvinuesa opened this issue Mar 23, 2022 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nvinuesa
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.18 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nicolas/.cache/go-build"
GOENV="/home/nicolas/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/nicolas/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/nicolas/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2624768179=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I simply created an NRGBA image with a single pixel (with a small value color (1 in uint8) and an alpha less than 0.5 (127 in uint8)), and tried to copy it using it as source of draw.Draw():

package main

import (
	"image"
	"image/color"
	"image/draw"
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestImgDrawNRGBA(t *testing.T) {
	pixel := color.NRGBA{1, 0, 0, 127}
	expected := image.NewNRGBA(image.Rect(0, 0, 1, 1))
	expected.SetNRGBA(0, 0, pixel)

	actual := image.NewNRGBA(expected.Rect)
	draw.Draw(actual, expected.Rect, expected, image.Point{0, 0}, draw.Src)
	assert.Equal(t, expected, actual)
}

Go playground link: https://go.dev/play/p/IYelc0epkM7

What did you expect to see?

I expected the test to pass because this test passes using go-1.17.8 (Go playground link: https://go.dev/play/p/IYelc0epkM7?v=goprev).

What did you see instead?

Instead I saw a difference between the original image (expected) and the actual one (drawn using draw.Draw()):

Diff:
--- Expected
+++ Actual
@@ -2,3 +2,3 @@
	Pix: ([]uint8) (len=4) {
-  00000000  01 00 00 7f                                       |....|
+  00000000  00 00 00 7f                                       |....|
	},

Even though this is a non-alpha-premultiplied image, it looks like a possible precision loss due to the value of the alpha channel: if I use an alpha higher or equal than 0.5 (128 in uint8) the test passes.

@nvinuesa
Copy link
Author

After some investigation, I found that when the operation is draw.Src (image copy) in https://github.com/golang/go/blob/master/src/image/draw/draw.go#L251 the destination image is written using .SetRGBA64():

dst0.SetRGBA64(x, y, src0.RGBA64At(sx, sy))

regardless the image type, which in our case is NRGBA.

First, the RGBA64 color is retrieved from the source image, therefore each channel is multiplied by the alpha channel.

Then, the retrieved RGBA64 color is passed to (p *NRGBA) SetRGBA64(x, y int, c color.RGBA64), and therefore the alpha channel needs to be de-multiplied (this is done on https://github.com/golang/go/blob/master/src/image/image.go#L394).

After having each channel de-multiplied, a shift is performed in order to go from uint16 (of color.RGBA64) to uint8.

These steps seem to be responsible for a possible precision loss.

@nvinuesa nvinuesa changed the title Possible precision loss in image/draw for NRGBA images image/draw: Possible precision loss for NRGBA images Mar 23, 2022
@mknyszek
Copy link
Contributor

There seems to have been some movement in the image/draw package in 1.18, so my first instinct is that this is probably a regression.

CC @nigeltao

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 23, 2022
@mknyszek mknyszek added this to the Backlog milestone Mar 23, 2022
@gopherbot
Copy link

Change https://go.dev/cl/396795 mentions this issue: image/draw: have draw.Src preserve NRGBA colors

@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Apr 5, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 5, 2022
@golang golang locked and limited conversation to collaborators Apr 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants