code review 6684046: image/jpeg: decode progressive JPEGs.

image/jpeg: decode progressive JPEGs.

To be clear, this supports decoding the bytes on the wire into an
in-memory image. There is no API change: jpeg.Decode will still not
return until the entire image is decoded.

The code is obviously more complicated, and costs around 10% in
performance on baseline JPEGs. The processSOS code could be cleaned up a
bit, and maybe some of that loss can be reclaimed, but I'll leave that
for follow-up CLs, to keep the diff for this one as small as possible.

Before:
BenchmarkDecode	    1000	   2855637 ns/op	  21.64 MB/s
After:
BenchmarkDecodeBaseline	     500	   3178960 ns/op	  19.44 MB/s
BenchmarkDecodeProgressive	     500	   4082640 ns/op	  15.14 MB/s

Fixes issue 3976.

The test data was generated by:
# Create intermediate files; cjpeg on Ubuntu 10.04 can't read PNG.
convert video-001.png video-001.bmp
convert video-005.gray.png video-005.gray.pgm
# Create new test files.
cjpeg -quality 100 -sample 1x1,1x1,1x1 -progressive video-001.bmp > video-001.progressive.jpeg
cjpeg -quality 50 -sample 2x2,1x1,1x1 video-001.bmp > video-001.q50.420.jpeg
cjpeg -quality 50 -sample 2x1,1x1,1x1 video-001.bmp > video-001.q50.422.jpeg
cjpeg -quality 50 -sample 1x1,1x1,1x1 video-001.bmp > video-001.q50.444.jpeg
cjpeg -quality 50 -sample 2x2,1x1,1x1 -progressive video-001.bmp > video-001.q50.420.progressive.jpeg
cjpeg -quality 50 -sample 2x1,1x1,1x1 -progressive video-001.bmp > video-001.q50.422.progressive.jpeg
cjpeg -quality 50 -sample 1x1,1x1,1x1 -progressive video-001.bmp > video-001.q50.444.progressive.jpeg
cjpeg -quality 50 video-005.gray.pgm > video-005.gray.q50.jpeg
cjpeg -quality 50 -progressive video-005.gray.pgm > video-005.gray.q50.progressive.jpeg
# Delete intermediate files.
rm video-001.bmp video-005.gray.pgm

[nigeltao, 2012-10-14 07:25:52]

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

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

[r, 2012-10-14 21:33:26]

LGTM

http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader.go
File src/pkg/image/jpeg/reader.go (right):

http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:317: // pixel co-ordinates is (16, 8) is the third
block in the first row:
s/is/are/

http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:318: // mx0 is 2 and my0 is 0, even though it is in
the second MCU.
s/it/the pixel/

http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:348: //	3 4 5
are all these possibilities covered by the test data?

http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader_tes...
File src/pkg/image/jpeg/reader_test.go (right):

http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader_tes...
src/pkg/image/jpeg/reader_test.go:20: check := func(prefix string, bounds
image.Rectangle, pix0, pix1 []byte, stride0, stride1 int) (ok bool) {
this is a big function. why not move it outside? other than t you're not
capturing any state.

[nigeltao, 2012-10-15 00:12:56]

https://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader.go
File src/pkg/image/jpeg/reader.go (right):

https://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader.go...
src/pkg/image/jpeg/reader.go:348: //	3 4 5
On 2012/10/14 21:33:26, r wrote:
> are all these possibilities covered by the test data?

All the src/pkg/image/testdata images are 150x103 pixels, or 19x13 blocks, which
don't span all the possibilities, but it does exercise this case. I do have a
larger set of real-world progressive JPEGs of varying sizes and chroma
subsampling ratios, based on a web crawl sample, but I don't think that it's
appropriate to check those in.

[nigeltao, 2012-10-15 00:21:38]

*** Submitted as http://code.google.com/p/go/source/detail?r=51f26e36ba98 ***

image/jpeg: decode progressive JPEGs.

To be clear, this supports decoding the bytes on the wire into an
in-memory image. There is no API change: jpeg.Decode will still not
return until the entire image is decoded.

The code is obviously more complicated, and costs around 10% in
performance on baseline JPEGs. The processSOS code could be cleaned up a
bit, and maybe some of that loss can be reclaimed, but I'll leave that
for follow-up CLs, to keep the diff for this one as small as possible.

Before:
BenchmarkDecode	    1000	   2855637 ns/op	  21.64 MB/s
After:
BenchmarkDecodeBaseline	     500	   3178960 ns/op	  19.44 MB/s
BenchmarkDecodeProgressive	     500	   4082640 ns/op	  15.14 MB/s

Fixes issue 3976.

The test data was generated by:
# Create intermediate files; cjpeg on Ubuntu 10.04 can't read PNG.
convert video-001.png video-001.bmp
convert video-005.gray.png video-005.gray.pgm
# Create new test files.
cjpeg -quality 100 -sample 1x1,1x1,1x1 -progressive video-001.bmp >
video-001.progressive.jpeg
cjpeg -quality 50 -sample 2x2,1x1,1x1 video-001.bmp > video-001.q50.420.jpeg
cjpeg -quality 50 -sample 2x1,1x1,1x1 video-001.bmp > video-001.q50.422.jpeg
cjpeg -quality 50 -sample 1x1,1x1,1x1 video-001.bmp > video-001.q50.444.jpeg
cjpeg -quality 50 -sample 2x2,1x1,1x1 -progressive video-001.bmp >
video-001.q50.420.progressive.jpeg
cjpeg -quality 50 -sample 2x1,1x1,1x1 -progressive video-001.bmp >
video-001.q50.422.progressive.jpeg
cjpeg -quality 50 -sample 1x1,1x1,1x1 -progressive video-001.bmp >
video-001.q50.444.progressive.jpeg
cjpeg -quality 50 video-005.gray.pgm > video-005.gray.q50.jpeg
cjpeg -quality 50 -progressive video-005.gray.pgm >
video-005.gray.q50.progressive.jpeg
# Delete intermediate files.
rm video-001.bmp video-005.gray.pgm

R=r
CC=golang-dev
http://codereview.appspot.com/6684046