code review 4435051: image/jpeg: add an encoder.

image/jpeg: add an encoder.

It is based on changeset 4186064 by Raph Levien <raph@google.com>.

[nigeltao, 2011-04-18 09:30:00]

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

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

[nigeltao_gnome, 2011-04-18 09:32:59]

On 18 April 2011 19:30,  <nigeltao@golang.org> wrote:
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
> Description:
> image/jpeg: add an encoder.
>
> It is based on changeset 4186064 by Raph Levien <raph@google.com>.


This changeset is simpler than Raph's original changeset, in that it
only does 4:2:0 and not 4:4:4 chroma subsampling, and does not encode
grayscale images differently from color images.

Those features can be added in a future changeset, but I've tried to
keep this initial encoder check-in as simple as possible.

[r, 2011-04-18 19:30:42]

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

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:174: // Each value maps to an uint32 of which the 8
most significant bits hold the
s/an/a/

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:217: // e.err and return void.
void?

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:247: // encoder encodes an image to the JPEG
format.
move this above its methods and combine the comments

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:260: // emit emits the least significant nBits bits
of bits to the bitstream.
nBits bits of bits to the bits. wow.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:303: // writeMarkerHeader writes the header for a
maker with the given length.
s/maker/marker/

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:308: e.writeByte(uint8(markerlen & 0xff))
this is a fair bit of overhead. we could put a [4]byte into encoder and do a
single Write call.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:332: e.writeByte(uint8(size.X & 0xff))
ditto.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:338: e.writeByte("\x00\x01\x01"[i])
ditto

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:352: e.write(s.value)
ditto

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:425:
e.writeString("\xff\xda\x00\x0c\x03\x01\x00\x02\x11\x03\x11\x00\x00\x00")
depending on the implementation of the writer, writeString may copy.  better
than this approach is to have a global
var sosHeader = []byte("....")

also i'm not sure you're getting much readability from turning these bytes into
strings.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:431: cBlock  block
ok for now, but these are more unnecessary allocations.
probably not important, because we only do one SOS per image (right?)

[nigeltao, 2011-04-19 00:47:06]

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

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:174: // Each value maps to an uint32 of which the 8
most significant bits hold the
On 2011/04/18 19:30:42, r wrote:
> s/an/a/

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:217: // e.err and return void.
On 2011/04/18 19:30:42, r wrote:
> void?

Rewritten.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:247: // encoder encodes an image to the JPEG
format.
On 2011/04/18 19:30:42, r wrote:
> move this above its methods and combine the comments

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:303: // writeMarkerHeader writes the header for a
maker with the given length.
On 2011/04/18 19:30:42, r wrote:
> s/maker/marker/

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:308: e.writeByte(uint8(markerlen & 0xff))
On 2011/04/18 19:30:42, r wrote:
> this is a fair bit of overhead. we could put a [4]byte into encoder and do a
> single Write call.

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:332: e.writeByte(uint8(size.X & 0xff))
On 2011/04/18 19:30:42, r wrote:
> ditto.

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:338: e.writeByte("\x00\x01\x01"[i])
On 2011/04/18 19:30:42, r wrote:
> ditto

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:352: e.write(s.value)
On 2011/04/18 19:30:42, r wrote:
> ditto

I don't think this applies here; s.value is a slice. Besides, the underlying
writer e.w is presumably buffered (since it implements a WriteByte method), so
I'm not sure if adding additional buffering would be a significant win.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:425:
e.writeString("\xff\xda\x00\x0c\x03\x01\x00\x02\x11\x03\x11\x00\x00\x00")
On 2011/04/18 19:30:42, r wrote:
> depending on the implementation of the writer, writeString may copy.  better
> than this approach is to have a global
> var sosHeader = []byte("....")
> 
> also i'm not sure you're getting much readability from turning these bytes
into
> strings.

Done.

http://codereview.appspot.com/4435051/diff/1002/src/pkg/image/jpeg/writer.go#...
src/pkg/image/jpeg/writer.go:431: cBlock  block
On 2011/04/18 19:30:42, r wrote:
> ok for now, but these are more unnecessary allocations.
> probably not important, because we only do one SOS per image (right?)

There is only one SOS per image. Also, eventually, escape analysis should be
able to discover that these blocks are stack-allocatable.

[r, 2011-04-19 00:52:14]

LGTM

[nigeltao, 2011-04-19 01:00:55]

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

image/jpeg: add an encoder.

It is based on changeset 4186064 by Raph Levien <raph@google.com>.

R=r, nigeltao_gnome
CC=golang-dev
http://codereview.appspot.com/4435051