code review 13453043: go.image/tiff: decoder support tiled tiff format

go.image/tiff: decoder support tiled tiff format

Use these commands to generate testdata:
tiffcp -s -r 64 video-001.tiff video-001-strip-64.tiff
tiffcp -t -l 64 -w 64 video-001.tiff video-001-tile-64x64.tiff

[chai2010, 2013-08-31 13:48:47]

Hello golang-dev@googlegroups.com,

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

[nigeltao, 2013-09-02 07:35:45]

Benny Siegert should also review this code, as he's written most if not all of
image/tiff.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode60
tiff/reader.go:60: f, ok := d.features[tag]
This change isn't necessary. If tag isn't in the d.features map, then f will be
the nil slice, and len(f) will be zero.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode234
tiff/reader.go:234: img.SetRGBA(x-xmin, y-ymin, color.RGBA{
I suspect that changing from setting img.Pix directly to setting it indirectly
via img.SetRGBA will be very bad for performance. What's the change in the
benchmark numbers (call "go test -test.bench=." and run the before/after numbers
through $GOROOT/misc/benchcmp)? If it's bad, please work with img.Pix directly.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode252
tiff/reader.go:252: 0xff,
This code has an implicit 0xff but the previous code (which called the built-in
copy function) did not. Which one is correct!?

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode430
tiff/reader.go:430: blockImage = image.NewGray(image.Rect(0, 0, blockWidth,
blockHeight))
You shouldn't need to allocate a separate blockImage and then copy that into
img. Instead, decode directly into img.SubImage(etc).

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode497
tiff/reader.go:497: drawImg.Set(x, y, blockImage.At(x-xmin, y-ymin))
This is a horrible way to copy an image. To do an efficient copy, call draw.Draw
from the image/draw package.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcod...
tiff/reader_test.go:104: f2, err := os.Open(testdataDir +
"video-001-strip-64.tiff")
How were your test files generated?

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcod...
tiff/reader_test.go:114: f3, err := os.Open(testdataDir +
"video-001-tile-64x64.tiff")
It seems like it's worth factoring out:

func load(name) (image.Image, error) {
	f, err := os.Open(testdataDir + name)
	if err != nil {
		return nil, err
	}
	defer f.Close()
	img, _, err := image.Decode(f)
	if err != nil {
		return nil, err
	}
	return img, nil
}

[chai2010, 2013-09-03 03:10:26]

PTAL

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode60
tiff/reader.go:60: f, ok := d.features[tag]
On 2013/09/02 07:35:45, nigeltao wrote:
> This change isn't necessary. If tag isn't in the d.features map, then f will
be
> the nil slice, and len(f) will be zero.

Done.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode234
tiff/reader.go:234: img.SetRGBA(x-xmin, y-ymin, color.RGBA{
On 2013/09/02 07:35:45, nigeltao wrote:
> I suspect that changing from setting img.Pix directly to setting it indirectly
> via img.SetRGBA will be very bad for performance. What's the change in the
> benchmark numbers (call "go test -test.bench=." and run the before/after
numbers
> through $GOROOT/misc/benchcmp)? If it's bad, please work with img.Pix
directly.

Done.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode252
tiff/reader.go:252: 0xff,
On 2013/09/02 07:35:45, nigeltao wrote:
> This code has an implicit 0xff but the previous code (which called the
built-in
> copy function) did not. Which one is correct!?

I don't understand the NRGBA model clearly.
But i think use 0xff is error.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode254
tiff/reader.go:254: off += 3
off += 4

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode430
tiff/reader.go:430: blockImage = image.NewGray(image.Rect(0, 0, blockWidth,
blockHeight))
On 2013/09/02 07:35:45, nigeltao wrote:
> You shouldn't need to allocate a separate blockImage and then copy that into
> img. Instead, decode directly into img.SubImage(etc).

Done.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode497
tiff/reader.go:497: drawImg.Set(x, y, blockImage.At(x-xmin, y-ymin))
On 2013/09/02 07:35:45, nigeltao wrote:
> This is a horrible way to copy an image. To do an efficient copy, call
draw.Draw
> from the image/draw package.

Done.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcod...
tiff/reader_test.go:104: f2, err := os.Open(testdataDir +
"video-001-strip-64.tiff")
On 2013/09/02 07:35:45, nigeltao wrote:
> How were your test files generated?

tiffcp -s -r 64 video-001.tiff video-001-strip-64.tiff
tiffcp -t -l 64 -w 64 video-001.tiff video-001-tile-64x64.tiff

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcod...
tiff/reader_test.go:114: f3, err := os.Open(testdataDir +
"video-001-tile-64x64.tiff")
On 2013/09/02 07:35:45, nigeltao wrote:
> It seems like it's worth factoring out:
> 
> func load(name) (image.Image, error) {
> 	f, err := os.Open(testdataDir + name)
> 	if err != nil {
> 		return nil, err
> 	}
> 	defer f.Close()
> 	img, _, err := image.Decode(f)
> 	if err != nil {
> 		return nil, err
> 	}
> 	return img, nil
> }

Done.

[bsiegert, 2013-09-03 07:52:26]

LGTM if you change off += 3 to 4, as Nigel wrote.

Am Dienstag, 3. September 2013 schrieb :

> PTAL
>
>
>
https://codereview.appspot.**com/13453043/diff/13001/tiff/**reader.go<https:/...
> File tiff/reader.go (right):
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader.go#newcode60<https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode60>
> tiff/reader.go:60: f, ok := d.features[tag]
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> This change isn't necessary. If tag isn't in the d.features map, then
>>
> f will be
>
>> the nil slice, and len(f) will be zero.
>>
>
> Done.
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader.go#newcode234<https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode234>
> tiff/reader.go:234: img.SetRGBA(x-xmin, y-ymin, color.RGBA{
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> I suspect that changing from setting img.Pix directly to setting it
>>
> indirectly
>
>> via img.SetRGBA will be very bad for performance. What's the change in
>>
> the
>
>> benchmark numbers (call "go test -test.bench=." and run the
>>
> before/after numbers
>
>> through $GOROOT/misc/benchcmp)? If it's bad, please work with img.Pix
>>
> directly.
>
> Done.
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader.go#newcode252<https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode252>
> tiff/reader.go:252: 0xff,
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> This code has an implicit 0xff but the previous code (which called the
>>
> built-in
>
>> copy function) did not. Which one is correct!?
>>
>
> I don't understand the NRGBA model clearly.
> But i think use 0xff is error.
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader.go#newcode254<https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode254>
> tiff/reader.go:254: off += 3
> off += 4
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader.go#newcode430<https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode430>
> tiff/reader.go:430: blockImage = image.NewGray(image.Rect(0, 0,
> blockWidth, blockHeight))
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> You shouldn't need to allocate a separate blockImage and then copy
>>
> that into
>
>> img. Instead, decode directly into img.SubImage(etc).
>>
>
> Done.
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader.go#newcode497<https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode497>
> tiff/reader.go:497: drawImg.Set(x, y, blockImage.At(x-xmin, y-ymin))
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> This is a horrible way to copy an image. To do an efficient copy, call
>>
> draw.Draw
>
>> from the image/draw package.
>>
>
> Done.
>
>
https://codereview.appspot.**com/13453043/diff/13001/tiff/**reader_test.go<ht...
> File tiff/reader_test.go (right):
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader_test.go#newcode104<https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcode104>
> tiff/reader_test.go:104: f2, err := os.Open(testdataDir +
> "video-001-strip-64.tiff")
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> How were your test files generated?
>>
>
> tiffcp -s -r 64 video-001.tiff video-001-strip-64.tiff
> tiffcp -t -l 64 -w 64 video-001.tiff video-001-tile-64x64.tiff
>
> https://codereview.appspot.**com/13453043/diff/13001/tiff/**
>
reader_test.go#newcode114<https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcode114>
> tiff/reader_test.go:114: f3, err := os.Open(testdataDir +
> "video-001-tile-64x64.tiff")
> On 2013/09/02 07:35:45, nigeltao wrote:
>
>> It seems like it's worth factoring out:
>>
>
>  func load(name) (image.Image, error) {
>>         f, err := os.Open(testdataDir + name)
>>         if err != nil {
>>                 return nil, err
>>         }
>>         defer f.Close()
>>         img, _, err := image.Decode(f)
>>         if err != nil {
>>                 return nil, err
>>         }
>>         return img, nil
>> }
>>
>
> Done.
>
>
https://codereview.appspot.**com/13453043/<https://codereview.appspot.com/134...
>


-- 
The first essential in chemistry is that you should perform practical work
and conduct experiments, for he who performs not practical work nor makes
experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[chai2010, 2013-09-05 01:16:28]

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader.go#newcode254
tiff/reader.go:254: off += 3
On 2013/09/03 03:10:27, chai2010 wrote:
> off += 4

Done.

[nigeltao, 2013-09-05 04:34:52]

Please confirm that the benchmarks show no significant regression.

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcod...
tiff/reader_test.go:104: f2, err := os.Open(testdataDir +
"video-001-strip-64.tiff")
On 2013/09/03 03:10:27, chai2010 wrote:
> On 2013/09/02 07:35:45, nigeltao wrote:
> > How were your test files generated?
> 
> tiffcp -s -r 64 video-001.tiff video-001-strip-64.tiff
> tiffcp -t -l 64 -w 64 video-001.tiff video-001-tile-64x64.tiff

Please add this to the change description, by running "hg change 13453043".

https://codereview.appspot.com/13453043/diff/26001/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13453043/diff/26001/tiff/reader.go#newcode209
tiff/reader.go:209: for y := ymin; y < ymax && y < img.Rect.Max.Y; y++ {
You shouldn't need to do two comparisons "y < ymax && y < img.Rect.Max.Y" on
each iteration. Calculate the minimum of ymax and img.Rect.Max.Y beforehand,
once.

Similarly below.

https://codereview.appspot.com/13453043/diff/26001/tiff/reader.go#newcode438
tiff/reader.go:438: if len(blockOffsets) < blocksAcross*blocksDown ||
len(blockCounts) < blocksAcross*blocksDown {
if n := blocksAcross*blocksDown; len(blockOffsets) < n || len(blockCounts) < n {

https://codereview.appspot.com/13453043/diff/26001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/26001/tiff/reader_test.go#newcode28
tiff/reader_test.go:28: f, err := os.Open(testdataDir + "no_rps.tiff")
This can also use the new load function.

https://codereview.appspot.com/13453043/diff/26001/tiff/reader_test.go#newcod...
tiff/reader_test.go:129: f, err := os.Open(testdataDir + name)
This could also re-use load, with a bit of tweaking.

[chai2010, 2013-09-05 05:30:57]

go test -test.bench=.

before:
BenchmarkDecodeCompressed            500           3420195 ns/op
BenchmarkDecodeUncompressed        10000            160909 ns/op
BenchmarkEncode   500000              4360 ns/op        14173.50 MB/s
ok      code.google.com/p/go.image/tiff 6.304s

after:
BenchmarkDecodeCompressed           1000           1801103 ns/op
BenchmarkDecodeUncompressed        10000            162209 ns/op
BenchmarkEncode   500000              4444 ns/op        13905.60 MB/s
ok      code.google.com/p/go.image/tiff 6.171s

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/13001/tiff/reader_test.go#newcod...
tiff/reader_test.go:104: f2, err := os.Open(testdataDir +
"video-001-strip-64.tiff")
On 2013/09/05 04:34:52, nigeltao wrote:
> On 2013/09/03 03:10:27, chai2010 wrote:
> > On 2013/09/02 07:35:45, nigeltao wrote:
> > > How were your test files generated?
> > 
> > tiffcp -s -r 64 video-001.tiff video-001-strip-64.tiff
> > tiffcp -t -l 64 -w 64 video-001.tiff video-001-tile-64x64.tiff
> 
> Please add this to the change description, by running "hg change 13453043".

Done.

https://codereview.appspot.com/13453043/diff/26001/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13453043/diff/26001/tiff/reader.go#newcode209
tiff/reader.go:209: for y := ymin; y < ymax && y < img.Rect.Max.Y; y++ {
On 2013/09/05 04:34:52, nigeltao wrote:
> You shouldn't need to do two comparisons "y < ymax && y < img.Rect.Max.Y" on
> each iteration. Calculate the minimum of ymax and img.Rect.Max.Y beforehand,
> once.
> 
> Similarly below.

Done.

https://codereview.appspot.com/13453043/diff/26001/tiff/reader.go#newcode438
tiff/reader.go:438: if len(blockOffsets) < blocksAcross*blocksDown ||
len(blockCounts) < blocksAcross*blocksDown {
On 2013/09/05 04:34:52, nigeltao wrote:
> if n := blocksAcross*blocksDown; len(blockOffsets) < n || len(blockCounts) < n
{

Done.

https://codereview.appspot.com/13453043/diff/26001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/26001/tiff/reader_test.go#newcode28
tiff/reader_test.go:28: f, err := os.Open(testdataDir + "no_rps.tiff")
On 2013/09/05 04:34:52, nigeltao wrote:
> This can also use the new load function.

Done.

https://codereview.appspot.com/13453043/diff/26001/tiff/reader_test.go#newcod...
tiff/reader_test.go:129: f, err := os.Open(testdataDir + name)
On 2013/09/05 04:34:52, nigeltao wrote:
> This could also re-use load, with a bit of tweaking.

Done.

[nigeltao, 2013-09-06 10:04:38]

LGTM.

I'll make some trivial edits and submit.

https://codereview.appspot.com/13453043/diff/37001/tiff/reader.go
File tiff/reader.go (left):

https://codereview.appspot.com/13453043/diff/37001/tiff/reader.go#oldcode194
tiff/reader.go:194: for x := 0; x < (dst.Bounds().Dx()-1)*spp; x++ {
For the record, the change to this line explains why the benchmarks improved so
much.

https://codereview.appspot.com/13453043/diff/37001/tiff/reader.go
File tiff/reader.go (right):

https://codereview.appspot.com/13453043/diff/37001/tiff/reader.go#newcode412
tiff/reader.go:412: blockOffsets := []uint{}
var blockOffsets, blockCounts []uint

would initialize them to nil instead of a non-nil empty slice.

https://codereview.appspot.com/13453043/diff/37001/tiff/reader_test.go
File tiff/reader_test.go (right):

https://codereview.appspot.com/13453043/diff/37001/tiff/reader_test.go#newcode76
tiff/reader_test.go:76: func load(name string) (image.Image, error) {
I'd now move this function to before its first use in TestNoRPS.

[nigeltao, 2013-09-06 10:08:29]

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

go.image/tiff: decoder support tiled tiff format

Use these commands to generate testdata:
tiffcp -s -r 64 video-001.tiff video-001-strip-64.tiff
tiffcp -t -l 64 -w 64 video-001.tiff video-001-tile-64x64.tiff

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

Committer: Nigel Tao <nigeltao@golang.org>