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/png: don't ignore PNG gAMA chunk #20613

Open
Deleplace opened this Issue Jun 8, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@Deleplace

Deleplace commented Jun 8, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8.1 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/valentin/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build951360311=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Decoded fruit.png (which contains a strong gamma value in a gAMA chunk).

Encoded back to result.png.

Opened result.png in a gAMA-aware viewer (Chromium)

What did you expect to see?

A pear.

What did you see instead?

An apple.

I understand (after reading file reader_test.go and part of the PNG spec) that handling ancillary chunks (e.g. gAMA) is not a requirement for a PNG decoder. I also understand that implementing it would require some (unknown to me) amount of work. So, the question boils down to "would it be a good idea or not to take the gAMA chunk into account?".

fruit

result

@Deleplace

This comment has been minimized.

Show comment
Hide comment
@Deleplace

Deleplace Jun 8, 2017

package main

import (
	"bytes"
	"image"
	_ "image/jpeg"
	"image/png"
	"io/ioutil"
)

func main() {
	data, err1 := ioutil.ReadFile("fruit.png")
	checkerr(err1)
	buf := bytes.NewBuffer(data)
	img, _, err2 := image.Decode(buf)
	checkerr(err2)

	var temoin bytes.Buffer
	err3 := png.Encode(&temoin, img)
	checkerr(err3)
	err4 := ioutil.WriteFile("result.png", temoin.Bytes(), 0666)
	checkerr(err4)
}

func checkerr(err error) {
	if err != nil {
		panic(err)
	}
}

Deleplace commented Jun 8, 2017

package main

import (
	"bytes"
	"image"
	_ "image/jpeg"
	"image/png"
	"io/ioutil"
)

func main() {
	data, err1 := ioutil.ReadFile("fruit.png")
	checkerr(err1)
	buf := bytes.NewBuffer(data)
	img, _, err2 := image.Decode(buf)
	checkerr(err2)

	var temoin bytes.Buffer
	err3 := png.Encode(&temoin, img)
	checkerr(err3)
	err4 := ioutil.WriteFile("result.png", temoin.Bytes(), 0666)
	checkerr(err4)
}

func checkerr(err error) {
	if err != nil {
		panic(err)
	}
}

@bradfitz bradfitz changed the title from Suggestion: not ignore PNG gAMA chunk to image/png: don't ignore PNG gAMA chunk Jun 8, 2017

@bradfitz bradfitz added this to the Go1.10 milestone Jun 8, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Jun 8, 2017

/cc @nigeltao

@nigeltao

This comment has been minimized.

Show comment
Hide comment
@nigeltao

nigeltao Jun 9, 2017

Contributor

It would be a good idea, but it's also non-trivial. The right solution would probably be to add some concept to the image/color package to represent color spaces (e.g. sRGB vs Adobe RGB), including the concept of gamma. Designing that new API is probably a substantial amount of work, and not something I have time for any time soon. Sorry.

See also #11420 "x/image/draw: color space-correct interpolation".

Contributor

nigeltao commented Jun 9, 2017

It would be a good idea, but it's also non-trivial. The right solution would probably be to add some concept to the image/color package to represent color spaces (e.g. sRGB vs Adobe RGB), including the concept of gamma. Designing that new API is probably a substantial amount of work, and not something I have time for any time soon. Sorry.

See also #11420 "x/image/draw: color space-correct interpolation".

@Deleplace

This comment has been minimized.

Show comment
Hide comment
@Deleplace

Deleplace Jun 9, 2017

Hello Nigel, thanks for the reply.

Though my example involved Decode and Encode, I was more concerned about what happens in Decode, when the gAMA is encountered and ignored. How about "modifying the color values of each pixel at the end of Decode", i.e. "applying" the custom gamma, and then forget about gamma concepts?

Acknowledged, this is not the same thing as (and would be less thorough than) introducing color spaces concepts to the std image and color packages.

Deleplace commented Jun 9, 2017

Hello Nigel, thanks for the reply.

Though my example involved Decode and Encode, I was more concerned about what happens in Decode, when the gAMA is encountered and ignored. How about "modifying the color values of each pixel at the end of Decode", i.e. "applying" the custom gamma, and then forget about gamma concepts?

Acknowledged, this is not the same thing as (and would be less thorough than) introducing color spaces concepts to the std image and color packages.

@nigeltao

This comment has been minimized.

Show comment
Hide comment
@nigeltao

nigeltao Jun 10, 2017

Contributor

I'm wary of making a short-term workaround like that. If, for example, we shipped that in Go 1.10, then maintaining backwards compatibility might constrain us in doing the right solution for a later Go 1.x version.

Contributor

nigeltao commented Jun 10, 2017

I'm wary of making a short-term workaround like that. If, for example, we shipped that in Go 1.10, then maintaining backwards compatibility might constrain us in doing the right solution for a later Go 1.x version.

@MichaelTJones

This comment has been minimized.

Show comment
Hide comment
@MichaelTJones

MichaelTJones Oct 3, 2017

Contributor

I have implemented the gAMA, cHRM, and sRGB chunks in PNG's writer.go. I added constants for rendering intent, and parameters in Encoder.

// sRGB rendering intent: (https://www.w3.org/TR/PNG/#11cHRM 11.3.3.5)
type RenderingIntent byte

const (
	// Perceptual: images preferring good adaptation to the output device
	// gamut at the expense of colorimetric accuracy, such as photographs.
	Perceptual RenderingIntent = 0

	// Relative colorimetric: images requiring colour appearance matching
	// (relative to the output device white point), such as logos.
	Relative RenderingIntent = 1

	// Saturation: images preferring preservation of saturation at the expense
	// of hue and lightness, such as charts and graphs.
	Saturation RenderingIntent = 2

	// Absolute: images requiring preservation of absolute colorimetry, such
	// as previews of images destined for a different output device (proofs).
	Absolute RenderingIntent = 3
)

However, this is write-only and does not change any pixel values, it simply allows the user to have the output PNG represent the nature of the data; any gamma-encoding must have already been done before the image was supplied to the encoder. I could add the read side if desired.

It is dangerous to to the gamma encode in that commonly 8-bit components commonly need to be companded to a larger size and there is no natural place for that here. That said, I do have the code handy...

Contributor

MichaelTJones commented Oct 3, 2017

I have implemented the gAMA, cHRM, and sRGB chunks in PNG's writer.go. I added constants for rendering intent, and parameters in Encoder.

// sRGB rendering intent: (https://www.w3.org/TR/PNG/#11cHRM 11.3.3.5)
type RenderingIntent byte

const (
	// Perceptual: images preferring good adaptation to the output device
	// gamut at the expense of colorimetric accuracy, such as photographs.
	Perceptual RenderingIntent = 0

	// Relative colorimetric: images requiring colour appearance matching
	// (relative to the output device white point), such as logos.
	Relative RenderingIntent = 1

	// Saturation: images preferring preservation of saturation at the expense
	// of hue and lightness, such as charts and graphs.
	Saturation RenderingIntent = 2

	// Absolute: images requiring preservation of absolute colorimetry, such
	// as previews of images destined for a different output device (proofs).
	Absolute RenderingIntent = 3
)

However, this is write-only and does not change any pixel values, it simply allows the user to have the output PNG represent the nature of the data; any gamma-encoding must have already been done before the image was supplied to the encoder. I could add the read side if desired.

It is dangerous to to the gamma encode in that commonly 8-bit components commonly need to be companded to a larger size and there is no natural place for that here. That said, I do have the code handy...

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 15, 2017

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment