code review 5528071: graphics/convolve: expose ConvolvePoint

graphics/convolve: expose ConvolvePoint

[crawshaw1, 2012-01-11 03:27:51]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://crawshaw%40google.com@code.google.com/p/graphics-go/

[nigeltao, 2012-01-11 05:47:28]

API looks reasonable. I feel like there exists a nice design somewhere that
unifies all these image transformations, but I'm happy to get there by
evolutionary steps.

http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile
File graphics/convolve/Makefile (right):

http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile#ne...
graphics/convolve/Makefile:10: separable.go\
Forget to "hg add separable.go" (and possibly separable_test.go)?

[crawshaw1, 2012-01-15 23:42:20]

PTAL

http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile
File graphics/convolve/Makefile (right):

http://codereview.appspot.com/5528071/diff/4001/graphics/convolve/Makefile#ne...
graphics/convolve/Makefile:10: separable.go\
On 2012/01/11 05:47:28, nigeltao wrote:
> Forget to "hg add separable.go" (and possibly separable_test.go)?

Done.

[nigeltao, 2012-01-16 05:18:09]

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.go
File graphics/convolve/convolve_test.go (right):

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test....
graphics/convolve/convolve_test.go:26: desc: "identity",
k doesn't look like an identity matrix to me.

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test....
graphics/convolve/convolve_test.go:145: []float64{0, 1, 0})
Add a ",\n" before the ")".

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go
File graphics/convolve/separable.go (right):

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n...
graphics/convolve/separable.go:15: // kernel. x and x are the per-axis weights.
Each slice must be the same
s/x and x/x and y/

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n...
graphics/convolve/separable.go:21: //		[]float64{1, 2, 1})
Add a ",\n" before the ")".

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n...
graphics/convolve/separable.go:183: buf := k.buildBuf(bounds, src)
So, if the src is 1000x1000, you'll build a 1000x1000 buffer even if the kernel
is 3x3?

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test.go
File graphics/convolve/separable_test.go (right):

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test...
graphics/convolve/separable_test.go:19: []float64{0, 1, 0})
Add a ",\n" before the ")".

[crawshaw1, 2012-01-26 23:27:45]

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test.go
File graphics/convolve/convolve_test.go (right):

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test....
graphics/convolve/convolve_test.go:26: desc: "identity",
On 2012/01/16 05:18:10, nigeltao wrote:
> k doesn't look like an identity matrix to me.

Done.

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/convolve_test....
graphics/convolve/convolve_test.go:145: []float64{0, 1, 0})
On 2012/01/16 05:18:10, nigeltao wrote:
> Add a ",\n" before the ")".

Done.

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go
File graphics/convolve/separable.go (right):

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n...
graphics/convolve/separable.go:15: // kernel. x and x are the per-axis weights.
Each slice must be the same
On 2012/01/16 05:18:10, nigeltao wrote:
> s/x and x/x and y/

Done.

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n...
graphics/convolve/separable.go:21: //		[]float64{1, 2, 1})
On 2012/01/16 05:18:10, nigeltao wrote:
> Add a ",\n" before the ")".

Done.

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable.go#n...
graphics/convolve/separable.go:183: buf := k.buildBuf(bounds, src)
On 2012/01/16 05:18:10, nigeltao wrote:
> So, if the src is 1000x1000, you'll build a 1000x1000 buffer even if the
kernel
> is 3x3?

Yes, ConvolvePoint on a separable kernel is not a good idea.

I could implement this as a traditional convolution (i.e. build the full
matrix). Another possibility is designing it into the API:

interface Kernel {
  With(image.Image) Convolve
}

interface Convolve {
  Convolve(dst draw.Image)
  ConvolvePoint(x, y int) color.Color)
}

Not sure if it pulls its weight.

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test.go
File graphics/convolve/separable_test.go (right):

http://codereview.appspot.com/5528071/diff/3/graphics/convolve/separable_test...
graphics/convolve/separable_test.go:19: []float64{0, 1, 0})
On 2012/01/16 05:18:10, nigeltao wrote:
> Add a ",\n" before the ")".

Done.

[nigeltao, 2012-01-30 06:14:56]

LGTM.

http://codereview.appspot.com/5528071/diff/12001/graphics/convolve/convolve.go
File graphics/convolve/convolve.go (right):

http://codereview.appspot.com/5528071/diff/12001/graphics/convolve/convolve.g...
graphics/convolve/convolve.go:38: RGBA(dst, src *image.RGBA)
I think that the methods should be called ConvolveRGBA and ConvolvePointRGBA.
The type should probably also be called KernelRGBA.