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

cmpimg: invalid handling of images that are not image.RGBA #715

Closed
sbinet opened this issue Aug 2, 2021 · 3 comments · Fixed by #716
Closed

cmpimg: invalid handling of images that are not image.RGBA #715

sbinet opened this issue Aug 2, 2021 · 3 comments · Fixed by #716
Assignees

Comments

@sbinet
Copy link
Member

sbinet commented Aug 2, 2021

in cmpimg, one sees:

func cmpImg(v1, v2 image.Image, delta float64) bool {
        img1, ok := v1.(*image.RGBA)
        if !ok {
                img1 = newRGBAFrom(v1)
        }

        img2, ok := v2.(*image.RGBA)
        if !ok {
                img2 = newRGBAFrom(v2)
        }

        if len(img1.Pix) != len(img2.Pix) {
                return false
        }

        max := delta * delta
        bnd := img1.Bounds()
        for x := bnd.Min.X; x < bnd.Max.X; x++ {
                for y := bnd.Min.Y; y < bnd.Max.Y; y++ {
                        c1 := img1.RGBAAt(x, y)
                        c2 := img2.RGBAAt(x, y)
                        if !yiqEqApprox(c1, c2, max) {
                                return false
                        }
                }
        }

        return ok
}

it may happen that v2 isn't a *image.RGBA.
in that case, ok==false and we convert v2 into img2 going through the necessary steps.

and if no pixel differed too much between img1 and img2, we return ok... which is still false.

@sbinet sbinet self-assigned this Aug 2, 2021
sbinet added a commit to sbinet-gonum/plot that referenced this issue Aug 2, 2021
@kortschak
Copy link
Member

ooops

sbinet added a commit that referenced this issue Aug 3, 2021
@sbinet
Copy link
Member Author

sbinet commented Aug 3, 2021

yeah... silly mistake, heh?

looking at the coverage of cmpimg.go, it would perhaps stand to reason to improve it a little bit.
WDYT?

@kortschak
Copy link
Member

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants