code review 13736044: go.image/tiff: support 16bit RGB

go.image/tiff: support 16bit RGB

[chai2010, 2013-09-17 02:51:25]

Hello golang-dev@googlegroups.com,

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

[bsiegert, 2013-09-17 18:46:32]

Thanks for doing this. Before I do the review, could you please add
video-001-16bit.tiff to this CL?

[bsiegert, 2013-09-17 20:00:15]

https://codereview.appspot.com/13736044/diff/2002/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13736044/diff/2002/tiff/reader.go#newcode204
tiff/reader.go:204: spp := len(d.features[tBitsPerSample]) // samples per pixel
Add:

bpp := spp * 2 // bytes per pixel

Then replace the "spp*2" expression with bpp below.

https://codereview.appspot.com/13736044/diff/2002/tiff/reader.go#newcode394
tiff/reader.go:394: return nil, UnsupportedError("non-16-bit RGB image")
This error message is a bit misleading.

This code is supposed to check that every entry in tBitsPerSample is 8, or that
every one is 16, and complain otherwise.

https://codereview.appspot.com/13736044/diff/2002/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13736044/diff/2002/tiff/writer.go#newcode380
tiff/writer.go:380: samplesPerPixel = 4
delete, this is the default above.

https://codereview.appspot.com/13736044/diff/2002/tiff/writer.go#newcode386
tiff/writer.go:386: samplesPerPixel = 4
ditto

[chai2010, 2013-09-18 01:23:37]

On 2013/09/17 18:46:32, bsiegert wrote:
> Thanks for doing this. Before I do the review, could you please add
> video-001-16bit.tiff to this CL?

The video-001-16bit.tiff has been added in CL13243047.

[chai2010, 2013-09-18 01:23:53]

PTAL

https://codereview.appspot.com/13736044/diff/2002/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13736044/diff/2002/tiff/reader.go#newcode204
tiff/reader.go:204: spp := len(d.features[tBitsPerSample]) // samples per pixel
On 2013/09/17 20:00:15, bsiegert wrote:
> Add:
> 
> bpp := spp * 2 // bytes per pixel
> 
> Then replace the "spp*2" expression with bpp below.

Done.

https://codereview.appspot.com/13736044/diff/2002/tiff/reader.go#newcode394
tiff/reader.go:394: return nil, UnsupportedError("non-16-bit RGB image")
On 2013/09/17 20:00:15, bsiegert wrote:
> This error message is a bit misleading.
> 
> This code is supposed to check that every entry in tBitsPerSample is 8, or
that
> every one is 16, and complain otherwise. 

Done.

https://codereview.appspot.com/13736044/diff/2002/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13736044/diff/2002/tiff/writer.go#newcode380
tiff/writer.go:380: samplesPerPixel = 4
On 2013/09/17 20:00:15, bsiegert wrote:
> delete, this is the default above.

Done.

https://codereview.appspot.com/13736044/diff/2002/tiff/writer.go#newcode386
tiff/writer.go:386: samplesPerPixel = 4
On 2013/09/17 20:00:15, bsiegert wrote:
> ditto

Done.

[nigeltao, 2013-09-18 03:44:38]

Please run "hg upload 13736044". I am seeing "error: old chunk mismatch" in e.g.
https://codereview.appspot.com/13736044/diff/9001/tiff/writer.go

[chai2010, 2013-09-18 04:23:22]

On 2013/09/18 03:44:38, nigeltao wrote:
> Please run "hg upload 13736044". I am seeing "error: old chunk mismatch" in
e.g.
> https://codereview.appspot.com/13736044/diff/9001/tiff/writer.go

Done.

[nigeltao, 2013-09-18 07:15:19]

LGTM.

[nigeltao, 2013-09-18 07:16:18]

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

go.image/tiff: support 16bit RGB

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

Committer: Nigel Tao <nigeltao@golang.org>

[bsiegert, 2013-09-18 09:27:59]

LGTM