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/color: are the coefficients for grayModel correct? #16251

Closed
lgg opened this Issue Jul 2, 2016 · 8 comments

Comments

Projects
None yet
7 participants

@bradfitz bradfitz changed the title from Are you sure that coefficients for grayModel() in color.go are right? to image/color: are the coefficients for grayModel correct? Jul 2, 2016

@lgg

This comment has been minimized.

Show comment
Hide comment
@lgg

lgg Jul 3, 2016

The formula using to calculate relative luminance (or brightness):
Y = 0.299 R + 0.587 G + 0.114

But it is correct in NTSC (1953) color space only. This formula is deprecated and legacy.
Actual color space sRGB significantly different: chromacity diagram.

Correct formula is:
Y = 0.2126 R + 0.7152 G + 0.0722 B

The proof of numbers
Source — Bruce Lindbloom site
See also — Charles Poynton Color FAQ

A lot of work or refactoring is not required. We can simply replace this coefficients.

lgg commented Jul 3, 2016

The formula using to calculate relative luminance (or brightness):
Y = 0.299 R + 0.587 G + 0.114

But it is correct in NTSC (1953) color space only. This formula is deprecated and legacy.
Actual color space sRGB significantly different: chromacity diagram.

Correct formula is:
Y = 0.2126 R + 0.7152 G + 0.0722 B

The proof of numbers
Source — Bruce Lindbloom site
See also — Charles Poynton Color FAQ

A lot of work or refactoring is not required. We can simply replace this coefficients.

@nigeltao nigeltao added this to the Go1.8 milestone Jul 7, 2016

@nigeltao

This comment has been minimized.

Show comment
Hide comment
@nigeltao

nigeltao Jul 7, 2016

Contributor

We're currently in code freeze for the upcoming Go 1.7 release. I'll take a look at it for Go 1.8.

Contributor

nigeltao commented Jul 7, 2016

We're currently in code freeze for the upcoming Go 1.7 release. I'll take a look at it for Go 1.8.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Sep 25, 2016

Ping @nigeltao.

@quentinmit quentinmit added the NeedsFix label Oct 7, 2016

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Oct 19, 2016

Contributor
Contributor

rsc commented Oct 19, 2016

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Oct 19, 2016

Contributor

It's fine with me to change to sRGB normalization but I'm sure it will break something.

Contributor

robpike commented Oct 19, 2016

It's fine with me to change to sRGB normalization but I'm sure it will break something.

@nigeltao

This comment has been minimized.

Show comment
Hide comment
@nigeltao

nigeltao Oct 20, 2016

Contributor

Note that the NTSC formula is also used by https://github.com/golang/go/blob/master/src/image/color/ycbcr.go#L10 based on the JFIF specification.

Changing the grayModel function to use the sRGB coefficients instead of the NTSC coefficients would break the nice property that, in theory, converting an image.RGBA to an image.YCbCr and then taking only the Y channel is equivalent to converting an image.RGBA directly to an image.Gray. In practice, the two use slightly different formulae (the ycbcr.go one uses a shift instead of a divide), but we could harmonize them.

We could conceivably change the ycbcr.go formulae to do something compatible with the sRGB coefficients, but that has larger knock-on effects, and we are then also disregarding the JFIF spec.

I appreciate that there are arguments for sRGB over NTSC, but I think this is a situation where everything has trade-offs, and there being multiple competing standards, instead of there being a single 'correct' answer. I'm happy with the trade-off that the image/color package makes.

You are of course free to write your own function that takes an image.RGBA and produces an image.Gray based on the sRGB coefficients.

Sorry for the delay in replying.

Contributor

nigeltao commented Oct 20, 2016

Note that the NTSC formula is also used by https://github.com/golang/go/blob/master/src/image/color/ycbcr.go#L10 based on the JFIF specification.

Changing the grayModel function to use the sRGB coefficients instead of the NTSC coefficients would break the nice property that, in theory, converting an image.RGBA to an image.YCbCr and then taking only the Y channel is equivalent to converting an image.RGBA directly to an image.Gray. In practice, the two use slightly different formulae (the ycbcr.go one uses a shift instead of a divide), but we could harmonize them.

We could conceivably change the ycbcr.go formulae to do something compatible with the sRGB coefficients, but that has larger knock-on effects, and we are then also disregarding the JFIF spec.

I appreciate that there are arguments for sRGB over NTSC, but I think this is a situation where everything has trade-offs, and there being multiple competing standards, instead of there being a single 'correct' answer. I'm happy with the trade-off that the image/color package makes.

You are of course free to write your own function that takes an image.RGBA and produces an image.Gray based on the sRGB coefficients.

Sorry for the delay in replying.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Oct 20, 2016

CL https://golang.org/cl/31538 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 20, 2016

image/color: tweak the formula for converting to gray.
This makes grayModel and gray16Model in color.go use the exact same
formula as RGBToYCbCr in ycbcr.go. They were the same formula in theory,
but in practice the color.go versions used a divide by 1000 and the
ycbcr.go versions used a (presumably faster) shift by 16.

This implies the nice property that converting an image.RGBA to an
image.YCbCr and then taking only the Y channel is equivalent to
converting an image.RGBA directly to an image.Gray.

The difference between the two formulae is non-zero, but small:
https://play.golang.org/p/qG7oe-eqHI

Updates #16251

Change-Id: I288ecb957fd6eceb9626410bd1a8084d2e4f8198
Reviewed-on: https://go-review.googlesource.com/31538
Reviewed-by: Rob Pike <r@golang.org>
@nigeltao

This comment has been minimized.

Show comment
Hide comment
@nigeltao

nigeltao Oct 20, 2016

Contributor

That commit harmonized the two NTSC implementations. As for replacing NTSC with sRGB coefficients, as I said, I'm not convinced that we should do that.

Contributor

nigeltao commented Oct 20, 2016

That commit harmonized the two NTSC implementations. As for replacing NTSC with sRGB coefficients, as I said, I'm not convinced that we should do that.

@nigeltao nigeltao closed this Oct 20, 2016

@golang golang locked and limited conversation to collaborators Oct 20, 2017

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