code review 6308049: go.image/tiff: use optimized routines for RGBA and NRGBA

go.image/tiff: use optimized routines for RGBA and NRGBA

Performance for an RGBA image:

benchmark          old ns/op    new ns/op    delta
BenchmarkEncode      1633495         7897  -99.52%

benchmark           old MB/s     new MB/s  speedup
BenchmarkEncode        37.83      7824.96  206.85x

NRGBA images are now encoded as such, the spec calls it
"unassociated alpha".

[bsiegert, 2012-06-07 13:01:27]

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

[rsc, 2012-06-07 13:03:14]

You're encoding at 8 GB/s? Sounds too good to be true.

Russ

[bsiegert, 2012-06-07 14:15:17]

On Thu, Jun 7, 2012 at 3:02 PM, Russ Cox <rsc@golang.org> wrote:
> You're encoding at 8 GB/s? Sounds too good to be true.

That's the bandwidth of io.Discard.

--Benny.

[rsc, 2012-06-07 14:24:15]

Aha. I'm a little surprised we're writing uncompressed TIFFs but carry on.

Russ

[nigeltao, 2012-06-08 01:56:35]

http://codereview.appspot.com/6308049/diff/3002/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/6308049/diff/3002/tiff/writer.go#newcode79
tiff/writer.go:79: if _, err := w.Write(m.Pix[y*m.Stride : y*m.Stride+linelen]);
err != nil {
"y*m.Stride" is incorrect if m.Rect does not start at the origin (0, 0). Call
m.PixOffset instead. If you're curious, look in image/image.go for the actual
formula.

It may be worth adding a test that you can round-trip an image modulo a
translation of its Rect.Min:

m0 := image.NewRGBA(image.Rect(3, 4, 9, 8))
// manually fill m0 with some non-zero pixels
// buf := Encode(m0)
// m1 := Decode(buf)
compare(m1, m0)

But, your writeRGBA and writeNRGBA functions are a copy-and-paste of each other.
Instead, how about:

func writePix(w io.Writer, pix []byte, nrows, length, stride int) error {
  // TODO: you could possibly optimize this if length == stride.
  // It may or may not be worthwhile.
  for ; nrows > 0; nrows-- {
    if _, err := w.Write(pix[:length]); err != nil {
      return err
    }
    pix = pix[stride:]
  }
  return nil
}

[bsiegert, 2012-06-12 14:04:31]

Thank you very much for your comments. PTAL.

http://codereview.appspot.com/6308049/diff/3002/tiff/writer.go
File tiff/writer.go (right):

http://codereview.appspot.com/6308049/diff/3002/tiff/writer.go#newcode79
tiff/writer.go:79: if _, err := w.Write(m.Pix[y*m.Stride : y*m.Stride+linelen]);
err != nil {
On 2012/06/08 01:56:35, nigeltao wrote:
> "y*m.Stride" is incorrect if m.Rect does not start at the origin (0, 0). Call
> m.PixOffset instead. If you're curious, look in image/image.go for the actual
> formula.

Okay. So if I loop from 0 to img.Rect.Dy(), then y*img.Stride is correct, I
assume. Anyway, I used the code you suggested below.

> It may be worth adding a test that you can round-trip an image modulo a
> translation of its Rect.Min:
> 
> m0 := image.NewRGBA(image.Rect(3, 4, 9, 8))
> // manually fill m0 with some non-zero pixels
> // buf := Encode(m0)
> // m1 := Decode(buf)
> compare(m1, m0)

Yes. I added this as TestRoundtrip2. I also had to tweak the compare function a
bit so that different origins work.

> But, your writeRGBA and writeNRGBA functions are a copy-and-paste of each
other.
> Instead, how about:
> 
> func writePix(w io.Writer, pix []byte, nrows, length, stride int) error {

Excellent!

>   // TODO: you could possibly optimize this if length == stride.
>   // It may or may not be worthwhile.

I did this, and it does show up in the benchmark as an additional 20% gain.

[nigeltao, 2012-06-13 00:39:29]

LGTM.

[nigeltao, 2012-06-13 00:40:47]

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

go.image/tiff: use optimized routines for RGBA and NRGBA

Performance for an RGBA image:

benchmark          old ns/op    new ns/op    delta
BenchmarkEncode      1633495         7897  -99.52%

benchmark           old MB/s     new MB/s  speedup
BenchmarkEncode        37.83      7824.96  206.85x

NRGBA images are now encoded as such, the spec calls it
"unassociated alpha".

R=nigeltao, rsc
CC=golang-dev
http://codereview.appspot.com/6308049

Committer: Nigel Tao <nigeltao@golang.org>