code review 4186064: image/jpeg: add encoder

image/jpeg: add encoder

[raph, 2011-02-21 01:47:19]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[nigeltao, 2011-02-21 04:18:21]

Just a bunch of very quick, mostly superficial comments for now. I'll have a
proper look at the actual algorithm later.

Also, it might be nice to have a writer_test that starts with a test pattern
image, encodes it, decodes it, and checks that the result is reasonably similar
to the original test pattern. image/png/writer_test.go has some code that you
could re-use for that.

Adding R=r since I think Rob was also interested in a JPEG encoder.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go
File src/pkg/image/jpeg/writer.go (right):

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:12: // "fmt"
Remove this, as well as the commented out fmt calls below.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:179: // By spec, 1 <= nbits <= 16
Can you be more specific about which part of the spec? For example, see the
comments in image/jpeg/reader.go.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:460: type UnknownColorModelError string
var (
  ErrUnknownColorMode = os.NewError("jpeg: unknown color model")
  ErrUnknownSubsamplingMethod = os.NewError("jpeg: unknown subsampling method")
)

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:473: subsample int
These fields need to start with a capital letter to be used outside of the jpeg
package.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:522: var e encoder
We usually use a var ( ... ) block instead of multiple lines starting with var.
Same elsewhere in this file.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:525: e.w = bw
e.w = bufio.NewWriter(w) and delete the two lines above.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:540: return UnknownColorModelError("")
I don't understand this restriction. Can't we encode a paletted image as a
non-paletted JPEG, since a paletted image implements At(x, y).RGBA()?

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:542: e.writeHeader()
All these write methods should return an os.Error, and if it returs non-nil you
should pass it up the call chain.

[r, 2011-02-22 19:54:46]

