code review 5177047: image/tiff: Implement PackBits decoding.

image/tiff: Implement PackBits decoding.

The decompression routine is in its own file because
G3 encoding (which is more complicated) will be put
there.

[bsiegert, 2011-10-09 20:34:46]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[nigeltao, 2011-10-10 03:26:20]

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/Makefile
File src/pkg/image/tiff/Makefile (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/Makefile#...
src/pkg/image/tiff/Makefile:10: compress.go\
If this file only speaks PackBits, and not other TIFF compression formats such
as LZW, then I'd call the file packbits.go.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress.go
File src/pkg/image/tiff/compress.go (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:10: // uncompressed data.
Can you also add a reference that specifies the file format? The Wikipedia
article points to an old apple.com page, but that's currently down for me.

Also, Wikipedia gives this example input: FE AA 02 80 00 2A FD AA 03 80 00 2A 22
F7 AA. It might be worth having a simple packbits_test.go test.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:11: func unpackBits(r io.Reader) (dst []byte) {
This function should return ([]byte, os.Error).

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:16: _, err := r.Read(buf[0:1])
If you're going to read a byte at a time, you should really sniff for
io.ByteReader, and use bufio.NewReader if r doesn't have it.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:20: code := int8(buf[0])
I'd write:
x := int(int8(buf[0]))

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:23: n, _ := r.Read(buf[0 : int(code)+1])
Read can return fewer bytes than requested. You should use io.ReadFull.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:25: dst = append(dst, buf[0:n]...)
I'd s/0//.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:30: // noop
Everything under src/pkg/image spells it "No-op."

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:36: dst = append(dst, buf[0])
Calling append each time through the loop seems inefficient. Instead:
n := 1-int(code)
for i, c := 1, buf[0]; i < n; i++ {
  buf[i] = c
}
dst = append(dst, buf[:n])

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
File src/pkg/image/tiff/reader_test.go (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:34: var decompressTests = []string{
I'd move this into TestDecompress:
----
func TestDecompress(t *testing.T) {
  var testCases = []string {
    "bw-deflate.tiff",
    "bw-packbits.tiff",
  }
  for _, tc := range testCases {
    etc.
  }
}
----

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:39: // TestDecompress reads the same image
with different compressions
TestDecompress tests that decoding some TIFF images that use different
compression formats result in the same pixel data.

Also, can we make the 0th image video-001-uncompressed.tiff?

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:42: var compareTo image.Image
I'd call the two images img0 and img1, or just m0 and m1, instead of compareTo
and img.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:47: }
Add a "defer f.Close()" after this.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:53: } else {
Add a "continue" above this line, drop the else, and most importantly, outdent
the rest of this block.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:61: t.Fatalf("wrong image size: want %d, %d,
%d, %d; got %d, %d, %d, %d", compareTo.Bounds().Min.X, compareTo.Bounds().Min.Y,
compareTo.Bounds().Max.X, compareTo.Bounds().Max.Y, b.Min.X, b.Min.Y, b.Max.X,
b.Max.Y)
No need for all this repetition. Just use "%s" and "m0.Bounds()" instead of "%d,
%d, %d, %d" and "compareTo.Bounds().Min.X, etc".

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:68: t.Fatalf("pixel at (%d, %d) has wrong
color: want %d, %d, %d, %d; got %d, %d, %d, %d", x, y, rr, gg, bb, aa, r, g, b,
a)
Again, you don't have to be so repetitive. I think you can do "%v" and "c0"
instead of "%d, %d, %d, %d", "rr, gg, bb, aa", if you earlier do "c0 := m0.At(x,
y)".

[bsiegert, 2011-10-11 21:09:02]

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

Please take another look.

[bsiegert, 2011-10-11 21:09:33]

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/Makefile
File src/pkg/image/tiff/Makefile (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/Makefile#...
src/pkg/image/tiff/Makefile:10: compress.go\
On 2011/10/10 03:26:20, nigeltao wrote:
> If this file only speaks PackBits, and not other TIFF compression formats such
> as LZW, then I'd call the file packbits.go.

I want to add G3 decoding in a later CL so I prefer this name. If you want, I
can name the file packbits.go and maybe rename it later. What do you think?

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress.go
File src/pkg/image/tiff/compress.go (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:10: // uncompressed data.
On 2011/10/10 03:26:20, nigeltao wrote:
> Can you also add a reference that specifies the file format? The Wikipedia
> article points to an old http://apple.com page, but that's currently down for
me.
> 
> Also, Wikipedia gives this example input: FE AA 02 80 00 2A FD AA 03 80 00 2A
22
> F7 AA. It might be worth having a simple packbits_test.go test.

Added a reference to the description in the TIFF spec. The test you mention is
now in reader_test.go.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:11: func unpackBits(r io.Reader) (dst []byte) {
On 2011/10/10 03:26:20, nigeltao wrote:
> This function should return ([]byte, os.Error).

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:16: _, err := r.Read(buf[0:1])
On 2011/10/10 03:26:20, nigeltao wrote:
> If you're going to read a byte at a time, you should really sniff for
> io.ByteReader, and use bufio.NewReader if r doesn't have it.

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:20: code := int8(buf[0])
On 2011/10/10 03:26:20, nigeltao wrote:
> I'd write:
> x := int(int8(buf[0]))

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:23: n, _ := r.Read(buf[0 : int(code)+1])
On 2011/10/10 03:26:20, nigeltao wrote:
> Read can return fewer bytes than requested. You should use io.ReadFull.

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:25: dst = append(dst, buf[0:n]...)
On 2011/10/10 03:26:20, nigeltao wrote:
> I'd s/0//.

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:30: // noop
On 2011/10/10 03:26:20, nigeltao wrote:
> Everything under src/pkg/image spells it "No-op."

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:36: dst = append(dst, buf[0])
On 2011/10/10 03:26:20, nigeltao wrote:
> Calling append each time through the loop seems inefficient. Instead:
> n := 1-int(code)
> for i, c := 1, buf[0]; i < n; i++ {
>   buf[i] = c
> }
> dst = append(dst, buf[:n])

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
File src/pkg/image/tiff/reader_test.go (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:34: var decompressTests = []string{
On 2011/10/10 03:26:20, nigeltao wrote:
> I'd move this into TestDecompress:
> ----
> func TestDecompress(t *testing.T) {
>   var testCases = []string {
>     "bw-deflate.tiff",
>     "bw-packbits.tiff",
>   }
>   for _, tc := range testCases {
>     etc.
>   }
> }
> ----

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:39: // TestDecompress reads the same image
with different compressions
On 2011/10/10 03:26:20, nigeltao wrote:
> TestDecompress tests that decoding some TIFF images that use different
> compression formats result in the same pixel data.

Done.

> Also, can we make the 0th image video-001-uncompressed.tiff?

No. The images are bilevel (b/w) because PackBits works only on greyscale
images, and G3 (to be done) works only on bilevel images.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:42: var compareTo image.Image
On 2011/10/10 03:26:20, nigeltao wrote:
> I'd call the two images img0 and img1, or just m0 and m1, instead of compareTo
> and img.

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:47: }
On 2011/10/10 03:26:20, nigeltao wrote:
> Add a "defer f.Close()" after this.

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:53: } else {
On 2011/10/10 03:26:20, nigeltao wrote:
> Add a "continue" above this line, drop the else, and most importantly, outdent
> the rest of this block.

Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:61: t.Fatalf("wrong image size: want %d, %d,
%d, %d; got %d, %d, %d, %d", compareTo.Bounds().Min.X, compareTo.Bounds().Min.Y,
compareTo.Bounds().Max.X, compareTo.Bounds().Max.Y, b.Min.X, b.Min.Y, b.Max.X,
b.Max.Y)
On 2011/10/10 03:26:20, nigeltao wrote:
> No need for all this repetition. Just use "%s" and "m0.Bounds()" instead of
"%d,
> %d, %d, %d" and "compareTo.Bounds().Min.X, etc".

Very nice! Done.

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:68: t.Fatalf("pixel at (%d, %d) has wrong
color: want %d, %d, %d, %d; got %d, %d, %d, %d", x, y, rr, gg, bb, aa, r, g, b,
a)
On 2011/10/10 03:26:20, nigeltao wrote:
> Again, you don't have to be so repetitive. I think you can do "%v" and "c0"
> instead of "%d, %d, %d, %d", "rr, gg, bb, aa", if you earlier do "c0 :=
m0.At(x,
> y)".

Done.

[nigeltao, 2011-10-12 09:59:54]

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/Makefile
File src/pkg/image/tiff/Makefile (right):

http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/Makefile#...
src/pkg/image/tiff/Makefile:10: compress.go\
On 2011/10/11 21:09:33, bsiegert wrote:
> On 2011/10/10 03:26:20, nigeltao wrote:
> > If this file only speaks PackBits, and not other TIFF compression formats
such
> > as LZW, then I'd call the file packbits.go.
> 
> I want to add G3 decoding in a later CL so I prefer this name. If you want, I
> can name the file packbits.go and maybe rename it later. What do you think?

OK, compress.go is fine.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress.go
File src/pkg/image/tiff/compress.go (right):

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:18: // unPackBits decodes the PackBits-compressed
data in src and returns the
"unPackBits" differs from the actual name "unpackBits".

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:33: switch err {
This looks unusual. I'd write:
if err != nil {
  if err == os.EOF {
    return dst, nil
  }
  return nil, err
}

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:44: n, err := br.Read(buf[:code+1])
Use io.ReadFull.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:45: if n > 0 {
if err != nil {
  return nil, err
}
dst = append(dst, buf[:n]...)

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:54: return dst, err
The idiom is that if you return a non-nil error, all other return values are
zero. Hence: "return nil, err".

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
File src/pkg/image/tiff/reader_test.go (right):

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:12: "bytes"
Keep imports sorted.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:46: buf, err :=
unpackBits(bytes.NewBufferString(u.compressed))
Use a strings.NewReader.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:79: img0, err := Decode(f)
I would name the uncompressed image img0 and this one img1.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:86: t.Fatalf("wrong image size: want %s, got
%s", b, img1.Bounds())
Sorry, I meant "%v" instead of "%s".

[bsiegert, 2011-10-12 16:09:37]

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

Please take another look.

[bsiegert, 2011-10-12 16:09:49]

Thank you for the review.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress.go
File src/pkg/image/tiff/compress.go (right):

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:18: // unPackBits decodes the PackBits-compressed
data in src and returns the
On 2011/10/12 09:59:54, nigeltao wrote:
> "unPackBits" differs from the actual name "unpackBits".

Sorry, sloppy editing. Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:33: switch err {
On 2011/10/12 09:59:54, nigeltao wrote:
> This looks unusual. I'd write:
> if err != nil {
>   if err == os.EOF {
>     return dst, nil
>   }
>   return nil, err
> }

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:44: n, err := br.Read(buf[:code+1])
On 2011/10/12 09:59:54, nigeltao wrote:
> Use io.ReadFull.

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:45: if n > 0 {
On 2011/10/12 09:59:54, nigeltao wrote:
> if err != nil {
>   return nil, err
> }
> dst = append(dst, buf[:n]...)

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/compress....
src/pkg/image/tiff/compress.go:54: return dst, err
On 2011/10/12 09:59:54, nigeltao wrote:
> The idiom is that if you return a non-nil error, all other return values are
> zero. Hence: "return nil, err".

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
File src/pkg/image/tiff/reader_test.go (right):

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:12: "bytes"
On 2011/10/12 09:59:54, nigeltao wrote:
> Keep imports sorted.

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:46: buf, err :=
unpackBits(bytes.NewBufferString(u.compressed))
On 2011/10/12 09:59:54, nigeltao wrote:
> Use a strings.NewReader.

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:79: img0, err := Decode(f)
On 2011/10/12 09:59:54, nigeltao wrote:
> I would name the uncompressed image img0 and this one img1.

Done.

http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_te...
src/pkg/image/tiff/reader_test.go:86: t.Fatalf("wrong image size: want %s, got
%s", b, img1.Bounds())
On 2011/10/12 09:59:54, nigeltao wrote:
> Sorry, I meant "%v" instead of "%s".

But %s works and calls the String() method:

--- FAIL: tiff.TestDecompress (0.00 seconds)
	wrong image size: want (0,0)-(153,55), got (0,0)-(153,55)
FAIL

(this is after inverting the test temporarily, obviously.)

[nigeltao, 2011-10-13 02:29:20]

LGTM.

[nigeltao, 2011-10-13 02:31:36]

*** Submitted as http://code.google.com/p/go/source/detail?r=1c95f8ab57bc ***

image/tiff: Implement PackBits decoding.

The decompression routine is in its own file because
G3 encoding (which is more complicated) will be put
there.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/5177047

Committer: Nigel Tao <nigeltao@golang.org>