code review 13243047: go.image/tiff: encoder support Gray/Gray16 format

go.image/tiff: encoder support Gray/Gray16/Paletted format

Use these commands to generate testdata:

# TIFF Tools
tiff2bw    video-001.tiff video-001-gray.tiff
tiffmedian video-001.tiff video-001-paletted.tiff

# ImageMagick
convert -depth 16 video-001.tiff      video-001-16bit.tiff
convert -depth 16 video-001-gray.tiff video-001-gray-16bit.tiff

[chai2010, 2013-09-12 07:08:43]

Hello golang-dev@googlegroups.com,

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

[nigeltao, 2013-09-13 01:37:59]

bsiegert should also look at this.

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go#newcode83
tiff/writer.go:83: return writePix(w, pix, dy, dx*2, stride)
Is Pix in the right endianness?

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go#newcode92
tiff/writer.go:92: v1 := binary.BigEndian.Uint16(pix[i : i+2])
You use BigEndian here, but the comment above says that "We only write
little-endian TIFF files".

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go#newcode317
tiff/writer.go:317: colorMap[i] = uint32(r)
r>>8, right?

[chai2010, 2013-09-13 02:55:36]

PTAL

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go#newcode83
tiff/writer.go:83: return writePix(w, pix, dy, dx*2, stride)
On 2013/09/13 01:37:59, nigeltao wrote:
> Is Pix in the right endianness?

Done.

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go#newcode92
tiff/writer.go:92: v1 := binary.BigEndian.Uint16(pix[i : i+2])
On 2013/09/13 01:37:59, nigeltao wrote:
> You use BigEndian here, but the comment above says that "We only write
> little-endian TIFF files".

This is src image, not the dst tiff image.
And the image.Gray16 use the big-endian format.

http://golang.org/pkg/image/#Gray16
Pix holds the image's pixels, as gray values in big-endian format. The pixel at
(x, y) starts at Pix[(y-Rect.Min.Y)*Stride + (x-Rect.Min.X)*2].

https://codereview.appspot.com/13243047/diff/11001/tiff/writer.go#newcode317
tiff/writer.go:317: colorMap[i] = uint32(r)
On 2013/09/13 01:37:59, nigeltao wrote:
> r>>8, right?

TIFF 6.0 Specification page 23:
In a TIFF ColorMap, all the Red values come first, followed by the Green values,
then the Blue values. In the ColorMap, black is represented by 0,0,0 and white
is
represented by 65535, 65535, 65535.

[nigeltao, 2013-09-13 03:10:41]

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go#newcode85
tiff/writer.go:85: max := y*stride + dx
dx*2.

Tests for this new code would be nice.

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go#newcode89
tiff/writer.go:89: v1 := binary.BigEndian.Uint16(pix[i : i+2])
I'd just write
v1 := uint16(pix[i])<<8 | uint16(pix[i+1])
as the interface method call seems expensive for an inner loop. Similarly for
PutUint16.

[chai2010, 2013-09-13 03:27:37]

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go#newcode85
tiff/writer.go:85: max := y*stride + dx
On 2013/09/13 03:10:42, nigeltao wrote:
> dx*2.
> 
> Tests for this new code would be nice.

Done.

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go#newcode89
tiff/writer.go:89: v1 := binary.BigEndian.Uint16(pix[i : i+2])
On 2013/09/13 03:10:42, nigeltao wrote:
> I'd just write
> v1 := uint16(pix[i])<<8 | uint16(pix[i+1])
> as the interface method call seems expensive for an inner loop. Similarly for
> PutUint16.

Done.

[nigeltao, 2013-09-13 03:38:57]

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/16001/tiff/writer.go#newcode85
tiff/writer.go:85: max := y*stride + dx
On 2013/09/13 03:27:37, chai2010 wrote:
> On 2013/09/13 03:10:42, nigeltao wrote:
> > dx*2.
> > 
> > Tests for this new code would be nice.
> 
> Done.

Tests?

https://codereview.appspot.com/13243047/diff/17002/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/17002/tiff/writer.go#newcode93
tiff/writer.go:93: v0, v1 = v1, (v1 - v0)
The ()s are unnecessary.

[chai2010, 2013-09-13 06:05:27]

I have made a Gray/Gray16/Paletted format test.
Please take another look.

