code review 101670045: go.image/draw: new package, a superset of the standard ...

go.image/draw: new package, a superset of the standard library's image/draw package, including the ability to scale an image.

[nigeltao, 2014-07-04 08:52:02]

Hello r@golang.org (cc: bradfitz@golang.org, couchmoney@gmail.com, golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.image

[chai2010, 2014-07-04 10:50:33]

https://codereview.appspot.com/101670045/diff/40001/draw/scale.go File draw/scale.go (right): https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode1 draw/scale.go:1: package draw copyright missing https://codereview.appspot.com/101670045/diff/40001/draw/scale_test.go File draw/scale_test.go (right): https://codereview.appspot.com/101670045/diff/40001/draw/scale_test.go#newcode69 draw/scale_test.go:69: if !reflect.DeepEqual(got, want) { IMO, reflect.DeepEqual is not a good way to compare image. Especially, png.Decode return a non *image.RGBA type image.

[klauspost, 2014-07-06 21:05:11]

Hi! I have implemented a few scalers, so the algorithms are fairly familiar to me. Unfortunately, unless I am missing something big, the "Default" linear interpolation algorithm doesn't work for scaling to less than 50%. When optimized, a (proper) linear interpolation shouldn't be much more than 20-30% faster than a 3-tap cubic interpolation, so I'm not really sure it should be the default, since it produces much more blurry results. I would prefer a shaper (Lanczos/sinc) option, and have cubic as default. Of course, there should be no float point operations in the inner loop of a resampler, but I assume this is just to get the basics down. If I can contribute feel free to contact me: "klauspost" on gmail. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go File draw/scale.go (right): https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode103 draw/scale.go:103: func (z *defaultScaler) Scale(dst Image, dp image.Point, src image.Image, sp image.Point) { If I read this algorithm correctly, it will be broken for all scaling below 50% because your source pixels will sometimes refer to the same pixel, creating aliasing. Scaling below 50% requires you to use more than 2 pixels for interpolation, which makes this algorithm implementation useless. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode257 draw/scale.go:257: totalWeight: totalWeight, Check if totalWeight is zero - otherwise you risk a division by zero. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode277 draw/scale.go:277: tw := s.totalWeight * 0xffff Precompute this value and store the inverse. That way you only pay the cost for a 1 pixel high image and you save 4 divisions below per pixel. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode298 draw/scale.go:298: dstColorRGBA64.R = ftou(r / s.totalWeight) You should precompute 1.0/s.TotalWeight (and store is as s.InvTotalWeight), so you can do ftou(r * s.InvTotalWeight) and avoid four divisions per pixel.

[nigeltao, 2014-07-07 00:58:40]

https://codereview.appspot.com/101670045/diff/40001/draw/scale.go File draw/scale.go (right): https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode1 draw/scale.go:1: package draw On 2014/07/04 10:50:32, chai2010 wrote: > copyright missing Done. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode103 draw/scale.go:103: func (z *defaultScaler) Scale(dst Image, dp image.Point, src image.Image, sp image.Point) { On 2014/07/06 21:05:11, klauspost wrote: > If I read this algorithm correctly, it will be broken for all scaling below 50% > because your source pixels will sometimes refer to the same pixel, creating > aliasing. I don't think you're reading it correctly. This loops over destination pixels, not source pixels, so if you're minifying an image from 100x100 to 10x10, each destination pixel is still blended from 4 source pixels. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode257 draw/scale.go:257: totalWeight: totalWeight, On 2014/07/06 21:05:11, klauspost wrote: > Check if totalWeight is zero - otherwise you risk a division by zero. And if totalWeight is zero, what do you think should happen? https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode277 draw/scale.go:277: tw := s.totalWeight * 0xffff On 2014/07/06 21:05:11, klauspost wrote: > Precompute this value and store the inverse. That way you only pay the cost for > a 1 pixel high image and you save 4 divisions below per pixel. Done. https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode298 draw/scale.go:298: dstColorRGBA64.R = ftou(r / s.totalWeight) On 2014/07/06 21:05:11, klauspost wrote: > You should precompute 1.0/s.TotalWeight (and store is as s.InvTotalWeight), so > you can do ftou(r * s.InvTotalWeight) and avoid four divisions per pixel. Done. https://codereview.appspot.com/101670045/diff/40001/draw/scale_test.go File draw/scale_test.go (right): https://codereview.appspot.com/101670045/diff/40001/draw/scale_test.go#newcode69 draw/scale_test.go:69: if !reflect.DeepEqual(got, want) { On 2014/07/04 10:50:33, chai2010 wrote: > IMO, reflect.DeepEqual is not a good way to compare image. > Especially, png.Decode return a non *image.RGBA type image. The images aren't that big, reflect.DeepEqual seems fine to me. I don't expect the decoded image to change from being an *image.RGBA any time soon, and anything smarter is just more code.

[r, 2014-07-08 19:44:54]

https://codereview.appspot.com/101670045/diff/100001/draw/scale.go File draw/scale.go (right): https://codereview.appspot.com/101670045/diff/100001/draw/scale.go#newcode20 draw/scale.go:20: NicestQuality Quality = +1 not sure i like these names. they don't convey real information. i expected to see things like BiLinear and NearestNeighbor. these names may make sense, but shouldn't i (also?) have API-level control over the actual algorithm? in a sense, i want to be able to use pre-defined kernels and also inject my own kernels https://codereview.appspot.com/101670045/diff/100001/draw/scale_test.go File draw/scale_test.go (right): https://codereview.appspot.com/101670045/diff/100001/draw/scale_test.go#newco... draw/scale_test.go:19: var genScaleFiles = flag.Bool("gen_scale_files", false, "Whether to generate the TestScaleXxx golden files.") s/W/w/ https://codereview.appspot.com/101670045/diff/100001/draw/scale_test.go#newco... draw/scale_test.go:25: // resultant pixels, and in such a case, use the gen_scale_files flag to pixels. In such a case, use ...

[klauspost, 2014-07-17 09:55:18]

Sorry, didn't realize I hadn't published this. TL;DR, the suggested bilinear is broken, and will in some cases give worse results than nearest neighbor. My proposal would be to drop bilinear completely and have a fast, which is nearest neighbor and the current bicubic as default. For "best" there should be something like a gamma correct 4-tap sinc/lanczos or similar as "very good" quality. Speaking of gamma - do we know the gamma (curve) of the image we are scaling? https://codereview.appspot.com/101670045/diff/40001/draw/scale.go File draw/scale.go (right): https://codereview.appspot.com/101670045/diff/40001/draw/scale.go#newcode103 draw/scale.go:103: func (z *defaultScaler) Scale(dst Image, dp image.Point, src image.Image, sp image.Point) { On 2014/07/07 00:58:40, nigeltao wrote: > I don't think you're reading it correctly. This loops over destination pixels, > not source pixels, so if you're minifying an image from 100x100 to 10x10, each > destination pixel is still blended from 4 source pixels. Ok, I am sorry to say that I am reading it correctly, and that is wrong. I may however not have explained it very well. To do a bilinear downscale from 100 -> 10 you *need* to consider all the pixels you are currently skipping, otherwise it is nothing better than nearest neighbor. The way GPU's solve this is to have mip-maps, so you never do a downscale of more than 50%, but that wouldn't make sense here. Other than that the fractions are completely off in the same case, since you assume you can use the float point fraction as weights, which doesn't make any sense for more than 50% downscale (you are no longer just interpolating between two adjacent pixels). Have you tried doing visual comparison of downscales?

[gobot, 2014-12-19 05:06:11]

R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/101670045 is best) in the description in your new CL. Thanks very much.

[nigeltao, 2015-02-09 05:11:55]

Sorry, I got interrupted by other work and dropped this CL. Please continue the conversation on http://golang.org/cl/4210