FAQ
Reviewers: r,

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

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


Description:
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 seems to cost 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

Please review this at http://codereview.appspot.com/6684046/

Affected files:
M src/pkg/image/decode_test.go
M src/pkg/image/jpeg/huffman.go
M src/pkg/image/jpeg/reader.go
A src/pkg/image/jpeg/reader_test.go
A src/pkg/image/jpeg/scan.go
M src/pkg/image/jpeg/writer.go
M src/pkg/image/jpeg/writer_test.go
A src/pkg/image/testdata/video-001.progressive.jpeg
A src/pkg/image/testdata/video-001.q50.420.jpeg
A src/pkg/image/testdata/video-001.q50.420.progressive.jpeg
A src/pkg/image/testdata/video-001.q50.422.jpeg
A src/pkg/image/testdata/video-001.q50.422.progressive.jpeg
A src/pkg/image/testdata/video-001.q50.444.jpeg
A src/pkg/image/testdata/video-001.q50.444.progressive.jpeg
A src/pkg/image/testdata/video-005.gray.q50.jpeg
A src/pkg/image/testdata/video-005.gray.q50.progressive.jpeg

Search Discussions

  • R at Oct 14, 2012 at 9:33 pm
    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#newcode317
    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#newcode318
    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#newcode348
    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_test.go
    File src/pkg/image/jpeg/reader_test.go (right):

    http://codereview.appspot.com/6684046/diff/6017/src/pkg/image/jpeg/reader_test.go#newcode20
    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.

    http://codereview.appspot.com/6684046/
  • Nigeltao at Oct 15, 2012 at 12:12 am
    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#newcode348
    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.

    https://codereview.appspot.com/6684046/
  • Nigeltao at Oct 15, 2012 at 12:27 am
    *** 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


    http://codereview.appspot.com/6684046/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 14, '12 at 7:31a
activeOct 15, '12 at 12:27a
posts4
users2
websitegolang.org

2 users in discussion

Nigeltao: 3 posts R: 1 post

People

Translate

site design / logo © 2021 Grokbase