code review 4174053: image/png: support for more formats

image/png: support for more formats

Added support for reading images with grayscale + alpha and
for images with depth 1, 2 or 4.

[mti, 2011-02-17 15:10:08]

Hello golang-dev@googlegroups.com (cc: mti@tillenius.com, nigeltao_gnome),

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

[rsc1, 2011-02-17 16:03:24]

R=nigeltao

[nigeltao, 2011-02-18 00:25:09]

Code looks good. I just have a couple of questions about the test data.

http://codereview.appspot.com/4174053/diff/7002/src/pkg/image/png/testdata/pn...
File src/pkg/image/png/testdata/pngsuite/README (right):

http://codereview.appspot.com/4174053/diff/7002/src/pkg/image/png/testdata/pn...
src/pkg/image/png/testdata/pngsuite/README:2: libpng.org, specifically
contrib/pngsuite/* in libpng-1.2.40.tar.gz.
The added .png files (such as basn0g01-30.png) are not in the tarball from
libpng.org. Nor are they in the original set from http:/www.schaik.com/pngsuite/

Is basn0g01-30.png dramatically different than basn0g01.png? If not, I wouldn't
bother adding them.

If you're keeping them, can you clarify where your new .png files come from, and
confirm that they are licensed appropriately?

http://codereview.appspot.com/4174053/diff/7002/src/pkg/image/png/testdata/pn...
src/pkg/image/png/testdata/pngsuite/README:10: generated from PNGs with a
bitdepth of 8.
Can you add the command line that you used to generate the .sng files in this
case?

Also, you seem to have modified other .sng files (such as basn4a08.sng) but
that's not part of basn0g0{1,2,4}.sng.

[mti, 2011-02-18 19:34:39]

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

Please take another look.

[mti, 2011-02-18 19:35:46]

I have added some more explanation about where the testfiles come
from in the README. Basically they are all simple conversions of
the orignal testfiles from libpng.

The -29, -30 and -31 files have said width (and height) which is 
useful for testing that rounding upwards from bits to whole bytes
work correctly. (Width 32 seem like a really bad choice!)

Since we loose information about the orignal depth and color model
in some cases when reading the *.png files we cannot generate *.sng 
files identical to the *.png:s. In these cases I have changed the 
*.sng files to a larger depth or to rgba instead of gray + alpha.

[nigeltao, 2011-02-21 06:51:56]

http://codereview.appspot.com/4174053/diff/5003/src/pkg/image/png/reader.go
File src/pkg/image/png/reader.go (right):

http://codereview.appspot.com/4174053/diff/5003/src/pkg/image/png/reader.go#n...
src/pkg/image/png/reader.go:330: bpp := (bitpp + 7) / 8 // Bytes per pixel.
I'd call the variables bytesPerPixel and bitsPerPixel, and do away with the
comment.

I'd also remove the empty line above at line 329.

http://codereview.appspot.com/4174053/diff/5003/src/pkg/image/png/reader_test.go
File src/pkg/image/png/reader_test.go (right):

http://codereview.appspot.com/4174053/diff/5003/src/pkg/image/png/reader_test...
src/pkg/image/png/reader_test.go:170: // so we just check a single random pixel
Add a full stop to finish the sentence.

http://codereview.appspot.com/4174053/diff/5003/src/pkg/image/png/testdata/pn...
File src/pkg/image/png/testdata/pngsuite/README (right):

http://codereview.appspot.com/4174053/diff/5003/src/pkg/image/png/testdata/pn...
src/pkg/image/png/testdata/pngsuite/README:10: not part of libpng but were
created from files in libpng. Their non-power-of-
s/libpng/pngsuite/g

[mti, 2011-02-21 07:43:04]

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

Please take another look.

[nigeltao, 2011-02-21 23:04:38]

LGTM.

[nigeltao, 2011-02-21 23:04:50]

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

image/png: support for more formats

Added support for reading images with grayscale + alpha and
for images with depth 1, 2 or 4.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/4174053

Committer: Nigel Tao <nigeltao@golang.org>