Issue 105990046: image/jpeg: Encode *image.Gray as grayscale JPEGs.
Descriptionimage/jpeg: Encode *image.Gray as grayscale JPEGs.
Fixes issue 8201.
Patch Set 1 #Patch Set 2 : diff -r 6146799f32ed https://code.google.com/p/go #Patch Set 3 : diff -r 6146799f32ed https://code.google.com/p/go #Patch Set 4 : diff -r 6146799f32ed https://code.google.com/p/go #
Total comments: 6
Patch Set 5 : diff -r 6146799f32ed https://code.google.com/p/go #
Total comments: 14
Patch Set 6 : diff -r 6a85be121cae https://code.google.com/p/go #Patch Set 7 : diff -r 6a85be121cae https://code.google.com/p/go #
Total comments: 3
MessagesTotal messages: 9
https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer.go File src/pkg/image/jpeg/writer.go (right): https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:449: // - the marker length "\x00\x08", s/tabs/spaces/ to be consistent with sosHeaderYCbCr. https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... File src/pkg/image/jpeg/writer_test.go (right): https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:162: // Test grayscale images survive round-trip through encode/decode cycle. Make this a separate test: func TestWriteGrayscale(t *testing.T) { https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:171: if err := Encode(&buf, m0, nil); err != nil { Don't encode twice.
Sign in to reply to this message.
FYI, in the future, please run "hg mail 105990046" to actually mail out the code review.
Sign in to reply to this message.
Sorry I messed up the normal work flow. I stopped reading the Contribution Guideline prematurely when I hit a section that didn't apply to me and had assumed when the server created a CL that the notification magic had already happened. Can you offer some advice on my TODO in writeDHT? I'm new to how JPEG works and would benefit from your experience. https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer.go File src/pkg/image/jpeg/writer.go (right): https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:449: // - the marker length "\x00\x08", On 2014/06/15 12:49:01, nigeltao wrote: > s/tabs/spaces/ to be consistent with sosHeaderYCbCr. I _think_ sosHeaderYCbCr is using tabs too, it's just that reitveld only shows the >> for code in the diff. https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... File src/pkg/image/jpeg/writer_test.go (right): https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:162: // Test grayscale images survive round-trip through encode/decode cycle. On 2014/06/15 12:49:01, nigeltao wrote: > Make this a separate test: > func TestWriteGrayscale(t *testing.T) { Done. https://codereview.appspot.com/105990046/diff/60001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:171: if err := Encode(&buf, m0, nil); err != nil { On 2014/06/15 12:49:01, nigeltao wrote: > Don't encode twice. Done.
Sign in to reply to this message.
https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer.go File src/pkg/image/jpeg/writer.go (right): https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:327: e.buf[3*i+7] = "\x22\x11\x11"[i] Strictly speaking, I think that the "22" should be "11" if there's only one component. There is no 4:2:0 chroma subsampling if we have no chroma. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:335: // TODO(wathiede): only write half of these when encoding grayscale? Yeah, I think you should be able to say specs := theHuffmanSpec[:] if nComponent == 1 { // Drop the Chrominance tables. specs = specs[:2] } You should be able to check that the resultant JPEG looks OK in anything using libjpeg: Google Chrome, the GIMP, eog, etc. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:494: for i := 0; i < 4; i++ { I don't think this inner i loop is correct for a 1-component image (and the += 16s above should be += 8). You might want to check that a grayscale image encoded by your code actually looks right in another image viewer. IIRC, the luma blocks in a luma-only JPEG should be written in this order: 1234 5678 instead of the order for a 4:2:0 luma/chroma JPEG: 1256 3478 since the MCU is 1x1 blocks instead of 2x2 blocks. It would be good if the unit test actually checks that the round-trip keeps the pixels in the right order. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:582: var nComponent int Easier might be nComponent := 3 switch m.(type) { etc } with no default case. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:584: case *image.Gray: We might eventually want to switch on m.ColorModel() instead of m.(type), but this is OK for a first CL. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... File src/pkg/image/jpeg/writer_test.go (right): https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:164: // Test grayscale images survive round-trip through encode/decode cycle. You could pull this up one line and make it a doc comment. // TestWriteGrayscale tests that a grayscale image survives a round-trip // through the encode/decode cycle. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:181: t.Errorf("want decoded image type *image.Gray, got %T", m1) I'd say t.Errorf("got %T, want *image.Gray", m1)
Sign in to reply to this message.
PTAL https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer.go File src/pkg/image/jpeg/writer.go (right): https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:327: e.buf[3*i+7] = "\x22\x11\x11"[i] On 2014/06/16 05:03:15, nigeltao wrote: > Strictly speaking, I think that the "22" should be "11" if there's only one > component. There is no 4:2:0 chroma subsampling if we have no chroma. Done. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:335: // TODO(wathiede): only write half of these when encoding grayscale? On 2014/06/16 05:03:14, nigeltao wrote: > Yeah, I think you should be able to say > > specs := theHuffmanSpec[:] > if nComponent == 1 { > // Drop the Chrominance tables. > specs = specs[:2] > } > > You should be able to check that the resultant JPEG looks OK in anything using > libjpeg: Google Chrome, the GIMP, eog, etc. Done. Tested with Chrome, gimp, and feh on Linux. All are happy now. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:494: for i := 0; i < 4; i++ { On 2014/06/16 05:03:14, nigeltao wrote: > I don't think this inner i loop is correct for a 1-component image (and the += > 16s above should be += 8). You might want to check that a grayscale image > encoded by your code actually looks right in another image viewer. IIRC, the > luma blocks in a luma-only JPEG should be written in this order: > > 1234 > 5678 > > instead of the order for a 4:2:0 luma/chroma JPEG: > > 1256 > 3478 > > since the MCU is 1x1 blocks instead of 2x2 blocks. It would be good if the unit > test actually checks that the round-trip keeps the pixels in the right order. Done. Also added an averageDelta check after the round-trip, should be a good proxy for 'pixels in the right order'. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:582: var nComponent int On 2014/06/16 05:03:14, nigeltao wrote: > Easier might be > nComponent := 3 > switch m.(type) { > etc > } > with no default case. Done. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer.go:584: case *image.Gray: On 2014/06/16 05:03:14, nigeltao wrote: > We might eventually want to switch on m.ColorModel() instead of m.(type), but > this is OK for a first CL. Done. Added TODO. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... File src/pkg/image/jpeg/writer_test.go (right): https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:164: // Test grayscale images survive round-trip through encode/decode cycle. On 2014/06/16 05:03:15, nigeltao wrote: > You could pull this up one line and make it a doc comment. > > // TestWriteGrayscale tests that a grayscale image survives a round-trip > // through the encode/decode cycle. Done. https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... src/pkg/image/jpeg/writer_test.go:181: t.Errorf("want decoded image type *image.Gray, got %T", m1) On 2014/06/16 05:03:15, nigeltao wrote: > I'd say > t.Errorf("got %T, want *image.Gray", m1) Done.
Sign in to reply to this message.
On 2014/06/17 03:56:10, wathiede wrote: > PTAL > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer.go > File src/pkg/image/jpeg/writer.go (right): > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer.go:327: e.buf[3*i+7] = "\x22\x11\x11"[i] > On 2014/06/16 05:03:15, nigeltao wrote: > > Strictly speaking, I think that the "22" should be "11" if there's only one > > component. There is no 4:2:0 chroma subsampling if we have no chroma. > > Done. > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer.go:335: // TODO(wathiede): only write half of these > when encoding grayscale? > On 2014/06/16 05:03:14, nigeltao wrote: > > Yeah, I think you should be able to say > > > > specs := theHuffmanSpec[:] > > if nComponent == 1 { > > // Drop the Chrominance tables. > > specs = specs[:2] > > } > > > > You should be able to check that the resultant JPEG looks OK in anything using > > libjpeg: Google Chrome, the GIMP, eog, etc. > > Done. Tested with Chrome, gimp, and feh on Linux. All are happy now. > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer.go:494: for i := 0; i < 4; i++ { > On 2014/06/16 05:03:14, nigeltao wrote: > > I don't think this inner i loop is correct for a 1-component image (and the += > > 16s above should be += 8). You might want to check that a grayscale image > > encoded by your code actually looks right in another image viewer. IIRC, the > > luma blocks in a luma-only JPEG should be written in this order: > > > > 1234 > > 5678 > > > > instead of the order for a 4:2:0 luma/chroma JPEG: > > > > 1256 > > 3478 > > > > since the MCU is 1x1 blocks instead of 2x2 blocks. It would be good if the > unit > > test actually checks that the round-trip keeps the pixels in the right order. > > Done. Also added an averageDelta check after the round-trip, should be a good > proxy for 'pixels in the right order'. > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer.go:582: var nComponent int > On 2014/06/16 05:03:14, nigeltao wrote: > > Easier might be > > nComponent := 3 > > switch m.(type) { > > etc > > } > > with no default case. > > Done. > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer.go:584: case *image.Gray: > On 2014/06/16 05:03:14, nigeltao wrote: > > We might eventually want to switch on m.ColorModel() instead of m.(type), but > > this is OK for a first CL. > > Done. Added TODO. > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > File src/pkg/image/jpeg/writer_test.go (right): > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer_test.go:164: // Test grayscale images survive > round-trip through encode/decode cycle. > On 2014/06/16 05:03:15, nigeltao wrote: > > You could pull this up one line and make it a doc comment. > > > > // TestWriteGrayscale tests that a grayscale image survives a round-trip > > // through the encode/decode cycle. > > Done. > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > src/pkg/image/jpeg/writer_test.go:181: t.Errorf("want decoded image type > *image.Gray, got %T", m1) > On 2014/06/16 05:03:15, nigeltao wrote: > > I'd say > > t.Errorf("got %T, want *image.Gray", m1) > > Done. I have officially confused myself. I noticed I left a logging statement in writeSOS, and when I remove it, it changes how the output image is detected by identify(1) from the imagemagick package, using the test program http://play.golang.org/p/62k1M0Vu6p saved as rwtest.go: # With logging statement go run rwjpeg.go < input-rgb.jpg > gray.jpg && identify gray.jpg 2014/06/17 07:37:15 jpeg type *image.YCbCr -> *image.Gray 2014/06/17 07:37:17 writeSOS *image.Gray gray.jpg JPEG 5616x3744 5616x3744+0+0 8-bit Gray 256c 858KB 0.000u 0:00.000 $ go run rwjpeg.go < input-gray.jpg > gray.jpg && identify gray.jpg 2014/06/17 07:37:23 jpeg type *image.Gray -> *image.Gray 2014/06/17 07:37:23 writeSOS *image.Gray gray.jpg JPEG 1601x1069 1601x1069+0+0 8-bit Gray 256c 203KB 0.000u 0:00.000 # Without logging statement $ go run rwjpeg.go < input-rgb.jpg > gray.jpg && identify gray.jpg 2014/06/17 07:37:36 jpeg type *image.YCbCr -> *image.Gray gray.jpg JPEG 5616x3744 5616x3744+0+0 8-bit sRGB 946KB 0.000u 0:00.000 $ go run rwjpeg.go < input-gray.jpg > gray.jpg && identify gray.jpg 2014/06/17 07:37:42 jpeg type *image.Gray -> *image.Gray gray.jpg JPEG 1601x1069 1601x1069+0+0 8-bit sRGB 213KB 0.000u 0:00.000 Gimp seems to confirm the output is now sRGB too. Do you have any suggestions on how to debug this?
Sign in to reply to this message.
On 2014/06/17 14:40:02, wathiede wrote: > On 2014/06/17 03:56:10, wathiede wrote: > > PTAL > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer.go > > File src/pkg/image/jpeg/writer.go (right): > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer.go:327: e.buf[3*i+7] = "\x22\x11\x11"[i] > > On 2014/06/16 05:03:15, nigeltao wrote: > > > Strictly speaking, I think that the "22" should be "11" if there's only one > > > component. There is no 4:2:0 chroma subsampling if we have no chroma. > > > > Done. > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer.go:335: // TODO(wathiede): only write half of these > > when encoding grayscale? > > On 2014/06/16 05:03:14, nigeltao wrote: > > > Yeah, I think you should be able to say > > > > > > specs := theHuffmanSpec[:] > > > if nComponent == 1 { > > > // Drop the Chrominance tables. > > > specs = specs[:2] > > > } > > > > > > You should be able to check that the resultant JPEG looks OK in anything > using > > > libjpeg: Google Chrome, the GIMP, eog, etc. > > > > Done. Tested with Chrome, gimp, and feh on Linux. All are happy now. > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer.go:494: for i := 0; i < 4; i++ { > > On 2014/06/16 05:03:14, nigeltao wrote: > > > I don't think this inner i loop is correct for a 1-component image (and the > += > > > 16s above should be += 8). You might want to check that a grayscale image > > > encoded by your code actually looks right in another image viewer. IIRC, the > > > luma blocks in a luma-only JPEG should be written in this order: > > > > > > 1234 > > > 5678 > > > > > > instead of the order for a 4:2:0 luma/chroma JPEG: > > > > > > 1256 > > > 3478 > > > > > > since the MCU is 1x1 blocks instead of 2x2 blocks. It would be good if the > > unit > > > test actually checks that the round-trip keeps the pixels in the right > order. > > > > Done. Also added an averageDelta check after the round-trip, should be a good > > proxy for 'pixels in the right order'. > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer.go:582: var nComponent int > > On 2014/06/16 05:03:14, nigeltao wrote: > > > Easier might be > > > nComponent := 3 > > > switch m.(type) { > > > etc > > > } > > > with no default case. > > > > Done. > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer.go:584: case *image.Gray: > > On 2014/06/16 05:03:14, nigeltao wrote: > > > We might eventually want to switch on m.ColorModel() instead of m.(type), > but > > > this is OK for a first CL. > > > > Done. Added TODO. > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > File src/pkg/image/jpeg/writer_test.go (right): > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer_test.go:164: // Test grayscale images survive > > round-trip through encode/decode cycle. > > On 2014/06/16 05:03:15, nigeltao wrote: > > > You could pull this up one line and make it a doc comment. > > > > > > // TestWriteGrayscale tests that a grayscale image survives a round-trip > > > // through the encode/decode cycle. > > > > Done. > > > > > https://codereview.appspot.com/105990046/diff/80001/src/pkg/image/jpeg/writer... > > src/pkg/image/jpeg/writer_test.go:181: t.Errorf("want decoded image type > > *image.Gray, got %T", m1) > > On 2014/06/16 05:03:15, nigeltao wrote: > > > I'd say > > > t.Errorf("got %T, want *image.Gray", m1) > > > > Done. > > I have officially confused myself. I noticed I left a logging statement in > writeSOS, and when I remove it, it changes how the output image is detected by > identify(1) from the imagemagick package, using the test program > http://play.golang.org/p/62k1M0Vu6p saved as rwtest.go: > > # With logging statement > go run rwjpeg.go < input-rgb.jpg > gray.jpg && identify gray.jpg > 2014/06/17 07:37:15 jpeg type *image.YCbCr -> *image.Gray > 2014/06/17 07:37:17 writeSOS *image.Gray > gray.jpg JPEG 5616x3744 5616x3744+0+0 8-bit Gray 256c 858KB 0.000u 0:00.000 > > $ go run rwjpeg.go < input-gray.jpg > gray.jpg && identify gray.jpg > 2014/06/17 07:37:23 jpeg type *image.Gray -> *image.Gray > 2014/06/17 07:37:23 writeSOS *image.Gray > gray.jpg JPEG 1601x1069 1601x1069+0+0 8-bit Gray 256c 203KB 0.000u 0:00.000 > > # Without logging statement > $ go run rwjpeg.go < input-rgb.jpg > gray.jpg && identify gray.jpg > 2014/06/17 07:37:36 jpeg type *image.YCbCr -> *image.Gray > gray.jpg JPEG 5616x3744 5616x3744+0+0 8-bit sRGB 946KB 0.000u 0:00.000 > > $ go run rwjpeg.go < input-gray.jpg > gray.jpg && identify gray.jpg > 2014/06/17 07:37:42 jpeg type *image.Gray -> *image.Gray > gray.jpg JPEG 1601x1069 1601x1069+0+0 8-bit sRGB 213KB 0.000u 0:00.000 > > Gimp seems to confirm the output is now sRGB too. Do you have any suggestions > on how to debug this? Apparently there was a cached object somewhere. Removed $GOROOT/pkg/* and reran $GOROOT/src/make.bash and now it is generating Gray JPGs without the log statement.
Sign in to reply to this message.
LGTM. I'll make the trivial changes and submit. https://codereview.appspot.com/105990046/diff/120001/src/pkg/image/jpeg/write... File src/pkg/image/jpeg/writer.go (right): https://codereview.appspot.com/105990046/diff/120001/src/pkg/image/jpeg/write... src/pkg/image/jpeg/writer.go:359: // writeBlock writes a block of pixel data using the given If anything, I would break it as: // writeBlock writes a block of pixel data using the given quantization table, // returning the post-quantized DC value of the DCT-transformed block. b is in // natural (not zig-zag) order. https://codereview.appspot.com/105990046/diff/120001/src/pkg/image/jpeg/write... File src/pkg/image/jpeg/writer_test.go (right): https://codereview.appspot.com/105990046/diff/120001/src/pkg/image/jpeg/write... src/pkg/image/jpeg/writer_test.go:172: t.Error(err) t.Fatal(err) https://codereview.appspot.com/105990046/diff/120001/src/pkg/image/jpeg/write... src/pkg/image/jpeg/writer_test.go:179: t.Errorf("bounds differ: %v and %v", m0.Bounds(), m1.Bounds()) t.Fatalf, as there's no point continuing if the bounds are different. averageDelta requires its two arguments to have the same bounds.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=f39dff1bf0d4 *** image/jpeg: encode *image.Gray as grayscale JPEGs. Fixes issue 8201. LGTM=nigeltao R=nigeltao CC=golang-codereviews https://codereview.appspot.com/105990046 Committer: Nigel Tao <nigeltao@golang.org>
Sign in to reply to this message.
|
