code review 6866051: tiff: support for writing compressed images.

go.image/tiff: support for writing compressed images.

Only Deflate compression is supported for now.

[bsiegert, 2012-12-04 13:29:30]

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-12-05 03:34:38]

https://codereview.appspot.com/6866051/diff/5001/tiff/consts.go
File tiff/consts.go (right):

https://codereview.appspot.com/6866051/diff/5001/tiff/consts.go#newcode133
tiff/consts.go:133: for _, v := range compressionEq {
I'd just hard-code it:

func (c CompressionType) specValue() uint32 {
  switch c {
  case Deflate:
    return cDeflate
  }
  return cNone
}

https://codereview.appspot.com/6866051/diff/5001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/6866051/diff/5001/tiff/writer.go#newcode243
tiff/writer.go:243: dst.(io.WriteCloser).Close()
s/WriteCloser/Closer/, and it's probably good form to check the error returned,
even though in for now you're writing to a bytes.Buffer:

if err := dst.(io.Closer).Close(); err != nil {
  return err
}

[bsiegert, 2012-12-10 15:43:58]

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

Please take another look.

[bsiegert, 2012-12-10 15:44:04]

https://codereview.appspot.com/6866051/diff/5001/tiff/consts.go
File tiff/consts.go (right):

https://codereview.appspot.com/6866051/diff/5001/tiff/consts.go#newcode133
tiff/consts.go:133: for _, v := range compressionEq {
On 2012/12/05 03:34:38, nigeltao wrote:
> I'd just hard-code it:
> 
> func (c CompressionType) specValue() uint32 {
>   switch c {
>   case Deflate:
>     return cDeflate
>   }
>   return cNone
> }

Done.

https://codereview.appspot.com/6866051/diff/5001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/6866051/diff/5001/tiff/writer.go#newcode243
tiff/writer.go:243: dst.(io.WriteCloser).Close()
On 2012/12/05 03:34:38, nigeltao wrote:
> s/WriteCloser/Closer/, and it's probably good form to check the error
returned,
> even though in for now you're writing to a bytes.Buffer:
> 
> if err := dst.(io.Closer).Close(); err != nil {
>   return err
> }

Done.

[nigeltao, 2012-12-11 00:44:09]

https://codereview.appspot.com/6866051/diff/12001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/6866051/diff/12001/tiff/writer.go#newcode184
tiff/writer.go:184: compression := Uncompressed
It might be better to write this as

compression := cNone
if opt != nil {
  predictor = opt.Predictor
  compression = opt.Compression.specValue()
}

in case someone tries to pass opt.Compression == CompressionType(42).

[mikio, 2012-12-11 00:48:44]

perhaps the CL description might be "go.image/tiff: ...".

[bsiegert, 2012-12-11 08:22:51]

PTAL. Also changed the CL description as requested.

https://codereview.appspot.com/6866051/diff/12001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/6866051/diff/12001/tiff/writer.go#newcode184
tiff/writer.go:184: compression := Uncompressed
On 2012/12/11 00:44:09, nigeltao wrote:
> It might be better to write this as
> 
> compression := cNone
> if opt != nil {
>   predictor = opt.Predictor
>   compression = opt.Compression.specValue()
> }
> 
> in case someone tries to pass opt.Compression == CompressionType(42).

Done.

[nigeltao, 2012-12-12 02:23:13]

LGTM.

[nigeltao, 2012-12-12 02:24:29]

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

go.image/tiff: support for writing compressed images.

Only Deflate compression is supported for now.

R=nigeltao, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/6866051

Committer: Nigel Tao <nigeltao@golang.org>