code review 1897049: Grayscale (8 bits only) support for png

Grayscale (8 bits only) support for png

[mpl, 2010-08-04 12:43:27]

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

I'd like you to review this change.

[nigeltao, 2010-08-05 03:08:39]

You're doing two things in your change: you're supporting grayscale images, and
you're supporting 16-bit color. Doing them both in the one changelist makes it
more complicated to understand and review than doing them separately.

I would suggest just supporting 8-bit grayscale in your first change, and if you
actually need to also support 16-bit grayscale, then make a second change that
does both 16-bit grayscale and 16-bit color.

[mpl, 2010-08-05 08:11:05]

I only did the 16 bits case because you introduced it in image, I wanted to be
consistent with that change. I think I was carefull enough to only allow 16 bits
when it's grayscale, not when it's color, so I don't see where the problem is.

On 2010/08/05 03:08:39, nigeltao_golang wrote:
> You're doing two things in your change: you're supporting grayscale images,
and
> you're supporting 16-bit color. Doing them both in the one changelist makes it
> more complicated to understand and review than doing them separately.
> 
> I would suggest just supporting 8-bit grayscale in your first change, and if
you
> actually need to also support 16-bit grayscale, then make a second change that
> does both 16-bit grayscale and 16-bit color.

[nigeltao_gnome, 2010-08-06 01:17:27]

On 5 August 2010 18:11,  <mathieu.lonjaret@gmail.com> wrote:
> I only did the 16 bits case because you introduced it in image, I wanted
> to be consistent with that change. I think I was carefull enough to only
> allow 16 bits when it's grayscale, not when it's color, so I don't see
> where the problem is.

The problem is that it makes the code change more complicated, and
unnecessarily harder to review. If you want to support Gray16, then do
it in the two stages I suggested.

There already were 16-bit color image.RGBA64 and image.NRGBA64 types,
and the png package does not support that yet. The fact that I added
the image.Gray16 type for completeness doesn't demand that png has to
support it when it supports image.Gray.

[mpl, 2010-08-06 08:41:29]

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

Please take another look.

[nigeltao, 2010-08-09 01:34:58]

http://codereview.appspot.com/1897049/diff/4/4004
File src/pkg/image/png/reader.go (right):

http://codereview.appspot.com/1897049/diff/4/4004#newcode289
src/pkg/image/png/reader.go:289: gray.Set(x, y, image.GrayColor{cdat[1*x+0]})
Drop the "1*" and "+0".

http://codereview.appspot.com/1897049/diff/4/4006
File src/pkg/image/png/writer.go (right):

http://codereview.appspot.com/1897049/diff/4/4006#newcode272
src/pkg/image/png/writer.go:272: cr[0][1*x+1] = uint8(gray >> 8)
Drop the "1*".

http://codereview.appspot.com/1897049/diff/4/4006#newcode345
src/pkg/image/png/writer.go:345: e.colorType = uint8(ctTrueColorAlpha)
I think you can drop the uint8. Yes, it's an oversight from a previous change,
not an error introduced in this change. But while you're here, you can fix it
up.

http://codereview.appspot.com/1897049/diff/4/4006#newcode350
src/pkg/image/png/writer.go:350: switch m.ColorModel() {
Your switch statement needs a default case. I would rewrite lines 347-356 as:

if pal != nil {
  e.colorType = ctPaletted
} else {
  switch m.ColorModel() {
  case image.GrayColorModel:
    e.colorType = ctGrayscale
  default:
    if opaque(m) {
      e.colorType = ctTrueColor
    }
  }
}

[mpl, 2010-08-09 09:41:09]

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

Please take another look.

[mpl, 2010-08-09 09:43:44]

On 2010/08/09 01:34:58, nigeltao_golang wrote:
> http://codereview.appspot.com/1897049/diff/4/4004
> File src/pkg/image/png/reader.go (right):
> 
> http://codereview.appspot.com/1897049/diff/4/4004#newcode289
> src/pkg/image/png/reader.go:289: gray.Set(x, y, image.GrayColor{cdat[1*x+0]})
> Drop the "1*" and "+0".

Done.

> http://codereview.appspot.com/1897049/diff/4/4006
> File src/pkg/image/png/writer.go (right):
> 
> http://codereview.appspot.com/1897049/diff/4/4006#newcode272
> src/pkg/image/png/writer.go:272: cr[0][1*x+1] = uint8(gray >> 8)
> Drop the "1*".

Done.
 
> http://codereview.appspot.com/1897049/diff/4/4006#newcode345
> src/pkg/image/png/writer.go:345: e.colorType = uint8(ctTrueColorAlpha)
> I think you can drop the uint8. Yes, it's an oversight from a previous change,
> not an error introduced in this change. But while you're here, you can fix it
> up.

Done.
 
> http://codereview.appspot.com/1897049/diff/4/4006#newcode350
> src/pkg/image/png/writer.go:350: switch m.ColorModel() {
> Your switch statement needs a default case. I would rewrite lines 347-356 as:
> 
> if pal != nil {
>   e.colorType = ctPaletted
> } else {
>   switch m.ColorModel() {
>   case image.GrayColorModel:
>     e.colorType = ctGrayscale
>   default:
>     if opaque(m) {
>       e.colorType = ctTrueColor
>     }
>   }
> }

it does not matter for now since it's always opaque, but don't we want to keep
the ctGrayscale case under un opaque check for later?
done anyways.

[r, 2010-08-10 02:01:24]

http://codereview.appspot.com/1897049/diff/19001/20001
File src/pkg/image/png/reader.go (right):

http://codereview.appspot.com/1897049/diff/19001/20001#newcode180
src/pkg/image/png/reader.go:180: return UnsupportedError("Grayscale
transparency")
s/G/g/

[nigeltao, 2010-08-10 02:25:07]

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

png: grayscale support.

R=nigeltao_golang, r
CC=golang-dev
http://codereview.appspot.com/1897049

Committer: Nigel Tao <nigeltao@golang.org>