a few cosmetic things. i haven't dug in yet

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/fdct.go
File src/pkg/image/jpeg/fdct.go (right):

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/fdct.go#ne...
src/pkg/image/jpeg/fdct.go:31: for y := 0; y < 8; y++ {
probably want (or maybe have) a constant for 8, since we have one for 64,
however 8 is also easy to understand and the code uses 0, 1, 2, 3, 4, 5, 6, 7
too. maybe just remind with a comment that blockSize is 8*8.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/fdct.go#ne...
src/pkg/image/jpeg/fdct.go:45: tmp3 = b[y*8+3] - b[y*8+4]
this code is fine for now but you can avoid half of the indexing operations.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go
File src/pkg/image/jpeg/writer.go (right):

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:24: type encoder struct {
they're obvious in meaning but still, these fields should have little comments

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:42: bitsDcLuminance = []byte{0, 1, 5, 1, 1, 1, 1,
1, 1, 0, 0, 0, 0, 0, 0, 0}
if you size these guys (you can use [...]) they'll be arrays, not slices, and
indexing can be more efficient.
but if you're not sure of the tradeoffs, leave them as is for now.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:211: 0x64, 0x00, 0x64, 0x00, 0x00})
these should be defined once  - no need to reallocate these objects every time

[raph, 2011-03-26 22:29:55]

On 2011/02/21 04:18:21, nigeltao wrote:
> Just a bunch of very quick, mostly superficial comments for now. I'll have a
> proper look at the actual algorithm later.
> 
> Also, it might be nice to have a writer_test that starts with a test pattern
> image, encodes it, decodes it, and checks that the result is reasonably
similar
> to the original test pattern. image/png/writer_test.go has some code that you
> could re-use for that.

I agree that's a good idea. Can I do that in a separate change?

> Adding R=r since I think Rob was also interested in a JPEG encoder.
> 
> http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go
> File src/pkg/image/jpeg/writer.go (right):
> 
>
http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:12: // "fmt"
> Remove this, as well as the commented out fmt calls below.

Done.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:179: // By spec, 1 <= nbits <= 16
> Can you be more specific about which part of the spec? For example, see the
> comments in image/jpeg/reader.go.

Done.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:460: type UnknownColorModelError string
> var (
>   ErrUnknownColorMode = os.NewError("jpeg: unknown color model")
>   ErrUnknownSubsamplingMethod = os.NewError("jpeg: unknown subsampling
method")
> )

Done.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:473: subsample int
> These fields need to start with a capital letter to be used outside of the
jpeg
> package.

Good catch. Done.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:522: var e encoder
> We usually use a var ( ... ) block instead of multiple lines starting with
var.
> Same elsewhere in this file.

Not relevant, as code has changed.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:525: e.w = bw
> e.w = bufio.NewWriter(w) and delete the two lines above.

Done.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:540: return UnknownColorModelError("")
> I don't understand this restriction. Can't we encode a paletted image as a
> non-paletted JPEG, since a paletted image implements At(x, y).RGBA()?

Done. Perhaps I've now made it too lenient, although it feels to me that
converting arbitrary images into RGB is reasonable.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:542: e.writeHeader()
> All these write methods should return an os.Error, and if it returs non-nil
you
> should pass it up the call chain.

Done, but with some trepidation. My motivation in originally not doing so was to
rely on the guarantee by bufio.Writer that if a write to the underlying writer
fails, subsequent writes woud be swallowed, but that the err would be available
later, particularly on the Flush call. Relying on this simplifies the code
significantly, because there are now 42 instances of checking e.err, or 19% of
the total number of lines of code (!).

But I discussed this with Andrew Gerrand, and we came to the conclusion that it
was, on balance, the best thing to do, so here it is.

[raph, 2011-03-26 22:33:38]

On 2011/02/22 19:54:46, r wrote:
> a few cosmetic things. i haven't dug in yet
> 
> http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/fdct.go
> File src/pkg/image/jpeg/fdct.go (right):
> 
>
http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/fdct.go#ne...
> src/pkg/image/jpeg/fdct.go:31: for y := 0; y < 8; y++ {
> probably want (or maybe have) a constant for 8, since we have one for 64,
> however 8 is also easy to understand and the code uses 0, 1, 2, 3, 4, 5, 6, 7
> too. maybe just remind with a comment that blockSize is 8*8.

Done (comment). Here, I was following the example of idct.go. Generally I feel
it's most important to be consistent.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/fdct.go#ne...
> src/pkg/image/jpeg/fdct.go:45: tmp3 = b[y*8+3] - b[y*8+4]
> this code is fine for now but you can avoid half of the indexing operations.

Done. Of course, a good compiler would optimize that for you, but in the
meantime...

> http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go
> File src/pkg/image/jpeg/writer.go (right):
> 
>
http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:24: type encoder struct {
> they're obvious in meaning but still, these fields should have little comments

Done.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:42: bitsDcLuminance = []byte{0, 1, 5, 1, 1, 1, 1,
> 1, 1, 0, 0, 0, 0, 0, 0, 0}
> if you size these guys (you can use [...]) they'll be arrays, not slices, and
> indexing can be more efficient.
> but if you're not sure of the tradeoffs, leave them as is for now.

I tried this, but then passing the sized byte array to e.w.Write() failed,
because that expects a slice. Given the choice, it seems like a better idea not
to allocate a new slice on write, but that's arguable either way. Also see above
wrt good compiler.

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
> src/pkg/image/jpeg/writer.go:211: 0x64, 0x00, 0x64, 0x00, 0x00})
> these should be defined once  - no need to reallocate these objects every time

Done.

[raph, 2011-03-26 22:34:45]

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

Please take another look.

[r2, 2011-03-26 23:32:15]

> 
> 
>
http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
>> src/pkg/image/jpeg/writer.go:42: bitsDcLuminance = []byte{0, 1, 5, 1,
> 1, 1, 1,
>> 1, 1, 0, 0, 0, 0, 0, 0, 0}
>> if you size these guys (you can use [...]) they'll be arrays, not
> slices, and
>> indexing can be more efficient.
>> but if you're not sure of the tradeoffs, leave them as is for now.
> 
> I tried this, but then passing the sized byte array to e.w.Write()
> failed, because that expects a slice. Given the choice, it seems like a
> better idea not to allocate a new slice on write, but that's arguable
> either way. Also see above wrt good compiler.

You don't allocate a slice. All that happens is a slice header, on the stack,
gets filled in.  It's super cheap.

-rob

[raph, 2011-03-27 00:03:13]

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

Please take another look.

[raph, 2011-03-27 00:05:18]

Ok, I'm hoping this is ready now. Still working on writing tests...

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go
File src/pkg/image/jpeg/writer.go (right):

http://codereview.appspot.com/4186064/diff/2001/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:42: bitsDcLuminance = []byte{0, 1, 5, 1, 1, 1, 1,
1, 1, 0, 0, 0, 0, 0, 0, 0}
On 2011/02/22 19:54:46, r wrote:
> if you size these guys (you can use [...]) they'll be arrays, not slices, and
> indexing can be more efficient.
> but if you're not sure of the tradeoffs, leave them as is for now.

Ok, done. Thanks for the advice re stack allocation.

[raph, 2011-03-27 01:33:30]

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

Please take another look.

[raph, 2011-03-27 01:49:33]

On 2011/03/27 01:33:30, raph wrote:
> Hello nigeltao, r, r2 (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Sorry for the clumsiness in using the codereview tool. I had to dig a bit deeper
to figure out how to add files to an existing CL, and separate upload from mail
so I could write meaningful comments. In any case, a roundtrip test is now
added, as per Nigel's request.

[nigeltao, 2011-03-29 08:40:55]

You could also cut down on the number of intermediate allocations by adding a
few (re-usable) buffers to the encoder struct, but there's more than enough to
chew on for now.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/Makefile
File src/pkg/image/jpeg/Makefile (right):

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/Makefile#...
src/pkg/image/jpeg/Makefile:11: fdct.go\
Please keep this list sorted alphabetically.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go
File src/pkg/image/jpeg/fdct.go (right):

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go#n...
src/pkg/image/jpeg/fdct.go:8: // distribution
Add a trailing full stop to complete the sentence.

Also, can you add a link or a definitive reference to the IJG distribution?

Finally, can you confirm the licensing requirements for the IJG code?

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go#n...
src/pkg/image/jpeg/fdct.go:11: fix_0_298631336 = 2446  /* FIX(0.298631336) */
I'd drop the /* FIX(...) */ comments. They don't seem to add anything above the
constant name.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go#n...
src/pkg/image/jpeg/fdct.go:31: // Pass 1: process rows
Trailing full stop, and again below in many places.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go#n...
src/pkg/image/jpeg/fdct.go:65: // Odd part per figure 8
Figure 8 of what?

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go#n...
src/pkg/image/jpeg/fdct.go:70: z1 = (tmp12 + tmp13) * fix_1_175875602 /*  c3 */
I'd drop the /* c3 */ comments unless you give some context somewhere for what
c3 means.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/fdct.go#n...
src/pkg/image/jpeg/fdct.go:91: // of 8
Move "of 8." to the previous comment line.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go
File src/pkg/image/jpeg/writer.go (right):

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:23: type encoder struct {
Please move the definition of the encoder type to line 178, just above its
methods.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:47: 0x01, 0x02, 0x03, 0x00, 0x04, 0x11, 0x05, 0x12,
Is it possible to document these magic numbers? Do they come from the JPEG spec?
There isn't an obvious pattern, AFAICT.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:178: // According to Annex C of the spec, Huffman
code lenghts greater than 16 bits
Typo in "lengths".

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:187: e.err = e.w.WriteByte(b)
Rather than having an e.err field, I think it is more idiomatic for all such
methods to return an os.Error.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:218: _, e.err = e.w.Write([]byte{0xff, 0xd8}) //
SOI marker
reader.go already defines const soiMarker = 0xd8. You should be able to replace
quite a few magic numbers in this file with named constants.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:325: func roundDivide(a int, b int) int {
s/a int, b int/a, b int/

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:332: func (e *encoder) writeBlock(b
*[blockSize]int, tab int, last int) int {
Given the number of times you use a [blockSize]int, it might be useful to
declare a "block" type above:
type block [blockSize]int

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:402: FIX_0_299   = 19595
It is possible to harmonize the style used in rgbToYCbCr here and the calcPixel
method in reader.go?

If not, at the very least, these constants' names should start with a lower case
letter so that they are not exported. You can also move them inside the
rgbToYCbCr function.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:414: // Convert one 8x8 color image block to YCbCr
// rgbToYCbCr converts an 8x8 color block to YCbCr.

Doc comments should start with the name of the thing they are documenting, and
end with a full stop. Similarly, the comment for roundDivide above should start
with:

// roundDivide returns a/b rounded to the nearest int, instead of rounded down.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:415: func rgbtoYCbCr(m image.Image, x0 int, y0 int,
yblock *[blockSize]int, cbblock *[blockSize]int, crblock *[blockSize]int) {
s/rgbto/rgbTo/

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:430: // Note: just shifting creates a small error
for proper 16-bit images
What's the error? Do you mean to do something like
r := int((ur + 128) >> 8)?

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:445: var color444SOI = []byte{0xff, 0xda, 0x00,
0x0c, 0x03, 0x01, 0x00, 0x02, 0x11,
Can you add a comment explaining these magic numbers?

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:453: m := e.m
This variable doesn't seem to be pulling its weight. Just use e.m below.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:455: var yblock [blockSize]int
var (
  yblock, cbblock, crblock [blockSize]int
  lasty, lastcr, lastcb int
)

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:507: cbblock[i] = new([blockSize]int)
Why is cbblock a [4]*[blockSize]int and not a [4][blockSize]int (and similarly,
scaleBlock's second argument should become a *[4][blockSize]int)?

If you do the latter, you can still get to the address of an element like
&cbblock[0] to pass to rgbToYCbCr.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:586: if quality <= 0 {
if quality < 1 {

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:605: if val <= 0 {
if val < 1 {

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer.go...
src/pkg/image/jpeg/writer.go:624: e.w = bufio.NewWriter(w)
I wouldn't add buffering if w was already buffered, and *bufio.Writer isn't the
only Writer implementation that is buffered.

I would define a writer interface and check for that, like what
compress/lzw/writer.go does.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer_te...
File src/pkg/image/jpeg/writer_test.go (right):

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer_te...
src/pkg/image/jpeg/writer_test.go:58: "video-001",
I'd drop the basn cases, just use "../testdata/video-001.png" here, and remove
the src/pkg/image/jpeg/testdata/images/* files that you were going to add.

If you want more photographic test images, we could probably use some from
http://code.google.com/speed/webp/gallery.html, but that should be a separate
changelist.

Similarly, we can add grayscale imagery in a separate changelist.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer_te...
src/pkg/image/jpeg/writer_test.go:62: // Also note, exact pixel tolerance should
be readjusted emprically as more test images are added.
Typo in empirically.

http://codereview.appspot.com/4186064/diff/21001/src/pkg/image/jpeg/writer_te...
src/pkg/image/jpeg/writer_test.go:63: var tolerances = []int{16 * 256, 84 * 256}
Wow, 84/256 in chroma-space even at high quality!? How does libjpeg's encoding
compare?