code review 6479044: tiff: new Options parameter for Encode.

tiff: new Options parameter for Encode.

No functional change at the moment but it will allow choosing
the compression type.

This is an incompatible, user-visible change. To get the old
behavior, callers of tiff.Encode need to add ", nil" as the
last parameter.

[bsiegert, 2012-08-22 11:26:58]

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-08-23 00:54:51]

http://codereview.appspot.com/6479044/diff/4001/tiff/consts.go
File tiff/consts.go (right):

http://codereview.appspot.com/6479044/diff/4001/tiff/consts.go#newcode113
tiff/consts.go:113: type CompressionType int
What other compression types will you eventually support? I don't know TIFF that
well.

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go#newcode143
tiff/writer.go:143: // Options encodes the options used for encoding a TIFF
image.
Using "encodes" twice is awkward. I'd just mimic what image/jpeg does:
// Options are the encoding parameters.

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go#newcode144
tiff/writer.go:144: // Compression is the type of compression used. Predictor
determines
These comments probably work better on the fields, not on the struct as as
whole.

// Options are the encoding parameters.
type Options struct {
  // Compression is the type of compression used.
  Compression CompressionType
  // Predictor determines etc.
  Predictor bool
}

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go#newcode159
tiff/writer.go:159: opt = &Options{}
I'd rather not allocate unnecessary garbage. image/jpeg does something like

quality := DefaultQuality
if opt != nil {
  quality = opt.Quality
}

[bsiegert, 2012-08-23 09:35:32]

Thanks for the review. All done, PTAL.

http://codereview.appspot.com/6479044/diff/4001/tiff/consts.go
File tiff/consts.go (right):

http://codereview.appspot.com/6479044/diff/4001/tiff/consts.go#newcode113
tiff/consts.go:113: type CompressionType int
On 2012/08/23 00:54:51, nigeltao wrote:
> What other compression types will you eventually support? I don't know TIFF
that
> well.

The easiest ones are PackBits and Deflate. I would like to get around to doing
CCITT eventually. There is also JPEG compression, which I have not looked into
yet; it is probably irrelevant, you could just write a .jpg directly instead.
Finally, there is LZW, but there seems to be an incompatibility between our LZW
package and what tiff expects.

There are internal constants for the compression types in this file (cXXX).

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go#newcode143
tiff/writer.go:143: // Options encodes the options used for encoding a TIFF
image.
On 2012/08/23 00:54:51, nigeltao wrote:
> Using "encodes" twice is awkward. I'd just mimic what image/jpeg does:
> // Options are the encoding parameters.

Done.

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go#newcode144
tiff/writer.go:144: // Compression is the type of compression used. Predictor
determines
On 2012/08/23 00:54:51, nigeltao wrote:
> These comments probably work better on the fields, not on the struct as as
> whole.
> 
> // Options are the encoding parameters.
> type Options struct {
>   // Compression is the type of compression used.
>   Compression CompressionType
>   // Predictor determines etc.
>   Predictor bool
> }

Done.

http://codereview.appspot.com/6479044/diff/4001/tiff/writer.go#newcode159
tiff/writer.go:159: opt = &Options{}
On 2012/08/23 00:54:51, nigeltao wrote:
> I'd rather not allocate unnecessary garbage. image/jpeg does something like
> 
> quality := DefaultQuality
> if opt != nil {
>   quality = opt.Quality
> }

Done.

[nigeltao, 2012-08-23 10:14:50]

LGTM.

[nigeltao, 2012-08-23 10:17:21]

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

tiff: new Options parameter for Encode.

No functional change at the moment but it will allow choosing
the compression type.

This is an incompatible, user-visible change. To get the old
behavior, callers of tiff.Encode need to add ", nil" as the
last parameter.

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

Committer: Nigel Tao <nigeltao@golang.org>