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