code review 107330043: go.image/vp8: implement the normal filter.

go.image/vp8: implement the normal filter.

The testdata was generated via:
cwebp foo.png -o foo.lossy.webp
dwebp -pgm foo.lossy.webp -o tmp.pgm
convert tmp.pgm foo.lossy.webp.ycbcr.png
rm tmp.pgm

[nigeltao, 2014-06-24 04:39:29]

Hello r@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.image

[r, 2014-06-24 17:42:23]

https://codereview.appspot.com/107330043/diff/40001/vp8/filter.go
File vp8/filter.go (right):

https://codereview.appspot.com/107330043/diff/40001/vp8/filter.go#newcode39
vp8/filter.go:39: if int(lutAbs[lutAbsBase+p3-p2]) > ilevel ||
since the type of lutAbs is less than the type of int, would it make sense to
declare an ilevel and hlevel of that type and avoid all the conversions? (it
won't work if their values can be out of range.) i doubt the generated code
would be better (although it might), but the source would be prettier.

https://codereview.appspot.com/107330043/diff/40001/vp8/filter.go#newcode347
vp8/filter.go:347: var lutClamp127 = [1020 + 1 + 1020]int8{
is this worth a table? it's pretty monotonous. would a function do better? it
would inline. you'd be swapping index bounds checks for value branches here.

https://codereview.appspot.com/107330043/diff/40001/webp/decode_test.go
File webp/decode_test.go (right):

https://codereview.appspot.com/107330043/diff/40001/webp/decode_test.go#newco...
webp/decode_test.go:37: "yellow_rose",
in codereview, all the new images do not render. is that OK?

[nigeltao, 2014-06-25 00:04:27]

https://codereview.appspot.com/107330043/diff/40001/vp8/filter.go
File vp8/filter.go (right):

https://codereview.appspot.com/107330043/diff/40001/vp8/filter.go#newcode39
vp8/filter.go:39: if int(lutAbs[lutAbsBase+p3-p2]) > ilevel ||
On 2014/06/24 17:42:22, r wrote:
> since the type of lutAbs is less than the type of int, would it make sense to
> declare an ilevel and hlevel of that type and avoid all the conversions? (it
> won't work if their values can be out of range.) i doubt the generated code
> would be better (although it might), but the source would be prettier.

This will probably all be moot when I eliminate the tables in a follow-up CL.
(See below).

https://codereview.appspot.com/107330043/diff/40001/vp8/filter.go#newcode347
vp8/filter.go:347: var lutClamp127 = [1020 + 1 + 1020]int8{
On 2014/06/24 17:42:22, r wrote:
> is this worth a table? it's pretty monotonous. would a function do better? it
> would inline. you'd be swapping index bounds checks for value branches here.

The C code uses a table, but C isn't bounds-checked. You're probably right that
a function would be better, but I'd rather do that as a follow-up CL.

https://codereview.appspot.com/107330043/diff/40001/webp/decode_test.go
File webp/decode_test.go (right):

https://codereview.appspot.com/107330043/diff/40001/webp/decode_test.go#newco...
webp/decode_test.go:37: "yellow_rose",
On 2014/06/24 17:42:22, r wrote:
> in codereview, all the new images do not render. is that OK?

Yeah, I think that that's OK.

[nigeltao, 2014-06-26 03:29:44]

*** Submitted as
https://code.google.com/p/go/source/detail?r=f5be9965bb52&repo=image ***

go.image/vp8: implement the normal filter.

The testdata was generated via:
cwebp foo.png -o foo.lossy.webp
dwebp -pgm foo.lossy.webp -o tmp.pgm
convert tmp.pgm foo.lossy.webp.ycbcr.png
rm tmp.pgm

TBR=r
R=r
CC=golang-codereviews
https://codereview.appspot.com/107330043