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/color: YcCbCr conversion to RGBA bug #11648

Closed
qeedquan opened this issue Jul 10, 2015 · 3 comments
Closed

image/color: YcCbCr conversion to RGBA bug #11648

qeedquan opened this issue Jul 10, 2015 · 3 comments
Assignees
Milestone

Comments

@qeedquan
Copy link
Contributor

@qeedquan qeedquan commented Jul 10, 2015

The image attached is decoded to color.YCbCr, if you do image encoding directly on it, ie, png.Encode(out, img) then it will generate the correct output, however, if you loop through it and use RGBA() to build the image, it will give you a garbled output as shown below.

The regression happened after this commit:

Author: Nigel Tao <nigeltao@golang.org>  2015-03-25 18:47:24
Committer: Nigel Tao <nigeltao@golang.org>  2015-03-26 18:30:55
Parent: b5c3a9e572a1257c0db47d74b45f8e03f2f91f27 (image: add image.CMYK and color.CMYK types.)
Child:  0def13ac3f5a5e9f8e5540d3f5469cd469bddfad (image/color: have CMYK.RGBA work in 16-bit color, per the Color interface.)
Branches: master, remotes/origin/dev.ssa, remotes/origin/master
Follows: go1.4beta1
Precedes: go1.5beta1

    image/color: have YCbCr.RGBA work in 16-bit color, per the Color
    interface.

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

I tried reverting the RGBA() code for YCbCr code to the original one and that works fine

package main

import (
    "image"
    "image/color"
    _ "image/jpeg"
    "image/png"
    "log"
    "os"
)

func main() {
    r, err := os.Open("start_screen.jpg")
    check(err)
    defer r.Close()

    img, _, err := image.Decode(r)
    check(err)

    b := img.Bounds()
    rgba := image.NewRGBA(b)
    for y := b.Min.Y; y < b.Max.Y; y++ {
        for x := b.Min.X; x < b.Max.X; x++ {
            r32, g32, b32, a32 := img.At(x, y).RGBA()
            c := color.RGBA{uint8(r32), uint8(g32), uint8(b32), uint8(a32)}
            rgba.SetRGBA(x, y, c)
        }
    }

    w, err := os.Create("test.png")
    check(err)
    defer w.Close()

    err = png.Encode(w, rgba)
    check(err)
}

func check(err error) {
    if err != nil {
        log.Fatal(err)
    }
}

start_screen
test

@qeedquan qeedquan changed the title image/jpeg regression color.YcCbCr conversion to RGBA bug Jul 10, 2015
@robpike robpike added this to the Go1.5 milestone Jul 10, 2015
@robpike robpike changed the title color.YcCbCr conversion to RGBA bug image/color: YcCbCr conversion to RGBA bug Jul 10, 2015
@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Jul 10, 2015

The bug is here:

r32, g32, b32, a32 := img.At(x, y).RGBA()
c := color.RGBA{uint8(r32), uint8(g32), uint8(b32), uint8(a32)}

http://golang.org/pkg/image/color/#Color says that "each value [returned by RGBA] ranges within [0, 0xFFFF]". Thus, the r32 value is effectively a 16-bit value (as a uint32).

You then say "uint8(r32)", which takes the 8 low bits, not the 8 high bits, of that 16-bit value. Instead, it should be "uint8(r32 >> 8)".

Separately, if you want to convert a YCbCr image to an RGBA image, you don't have to roll your own loop. Instead, the easiest way to do that is the "Converting an Image to RGBA" section of http://blog.golang.org/go-imagedraw-package

@nigeltao nigeltao closed this Jul 10, 2015
@qeedquan
Copy link
Contributor Author

@qeedquan qeedquan commented Jul 10, 2015

This was for a C pixel buffer, I ended up using color.RGBAModel.Convert().(color.RGBA), thanks for noticing the bug in the code.

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Jul 11, 2015

This is becoming tangential, but I don't think the C-ness of the pixel buffer makes a difference.

Separately, using color.RGBAModel is fine, but note that it pretty much just does what I recommended to do. The rgbaModel function in http://golang.org/src/image/color/color.go says:

r, g, b, a := c.RGBA()
return RGBA{uint8(r >> 8), uint8(g >> 8), uint8(b >> 8), uint8(a >> 8)}

@golang golang locked and limited conversation to collaborators Jul 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.