https://codereview.appspot.com/13243047/diff/17002/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/17002/tiff/writer.go#newcode93
tiff/writer.go:93: v0, v1 = v1, (v1 - v0)
On 2013/09/13 03:38:57, nigeltao wrote:
> The ()s are unnecessary.

Done.

[chai2010, 2013-09-13 06:40:28]

go test -test.bench=.

before:
BenchmarkEncode             500000            6058 ns/op       10200.80 MB/s
BenchmarkEncodePaletted     5000            689439 ns/op          22.41 MB/s
BenchmarkEncodeGray         2000            999557 ns/op          15.46 MB/s
BenchmarkEncodeGray16       2000           1141565 ns/op          27.07 MB/s
ok      code.google.com/p/go.image/tiff 16.870s

after:
BenchmarkEncode           500000              6048 ns/op       10217.67 MB/s
BenchmarkEncodePaletted   100000             25821 ns/op         598.34 MB/s
BenchmarkEncodeGray       500000              6972 ns/op        2215.88 MB/s
BenchmarkEncodeGray16      10000            122807 ns/op         251.61 MB/s
ok      code.google.com/p/go.image/tiff 15.048s

[bsiegert, 2013-09-13 06:50:19]

This look great, thanks.

https://codereview.appspot.com/13243047/diff/46001/tiff/writer_test.go
File tiff/writer_test.go (right):

https://codereview.appspot.com/13243047/diff/46001/tiff/writer_test.go#newcode20
tiff/writer_test.go:20: //{"video-001-16bit.tiff", nil},
remove

https://codereview.appspot.com/13243047/diff/46001/tiff/writer_test.go#newcode92
tiff/writer_test.go:92: img, err := openImage("video-001-paletted.tiff")
You could define a function that takes a B and a filename and get rid of all
that duplicate code.

[bsiegert, 2013-09-13 06:50:37]

LGTM

[chai2010, 2013-09-13 06:56:54]

PTAL

https://codereview.appspot.com/13243047/diff/46001/tiff/writer_test.go
File tiff/writer_test.go (right):

https://codereview.appspot.com/13243047/diff/46001/tiff/writer_test.go#newcode20
tiff/writer_test.go:20: //{"video-001-16bit.tiff", nil},
On 2013/09/13 06:50:19, bsiegert wrote:
> remove

Done.

https://codereview.appspot.com/13243047/diff/46001/tiff/writer_test.go#newcode92
tiff/writer_test.go:92: img, err := openImage("video-001-paletted.tiff")
On 2013/09/13 06:50:19, bsiegert wrote:
> You could define a function that takes a B and a filename and get rid of all
> that duplicate code.

Done.

[nigeltao, 2013-09-13 07:36:59]

LGTM.

I'll make the trivial changes and submit.

https://codereview.appspot.com/13243047/diff/52001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13243047/diff/52001/tiff/writer.go#newcode90
tiff/writer.go:90: // Pix holds the image's pixels, as gray values in big-endian
format.
I'd just say
// An image.Gray16's Pix is in big-endian order.

https://codereview.appspot.com/13243047/diff/52001/tiff/writer.go#newcode320
tiff/writer.go:320: colorMap[i] = uint32(r)
colorMap[i + 0*256] = etc
colorMap[i + 1*256] = etc
colorMap[i + 2*256] = etc

https://codereview.appspot.com/13243047/diff/52001/tiff/writer_test.go
File tiff/writer_test.go (right):

https://codereview.appspot.com/13243047/diff/52001/tiff/writer_test.go#newcode76
tiff/writer_test.go:76: // BenchmarkEncode benchmarks the encoding of an image.
I'd delete the obvious comment.

[nigeltao, 2013-09-13 07:43:07]

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

go.image/tiff: encoder support Gray/Gray16/Paletted format

Use these commands to generate testdata:

# TIFF Tools
tiff2bw    video-001.tiff video-001-gray.tiff
tiffmedian video-001.tiff video-001-paletted.tiff

# ImageMagick
convert -depth 16 video-001.tiff      video-001-16bit.tiff
convert -depth 16 video-001-gray.tiff video-001-gray-16bit.tiff

R=nigeltao, bsiegert
CC=golang-dev
https://codereview.appspot.com/13243047

Committer: Nigel Tao <nigeltao@golang.org>