code review 5694051: go.image/tiff: initial support for writing TIFF images

go.image/tiff: initial support for writing TIFF images

The basic functionality works. Features to add in future CLs:

- compression
- fast paths for image formats that can be directly expressed in TIFF,
  such as RGBA, NRGBA and maybe Gray and Paletted.

[bsiegert, 2012-02-23 17:31:09]

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

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

[nigeltao, 2012-02-24 02:17:33]

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode31
tiff/writer.go:31: data     []uint
Should uint instead be uint32?

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode74
tiff/writer.go:74: width:  uint(m.Bounds().Dx()),
Why are width and height fields, instead of just local variables?

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode82
tiff/writer.go:82: // so give a bogus value of 72x72 dpi.
Instead of a bogus 72x72, can we just leave the field out altogether?

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode84
tiff/writer.go:84: {tImageWidth, dtShort, []uint{uint(e.width)}},
The uint seems unnecessary.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode109
tiff/writer.go:109: buf[i] = uint8(r)
s/i/i+0/ maintains symmetry with the lines below.

Also, Color.RGBA returns values within [0, 0xFFFF]. Thus, s/r/r>>8/, and
similarly below.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode150
tiff/writer.go:150: newarea := make([]byte, len(parea)+1024)
1024 isn't guaranteed to be long enough, right?

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode155
tiff/writer.go:155: enc.PutUint32(buf[8:12], uint32(pstart+o))
uint32 is unnecessary, but I think it would be cleaner if you computed mostly in
terms of ints, instead of in uint32s. In other words, do the conversion of
things like slice lengths from int to uint32 later rather than earlier.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode172
tiff/writer.go:172: var err error
Just use := on the next line.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode187
tiff/writer.go:187: err = e.writeIFD(w)
return e.writeIFD(w)

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode193
tiff/writer.go:193: e := newEncoder(m)
return newEncoder(m).encode(w)

[bsiegert, 2012-02-24 20:22:55]

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

Please take another look.

[bsiegert, 2012-02-24 20:23:18]

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode31
tiff/writer.go:31: data     []uint
On 2012/02/24 02:17:33, nigeltao wrote:
> Should uint instead be uint32?

Done.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode74
tiff/writer.go:74: width:  uint(m.Bounds().Dx()),
On 2012/02/24 02:17:33, nigeltao wrote:
> Why are width and height fields, instead of just local variables?

Hm. It was necessary in an earlier version of the patch. Removed.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode82
tiff/writer.go:82: // so give a bogus value of 72x72 dpi.
On 2012/02/24 02:17:33, nigeltao wrote:
> Instead of a bogus 72x72, can we just leave the field out altogether?

Alas, no. These tags are mentioned in the table about "required fields" on page
21 of the spec. They are thus mandatory. 72 dpi is the "standard" definition of
a point (well, 72.27 actually).

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode84
tiff/writer.go:84: {tImageWidth, dtShort, []uint{uint(e.width)}},
On 2012/02/24 02:17:33, nigeltao wrote:
> The uint seems unnecessary.

Done.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode109
tiff/writer.go:109: buf[i] = uint8(r)
On 2012/02/24 02:17:33, nigeltao wrote:
> s/i/i+0/ maintains symmetry with the lines below.
> 
> Also, Color.RGBA returns values within [0, 0xFFFF]. Thus, s/r/r>>8/, and
> similarly below.

Done. Thanks.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode150
tiff/writer.go:150: newarea := make([]byte, len(parea)+1024)
On 2012/02/24 02:17:33, nigeltao wrote:
> 1024 isn't guaranteed to be long enough, right?

Good point. I replaced the if by for above.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode155
tiff/writer.go:155: enc.PutUint32(buf[8:12], uint32(pstart+o))
On 2012/02/24 02:17:33, nigeltao wrote:
> uint32 is unnecessary, but I think it would be cleaner if you computed mostly
in
> terms of ints, instead of in uint32s. In other words, do the conversion of
> things like slice lengths from int to uint32 later rather than earlier.

