code review 13407045: go.image/bmp: add Encode

go.image/bmp: add Encode

[chai2010, 2013-09-04 06:07:53]

Hello golang-dev@googlegroups.com,

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

[nigeltao, 2013-09-06 09:05:20]

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode1
bmp/writer.go:1: // Copyright 2011 The Go Authors. All rights reserved.
2013.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode42
bmp/writer.go:42: grayPalette = make([]color.Color, 256)
Don't bother with a grayPalette palette, since you don't really use it. All you
do is convert it back to bytes at line 64, so just fill in the bytes directly.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode48
bmp/writer.go:48: func writeImagePaletted(w io.Writer, m image.Image, pix
[]uint8, stride int, palette color.Palette) error {
Change "writeImage" to "encode" and similarly below.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode49
bmp/writer.go:49: step := ((m.Bounds().Dx() + 3) &^ 3)
The outer ()s are unnecessary.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode50
bmp/writer.go:50: bmpPalette := make([]bmpPaletteRGBA, 256)
I wouldn't bother with the bmpPaletteRGBA type. Just make([]byte, 1024) and fill
it in directly.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode66
bmp/writer.go:66: bmpPalette[i].r = uint8(r)
This isn't right. http://golang.org/pkg/image/color/#Color says that RGBA
returns values "within [0, 0xFFFF]". Thus, "uint8(r)" should be "uint8(r>>8)"
and similarly below.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode80
bmp/writer.go:80: max := y*stride + stride
The +stride should be +width. The width can be less than the stride. See
http://golang.org/doc/articles/image-package-05.png in
http://golang.org/doc/articles/image_package.html

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode90
bmp/writer.go:90: head := &bmpHead{
It seems like you could extract a

head := etc
if err := binary.Write(w, binary.LittleEndian, head); err != nil {
  etc
}

into func Encode instead of copy/pasting it at the start of each of
encodePaletted, encodeRGBA and encode.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode109
bmp/writer.go:109: buf[off+2] = byte(pix[i+0])
The byte is unnecessary.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode141
bmp/writer.go:141: buf[off+2] = byte(r)
r>>8.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode155
bmp/writer.go:155: switch img := m.(type) {
Idiomatic is
switch m := m.(type) {

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode162
bmp/writer.go:162: case *image.NRGBA:
This isn't right. Sending a color.NRGBA value of {0xFF, 0xFF, 0xFF, 0} through
the same code path as for a color.RGBA value would incorrectly encode a white
pixel instead of a black one.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer_test.go
File bmp/writer_test.go (right):

https://codereview.appspot.com/13407045/diff/14001/bmp/writer_test.go#newcode1
bmp/writer_test.go:1: // Copyright 2011 The Go Authors. All rights reserved.
2013.

[chai2010, 2013-09-06 10:21:16]

PTAL

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode1
bmp/writer.go:1: // Copyright 2011 The Go Authors. All rights reserved.
On 2013/09/06 09:05:20, nigeltao wrote:
> 2013.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode42
bmp/writer.go:42: grayPalette = make([]color.Color, 256)
On 2013/09/06 09:05:20, nigeltao wrote:
> Don't bother with a grayPalette palette, since you don't really use it. All
you
> do is convert it back to bytes at line 64, so just fill in the bytes directly.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode48
bmp/writer.go:48: func writeImagePaletted(w io.Writer, m image.Image, pix
[]uint8, stride int, palette color.Palette) error {
On 2013/09/06 09:05:20, nigeltao wrote:
> Change "writeImage" to "encode" and similarly below.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode49
bmp/writer.go:49: step := ((m.Bounds().Dx() + 3) &^ 3)
On 2013/09/06 09:05:20, nigeltao wrote:
> The outer ()s are unnecessary.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode50
bmp/writer.go:50: bmpPalette := make([]bmpPaletteRGBA, 256)
On 2013/09/06 09:05:20, nigeltao wrote:
> I wouldn't bother with the bmpPaletteRGBA type. Just make([]byte, 1024) and
fill
> it in directly.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode66
bmp/writer.go:66: bmpPalette[i].r = uint8(r)
On 2013/09/06 09:05:20, nigeltao wrote:
> This isn't right. http://golang.org/pkg/image/color/#Color says that RGBA
> returns values "within [0, 0xFFFF]". Thus, "uint8(r)" should be "uint8(r>>8)"
> and similarly below.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode80
bmp/writer.go:80: max := y*stride + stride
On 2013/09/06 09:05:20, nigeltao wrote:
> The +stride should be +width. The width can be less than the stride. See
> http://golang.org/doc/articles/image-package-05.png in
> http://golang.org/doc/articles/image_package.html

I think this should be +step.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode90
bmp/writer.go:90: head := &bmpHead{
On 2013/09/06 09:05:20, nigeltao wrote:
> It seems like you could extract a
> 
> head := etc
> if err := binary.Write(w, binary.LittleEndian, head); err != nil {
>   etc
> }
> 
> into func Encode instead of copy/pasting it at the start of each of
> encodePaletted, encodeRGBA and encode.

some head fields are different bettwen RGB/Gray.
do we need create a new func write head?

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode109
bmp/writer.go:109: buf[off+2] = byte(pix[i+0])
On 2013/09/06 09:05:20, nigeltao wrote:
> The byte is unnecessary.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode141
bmp/writer.go:141: buf[off+2] = byte(r)
On 2013/09/06 09:05:20, nigeltao wrote:
> r>>8.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode155
bmp/writer.go:155: switch img := m.(type) {
On 2013/09/06 09:05:20, nigeltao wrote:
> Idiomatic is
> switch m := m.(type) {

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode162
bmp/writer.go:162: case *image.NRGBA:
On 2013/09/06 09:05:20, nigeltao wrote:
> This isn't right. Sending a color.NRGBA value of {0xFF, 0xFF, 0xFF, 0} through
> the same code path as for a color.RGBA value would incorrectly encode a white
> pixel instead of a black one.

Done.

https://codereview.appspot.com/13407045/diff/14001/bmp/writer_test.go
File bmp/writer_test.go (right):

https://codereview.appspot.com/13407045/diff/14001/bmp/writer_test.go#newcode1
bmp/writer_test.go:1: // Copyright 2011 The Go Authors. All rights reserved.
On 2013/09/06 09:05:20, nigeltao wrote:
> 2013.

Done.

[nigeltao, 2013-09-09 01:44:32]

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode90
bmp/writer.go:90: head := &bmpHead{
On 2013/09/06 10:21:16, chai2010 wrote:
> some head fields are different bettwen RGB/Gray.
> do we need create a new func write head?

It doesn't need to be a function. Put it at the top of Encode:

var palette, pix []byte
h := &header {
  etc
}
switch m := m.(type) {
case *image.Gray:
  h.bpp = 8
  palette = make([]byte, 1024)
  etc
  pix = m.Pix
etc
}
h.fileSize += len(palette)
if err := binary.Write(w, binary.LittleEndian, head); err != nil {
  etc
}
switch {
case palette != nil:
  write pix for image.Gray or image.Paletted
case pix != nil:
  write pix for image.RGBA
default:
  write other images
}
return nil

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go#newcode14
bmp/writer.go:14: type bmpHead struct {
Rename bmpHead to header. We're already in package bmp.

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go#newcode34
bmp/writer.go:34: bmpPalette := make([]byte, 1024)
Drop the "bmp". We're already in package bmp.

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go#newcode73
bmp/writer.go:73: max := y*stride + step
Won't pix[min:max] panic if the image is e.g. 3 pixels wide (so step is 4)?

[chai2010, 2013-09-09 05:28:27]

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/14001/bmp/writer.go#newcode90
bmp/writer.go:90: head := &bmpHead{
On 2013/09/09 01:44:33, nigeltao wrote:
> On 2013/09/06 10:21:16, chai2010 wrote:
> > some head fields are different bettwen RGB/Gray.
> > do we need create a new func write head?
> 
> It doesn't need to be a function. Put it at the top of Encode:
> 
> var palette, pix []byte
> h := &header {
>   etc
> }
> switch m := m.(type) {
> case *image.Gray:
>   h.bpp = 8
>   palette = make([]byte, 1024)
>   etc
>   pix = m.Pix
> etc
> }
> h.fileSize += len(palette)
> if err := binary.Write(w, binary.LittleEndian, head); err != nil {
>   etc
> }
> switch {
> case palette != nil:
>   write pix for image.Gray or image.Paletted
> case pix != nil:
>   write pix for image.RGBA
> default:
>   write other images
> }
> return nil

Done.

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go#newcode14
bmp/writer.go:14: type bmpHead struct {
On 2013/09/09 01:44:33, nigeltao wrote:
> Rename bmpHead to header. We're already in package bmp.

Done.

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go#newcode34
bmp/writer.go:34: bmpPalette := make([]byte, 1024)
On 2013/09/09 01:44:33, nigeltao wrote:
> Drop the "bmp". We're already in package bmp.

Done.

https://codereview.appspot.com/13407045/diff/20001/bmp/writer.go#newcode73
bmp/writer.go:73: max := y*stride + step
On 2013/09/09 01:44:33, nigeltao wrote:
> Won't pix[min:max] panic if the image is e.g. 3 pixels wide (so step is 4)?

if stride < step {
	// use stride, but need write padding
} else {
	// use step
}

[nigeltao, 2013-09-09 07:18:25]

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go#newcode36
bmp/writer.go:36: max := y*stride + stride
This isn't right. Again, this can panic if width < stride.

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go#newcode47
bmp/writer.go:47: max := y*stride + step
This will panic if width < step && step <= stride.

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go#newcode146
bmp/writer.go:146: if err := binary.Write(w, binary.LittleEndian, palette); err
!= nil {
Guard this by
if palette != nil

[chai2010, 2013-09-09 07:42:41]

PTAL

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go#newcode36
bmp/writer.go:36: max := y*stride + stride
On 2013/09/09 07:18:26, nigeltao wrote:
> This isn't right. Again, this can panic if width < stride.

Done.

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go#newcode47
bmp/writer.go:47: max := y*stride + step
On 2013/09/09 07:18:26, nigeltao wrote:
> This will panic if width < step && step <= stride.

Done.

https://codereview.appspot.com/13407045/diff/26001/bmp/writer.go#newcode146
bmp/writer.go:146: if err := binary.Write(w, binary.LittleEndian, palette); err
!= nil {
On 2013/09/09 07:18:26, nigeltao wrote:
> Guard this by
> if palette != nil

Done.

[nigeltao, 2013-09-10 01:32:11]

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode31
bmp/writer.go:31: func encodePaletted(w io.Writer, m image.Image, pix []uint8,
stride, step int) error {
It seems like all you really want from the m argument is its bounds' dx and dy.
Pass those (as int) instead of passing an image.Image. Similarly below.

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode74
bmp/writer.go:74: for x := 0; x < m.Bounds().Dx(); x++ {
Calling m.Bounds().Dx() on every iteration of the inner loop can be inefficient:
https://codereview.appspot.com/13453043/diff/37001/tiff/reader.go#oldcode194

You should ideally call m.Bounds() only once (or pass it as an argument).

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode75
bmp/writer.go:75: r, g, b, _ := m.At(x, y).RGBA()
At(x, y) isn't right if the image's bounds do not start at (0, 0). Please read
http://golang.org/doc/articles/image_package.html and the part that says "the
correct way to iterate over an Image m's pixels" if you haven't already done so.

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode95
bmp/writer.go:95: width:         uint32(m.Bounds().Dx()),
You call m.Bounds().Dx and m.Bounds().Dy enough times here that it might be
worth at the top:
d := m.Bounds().Size()
and replace m.Bounds().Dx() with d.X, and similarly for Y.

[chai2010, 2013-09-10 06:34:21]

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

Please take another look.

[chai2010, 2013-09-10 06:34:35]

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go
File bmp/writer.go (right):

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode31
bmp/writer.go:31: func encodePaletted(w io.Writer, m image.Image, pix []uint8,
stride, step int) error {
On 2013/09/10 01:32:11, nigeltao wrote:
> It seems like all you really want from the m argument is its bounds' dx and
dy.
> Pass those (as int) instead of passing an image.Image. Similarly below.

Done.

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode74
bmp/writer.go:74: for x := 0; x < m.Bounds().Dx(); x++ {
On 2013/09/10 01:32:11, nigeltao wrote:
> Calling m.Bounds().Dx() on every iteration of the inner loop can be
inefficient:
> https://codereview.appspot.com/13453043/diff/37001/tiff/reader.go#oldcode194
> 
> You should ideally call m.Bounds() only once (or pass it as an argument).

Done.

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode75
bmp/writer.go:75: r, g, b, _ := m.At(x, y).RGBA()
On 2013/09/10 01:32:11, nigeltao wrote:
> At(x, y) isn't right if the image's bounds do not start at (0, 0). Please read
> http://golang.org/doc/articles/image_package.html and the part that says "the
> correct way to iterate over an Image m's pixels" if you haven't already done
so.

Done.

https://codereview.appspot.com/13407045/diff/31001/bmp/writer.go#newcode95
bmp/writer.go:95: width:         uint32(m.Bounds().Dx()),
On 2013/09/10 01:32:11, nigeltao wrote:
> You call m.Bounds().Dx and m.Bounds().Dy enough times here that it might be
> worth at the top:
> d := m.Bounds().Size()
> and replace m.Bounds().Dx() with d.X, and similarly for Y.

Done.

[nigeltao, 2013-09-10 07:18:13]

LGTM.

[nigeltao, 2013-09-10 07:19:45]

*** Submitted as
https://code.google.com/p/go/source/detail?r=cd62a2ebd552&repo=image ***

go.image/bmp: add Encode

R=nigeltao
CC=golang-dev
https://codereview.appspot.com/13407045

Committer: Nigel Tao <nigeltao@golang.org>