code review 162850043: go.image/riff: new package.

go.image/riff: new package.

Also update package webp to use package riff.

[nigeltao, 2014-10-20 11:14:45]

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

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

[r, 2014-10-20 19:54:26]

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go
File riff/riff.go (right):

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode48
riff/riff.go:48: type FourCC [4]byte
would this be nicer as a slice? the code that uses it has to work around it
being an array. you'd need to copy() but that's ok; a helper could do all the
work

if it stays as an array, helpers could also clean this up

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode51
riff/riff.go:51: var LIST = FourCC{'L', 'I', 'S', 'T'}
where is this used?

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode63
riff/riff.go:63: if buf[0] != 'R' || buf[1] != 'I' || buf[2] != 'F' || buf[3] !=
'F' {
too bad you can't do 
if buf[0:4] != [0:chunkHeaderSize]byte{'R', 'I', 'F', 'F'} {

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode80
riff/riff.go:80: return FourCC{}, nil, err
if fourcc was a slice, you could return nil here, not that that's important

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode145
riff/riff.go:145: chunkLen = uint32(z.buf[4]) | uint32(z.buf[5])<<8 |
uint32(z.buf[6])<<16 | uint32(z.buf[7])<<24
write a helper

[nigeltao, 2014-10-22 10:52:54]

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go
File riff/riff.go (right):

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode48
riff/riff.go:48: type FourCC [4]byte
On 2014/10/20 19:54:26, r wrote:
> would this be nicer as a slice?

It's a fixed length concept. I prefer an array.

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode51
riff/riff.go:51: var LIST = FourCC{'L', 'I', 'S', 'T'}
On 2014/10/20 19:54:26, r wrote:
> where is this used?

I added an example_test.go that uses LIST.

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode63
riff/riff.go:63: if buf[0] != 'R' || buf[1] != 'I' || buf[2] != 'F' || buf[3] !=
'F' {
On 2014/10/20 19:54:26, r wrote:
> too bad you can't do 
> if buf[0:4] != [0:chunkHeaderSize]byte{'R', 'I', 'F', 'F'} {

Acknowledged.

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode80
riff/riff.go:80: return FourCC{}, nil, err
On 2014/10/20 19:54:26, r wrote:
> if fourcc was a slice, you could return nil here, not that that's important

Acknowledged.

https://codereview.appspot.com/162850043/diff/40001/riff/riff.go#newcode145
riff/riff.go:145: chunkLen = uint32(z.buf[4]) | uint32(z.buf[5])<<8 |
uint32(z.buf[6])<<16 | uint32(z.buf[7])<<24
On 2014/10/20 19:54:26, r wrote:
> write a helper

Done.

[r, 2014-10-22 15:09:54]

LGTM

https://codereview.appspot.com/162850043/diff/160001/riff/riff.go
File riff/riff.go (right):

https://codereview.appspot.com/162850043/diff/160001/riff/riff.go#newcode31
riff/riff.go:31: func u32(b []byte) uint32 {
// u32 decodes the first four bytes of b as a little-endian integer.

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go
File webp/decode.go (right):

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode73
webp/decode.go:73: filter := (buf[0] >> 2) & 0x03
the variable isn't used after the next line so has no purpose.
however, if you plan to implement the feature and will need the variable, stet

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode78
webp/decode.go:78: compression := buf[0] & 0x03
ditto

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode162
webp/decode.go:162: animationBit    = 1 << 1
you could do
animationBit = 1 << (iota+1)
xmpMetadataBit
etc.
optional

[nigeltao, 2014-10-22 21:58:31]

https://codereview.appspot.com/162850043/diff/160001/riff/riff.go
File riff/riff.go (right):

https://codereview.appspot.com/162850043/diff/160001/riff/riff.go#newcode31
riff/riff.go:31: func u32(b []byte) uint32 {
On 2014/10/22 15:09:54, r wrote:
> // u32 decodes the first four bytes of b as a little-endian integer.

Done.

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go
File webp/decode.go (right):

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode73
webp/decode.go:73: filter := (buf[0] >> 2) & 0x03
On 2014/10/22 15:09:54, r wrote:
> the variable isn't used after the next line so has no purpose.
> however, if you plan to implement the feature and will need the variable, stet

I plan to implement, once the official test suite covers that code path. See
https://codereview.appspot.com/154350043/#msg2

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode78
webp/decode.go:78: compression := buf[0] & 0x03
On 2014/10/22 15:09:54, r wrote:
> ditto

Acknowledged.

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode162
webp/decode.go:162: animationBit    = 1 << 1
On 2014/10/22 15:09:54, r wrote:
> you could do
> animationBit = 1 << (iota+1)
> xmpMetadataBit
> etc.
> optional

I don't like using iota for wire format constants, where the actual values are
fixed regardless of whether or not we add, remove or re-order lines. I also like
the explicit values, instead of iota's implicit values, when I'm debugging the
thing and I see that the flag value is 0x10 and need to figure out what that bit
means.

[nigeltao, 2014-10-22 22:01:01]

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

go.image/riff: new package.

Also update package webp to use package riff.

LGTM=r
R=r
CC=golang-codereviews, pascal.massimino
https://codereview.appspot.com/162850043

[skal, 2014-10-29 16:01:34]

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go
File webp/decode.go (right):

https://codereview.appspot.com/162850043/diff/160001/webp/decode.go#newcode73
webp/decode.go:73: filter := (buf[0] >> 2) & 0x03
On 2014/10/22 21:58:31, nigeltao wrote:
> On 2014/10/22 15:09:54, r wrote:
> > the variable isn't used after the next line so has no purpose.
> > however, if you plan to implement the feature and will need the variable,
stet
> 
> I plan to implement, once the official test suite covers that code path. See
> https://codereview.appspot.com/154350043/#msg2

vectors for filtering methods 1/2/3 + NO_COMPRESSION cases have been added
to the test-suite thanks to: https://gerrit.chromium.org/gerrit/#/c/72033/