code review 5177047: image/tiff: Implement PackBits decoding. (issue 5177047)
bsie...@gmail.com
Message:
Hello nige...@golang.org (cc: golan...@googlegroups.com),
I'd like you to review this change to
https://go.googlecode.com/hg/
Description:
image/tiff: Implement PackBits decoding.
The decompression routine is in its own file because
G3 encoding (which is more complicated) will be put
there.
Please review this at http://codereview.appspot.com/5177047/
Affected files:
M src/pkg/image/tiff/Makefile
A src/pkg/image/tiff/compress.go
M src/pkg/image/tiff/reader.go
M src/pkg/image/tiff/reader_test.go
A src/pkg/image/tiff/testdata/bw-deflate.tiff
A src/pkg/image/tiff/testdata/bw-packbits.tiff
Index: src/pkg/image/tiff/Makefile
===================================================================
--- a/src/pkg/image/tiff/Makefile
+++ b/src/pkg/image/tiff/Makefile
@@ -7,6 +7,7 @@
TARG=image/tiff
GOFILES=\
buffer.go\
+ compress.go\
consts.go\
reader.go\
Index: src/pkg/image/tiff/compress.go
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/pkg/image/tiff/compress.go
@@ -0,0 +1,41 @@
+// Copyright 2011 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package tiff
+
+import "io"
+
+// unPackBits decodes the PackBits-compressed data in src and returns the
+// uncompressed data.
+func unpackBits(r io.Reader) (dst []byte) {
+ buf := make([]byte, 128)
+ dst = make([]byte, 0, 1024)
+
+ for {
+ _, err := r.Read(buf[0:1])
+ if err != nil {
+ return
+ }
+ code := int8(buf[0])
+ switch {
+ case code >= 0:
+ n, _ := r.Read(buf[0 : int(code)+1])
+ if n > 0 {
+ dst = append(dst, buf[0:n]...)
+ } else {
+ return
+ }
+ case code == -128:
+ // noop
+ default:
+ if _, err := r.Read(buf[0:1]); err != nil {
+ return
+ }
+ for j := 0; j < 1-int(code); j++ {
+ dst = append(dst, buf[0])
+ }
+ }
+ }
+ return
+}
Index: src/pkg/image/tiff/reader.go
===================================================================
--- a/src/pkg/image/tiff/reader.go
+++ b/src/pkg/image/tiff/reader.go
@@ -412,6 +412,8 @@
}
d.buf, err = ioutil.ReadAll(r)
r.Close()
+ case cPackBits:
+ d.buf = unpackBits(io.NewSectionReader(d.r, offset, n))
default:
err = UnsupportedError("compression")
}
Index: src/pkg/image/tiff/reader_test.go
===================================================================
--- a/src/pkg/image/tiff/reader_test.go
+++ b/src/pkg/image/tiff/reader_test.go
@@ -5,6 +5,7 @@
package tiff
import (
+ "image"
"io/ioutil"
"os"
"testing"
@@ -30,6 +31,48 @@
}
}
+var decompressTests = []string{
+ "bw-deflate.tiff",
+ "bw-packbits.tiff",
+}
+
+// TestDecompress reads the same image with different compressions
+// and compares them to the image read first.
+func TestDecompress(t *testing.T) {
+ var compareTo image.Image
+ for _, name := range decompressTests {
+ f, err := os.Open("testdata/" + name)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if compareTo == nil {
+ compareTo, err = Decode(f)
+ if err != nil {
+ t.Fatal(err)
+ }
+ } else {
+ img, err := Decode(f)
+ if err != nil {
+ t.Fatal(err)
+ }
+ b := img.Bounds()
+ // Compare images.
+ if !b.Eq(compareTo.Bounds()) {
+ 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)
+ }
+ for y := b.Min.Y; y < b.Max.Y; y++ {
+ for x := b.Min.X; x < b.Max.X; x++ {
+ r, g, b, a := img.At(x, y).RGBA()
+ rr, gg, bb, aa := compareTo.At(x, y).RGBA()
+ if r != rr || g != gg || b != bb || a != aa {
+ 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)
+ }
+ }
+ }
+ }
+ }
+}
+
const filename = "testdata/video-001-uncompressed.tiff"
// BenchmarkDecode benchmarks the decoding of an image.
Index: src/pkg/image/tiff/testdata/bw-deflate.tiff
===================================================================
new file mode 100644
index
0000000000000000000000000000000000000000..137a0c3ef1f669b7c51a5d437160e6784675e68c
GIT binary patch
literal 594
zc%1X})M7AVWMHV6(|g&_`>=xu`-lHenXWr!99h|Xqi61tvR4aKn&g)R*f&^MrdWtB
zX_i08|41V0;QEOd4VyMUzuUTP`uC_8w?8R=|EI6dd10mdhDpm(rmeA?8Ea>2uzO;#
z*Q3b2n>(gfEnZglM0=}ayvvnylf)f!YeI{5=Pl^0eSYRhgzn0dyAKMmZakXr?0aUv
zR>~2hkByus*Is<k5|Orht}*kkBX@R|^s64Jx?{3EP?&kMg-WVcb**%{$6>vFJUJ<x
zWhYP1<qSS3U6%IB%yPpY{wqP(d3Kg@_nJ>Ewf0W<mb-HrTO;qoeF5dgg3q-4XRuF`
zvX4LB!~gBt?I>^A+gBJD?=CN^>PbCtR8D&br*AHk=SjZfZX5Zl&-<*4w08Mp?tVmV
z<FS6Hs^9F%E8H0v1Q`A?urL6F3<4OL!R(nJCX{Us#7s~&2s1<3n;IAx*r03>;ALcD
z@C4F7fC7Sy5HmS|SO`h4C=y!?%60%^aj1GtAR)!b3T8VoF)&C&*<L`l43b_AD4Pjt
zw|;1GYEiL%QgLQ#dTLRLzDsIZYEFJZYLUKszJ760vVK8!x_)MEVtT56NoHD_eo1O^
eNlIc#qJC1jZc1uePGU)_9!LUgm<QAnFbx1KVzWX3
Index: src/pkg/image/tiff/testdata/bw-packbits.tiff
===================================================================
new file mode 100644
index
0000000000000000000000000000000000000000..d59fa4aeed32c855d74bec465904dd7474243fd0
GIT binary patch
literal 890
zc$`(yPiqrF7zXfXW|C|Iu}KSR(PH-)1iRIX9+X5VXgvyEJT*zTAy#SFUDQJk(+bjq
zUqJAKcnnqWYvkxL+SWf!vg14NPHMU^JilkNGqai9+`NhB68H(co#Gf$nSN@seDsy`
z_I||qP~Ay!0woT-PYiYBePDP$)O*H4Nl3;@`JG_Y?)MqF+Jh0avHD(Togy!4-l3z~
zURXvWWBX*}D7L;0;Aiy3XRPxALyS;m`DoyV75o$^v8trXxyr0!rc(&*_zQ*AQ4YA5
zGx!b9GXp9cRQ{mo84Ws+KDECntx|&k4ym3&3NxS;=s$tGFvAIy<~D|?r{h_hKvs-S
zQK)NrOM7Pu>6u~5yPe+>_0bf@7^vADET<?bV}@`KozV#!J5g=9eN@Gm!SsKp>3YJo
z$gg4ac-D?DqCCUWMfkwl3=IdNE#x{BfBz!NoH_J3)fa73$o`b0k%i_^6jh|i*`v!Y
zq1YcSo$u0+&UPs0JRM=d*<P1~#1l$i0gEDAvq)oUZtmqdUvZ}M!&UeEQ~IRWM4tVE
z*17H+kyqyC)|{o=J9rj{Q5>}5ZrBN<B-jXFhcEYfVH7;t3*x99^maQz_f@kK21$2k
gCrH9L*={Dyp!Mcfui4&hb(46_{$dw<>^9;22gls!oB#j-
nige...@golang.org
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#newcode10
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.go#newcode10
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.go#newcode11
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.go#newcode16
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.go#newcode20
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.go#newcode23
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.go#newcode25
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.go#newcode30
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.go#newcode36
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_test.go
File src/pkg/image/tiff/reader_test.go (right):
http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_test.go#newcode34
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_test.go#newcode39
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_test.go#newcode42
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_test.go#newcode47
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_test.go#newcode53
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_test.go#newcode61
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_test.go#newcode68
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)".
bsie...@gmail.com
bsie...@gmail.com
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#newcode10
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.go#newcode10
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.go#newcode11
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.go#newcode16
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.go#newcode20
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.go#newcode23
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.go#newcode25
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.go#newcode30
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.go#newcode36
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_test.go
File src/pkg/image/tiff/reader_test.go (right):
http://codereview.appspot.com/5177047/diff/12002/src/pkg/image/tiff/reader_test.go#newcode34
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_test.go#newcode39
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_test.go#newcode42
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_test.go#newcode47
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_test.go#newcode53
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_test.go#newcode61
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_test.go#newcode68
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.
nige...@golang.org
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#newcode10
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.go#newcode18
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.go#newcode33
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.go#newcode44
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.go#newcode45
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.go#newcode54
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_test.go
File src/pkg/image/tiff/reader_test.go (right):
http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_test.go#newcode12
src/pkg/image/tiff/reader_test.go:12: "bytes"
Keep imports sorted.
http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_test.go#newcode46
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_test.go#newcode79
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_test.go#newcode86
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".
bsie...@gmail.com
bsie...@gmail.com
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.go#newcode18
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.go#newcode33
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.go#newcode44
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.go#newcode45
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.go#newcode54
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_test.go
File src/pkg/image/tiff/reader_test.go (right):
http://codereview.appspot.com/5177047/diff/15008/src/pkg/image/tiff/reader_test.go#newcode12
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_test.go#newcode46
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_test.go#newcode79
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_test.go#newcode86
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.)
nige...@golang.org
nige...@golang.org
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 <nige...@golang.org>