Done, I think.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode172
tiff/writer.go:172: var err error
On 2012/02/24 02:17:33, nigeltao wrote:
> Just use := on the next line.

Done.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode187
tiff/writer.go:187: err = e.writeIFD(w)
On 2012/02/24 02:17:33, nigeltao wrote:
> return e.writeIFD(w)

Done.

http://codereview.appspot.com/5694051/diff/2001/tiff/writer.go#newcode193
tiff/writer.go:193: e := newEncoder(m)
On 2012/02/24 02:17:33, nigeltao wrote:
> return newEncoder(m).encode(w)

Done.

[nigeltao, 2012-02-27 12:02:27]

I'm still not convinced that we really need an encoder type, but I might see how
that looks in a follow-up CL, not in this one.

http://codereview.appspot.com/5694051/diff/1002/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/5694051/diff/1002/tiff/writer.go#newcode67
tiff/writer.go:67: imageLen uint32 // Length of the image in bytes.
s/uint32/int/

http://codereview.appspot.com/5694051/diff/1002/tiff/writer.go#newcode73
tiff/writer.go:73: e := &encoder{img: m}
You might as well make this a complete struct literal, instead of initializing
some fields afterwards:

return &encoder{
  img: m,
  // For uncompressed etc.
  imageLen: width * height * 4,
  // There is etc
  ifd: ifd{
    {tImageWidth, etc},
  },
}

[bsiegert, 2012-03-02 17:36:59]

All done, PTAL.

Regarding the need for a encoder type: I tried to use this as a structuring
element for the code. The type is not exported, so it is not set in stone.

http://codereview.appspot.com/5694051/diff/1002/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/5694051/diff/1002/tiff/writer.go#newcode67
tiff/writer.go:67: imageLen uint32 // Length of the image in bytes.
On 2012/02/27 12:02:27, nigeltao wrote:
> s/uint32/int/

Done.

http://codereview.appspot.com/5694051/diff/1002/tiff/writer.go#newcode73
tiff/writer.go:73: e := &encoder{img: m}
On 2012/02/27 12:02:27, nigeltao wrote:
> You might as well make this a complete struct literal, instead of initializing
> some fields afterwards:
> 
> return &encoder{
>   img: m,
>   // For uncompressed etc.
>   imageLen: width * height * 4,
>   // There is etc
>   ifd: ifd{
>     {tImageWidth, etc},
>   },
> }

Done.

http://codereview.appspot.com/5694051/diff/6001/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/5694051/diff/6001/tiff/writer.go#newcode143
tiff/writer.go:143: datalen := int(count * lengths[ent.datatype])
I also fixed a bug here. For a dtRational, too many bytes were written.

[nigeltao, 2012-03-05 00:45:13]

LGTM.

[nigeltao, 2012-03-05 00:46:03]

*** Submitted as
http://code.google.com/p/go/source/detail?r=2bf0db14a16d&repo=image ***

go.image/tiff: initial support for writing TIFF images

The basic functionality works. Features to add in future CLs:

- compression
- fast paths for image formats that can be directly expressed in TIFF,
  such as RGBA, NRGBA and maybe Gray and Paletted.

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

Committer: Nigel Tao <nigeltao@golang.org>

[bsiegert, 2012-03-05 23:08:48]

*** Abandoned ***

[bsiegert, 2012-03-05 23:17:44]

On Tue, Mar 6, 2012 at 00:08,  <bsiegert@gmail.com> wrote:
> *** Abandoned ***
>
> http://codereview.appspot.com/5694051/

This looks like a bug in codereview:

% hg sync
pulling from https://code.google.com/p/go.image
added 1 changesets with 3 changes to 3 files
3 files updated, 0 files merged, 0 files removed, 0 files unresolved
Removing unmodified files from CL 5694051:
       tiff/consts.go
       tiff/writer.go
       tiff/writer_test.go
CL 5694051 has no files; delete (abandon) with hg change -d 5694051

--Benny.