FAQ
Reviewers: r,

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

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


Description:
image/png: degrade gracefully for palette index values that aren't
defined by the PLTE chunk. Such pixels decode to transparent black,
which matches what libpng does.

Fixes issue 4319.

On my reading, the PNG spec isn't clear whether palette index values
outside of those defined by the PLTE chunk is an error, and if not,
what to do.


Libpng 1.5.3 falls back to transparent black. png_set_PLTE says:

/* Changed in libpng-1.2.1 to allocate PNG_MAX_PALETTE_LENGTH instead
* of num_palette entries, in case of an invalid PNG file that has
* too-large sample values.
*/
png_ptr->palette = (png_colorp)png_calloc(png_ptr,
PNG_MAX_PALETTE_LENGTH * png_sizeof(png_color));


ImageMagick 6.5.7 returns an error:

$ convert -version
Version: ImageMagick 6.5.7-8 2012-08-17 Q16 http://www.imagemagick.org
Copyright: Copyright (C) 1999-2009 ImageMagick Studio LLC
Features: OpenMP
$ convert packetloss.png x.bmp
convert: Invalid colormap index `packetloss.png' @
image.c/SyncImage/3849.

Please review this at http://codereview.appspot.com/6822065/

Affected files:
M src/pkg/image/png/reader.go


Index: src/pkg/image/png/reader.go
===================================================================
--- a/src/pkg/image/png/reader.go
+++ b/src/pkg/image/png/reader.go
@@ -193,10 +193,18 @@
d.crc.Write(d.tmp[:n])
switch d.cb {
case cbP1, cbP2, cbP4, cbP8:
- d.palette = color.Palette(make([]color.Color, np))
+ d.palette = make(color.Palette, 256)
for i := 0; i < np; i++ {
d.palette[i] = color.RGBA{d.tmp[3*i+0], d.tmp[3*i+1], d.tmp[3*i+2],
0xff}
}
+ for i := np; i < 256; i++ {
+ // Initialize the rest of the palette to transparent black. The spec
isn't
+ // clear whether palette index values outside of those defined by the
PLTE
+ // chunk is an error: libpng 1.5.13 falls back to transparent black, the
+ // same as we do here, ImageMagick 6.5.7 returns an error.
+ d.palette[i] = color.RGBA{}
+ }
+ d.palette = d.palette[:np]
case cbTC8, cbTCA8, cbTC16, cbTCA16:
// As per the PNG spec, a PLTE chunk is optional (and for practical
purposes,
// ignorable) for the ctTrueColor and ctTrueColorAlpha color types
(section 4.1.2).
@@ -221,8 +229,8 @@
case cbTC8, cbTC16:
return UnsupportedError("truecolor transparency")
case cbP1, cbP2, cbP4, cbP8:
- if n > len(d.palette) {
- return FormatError("bad tRNS length")
+ if len(d.palette) < n {
+ d.palette = d.palette[:n]
}
for i := 0; i < n; i++ {
rgba := d.palette[i].(color.RGBA)
@@ -279,7 +287,6 @@
}
defer r.Close()
bitsPerPixel := 0
- maxPalette := uint8(0)
pixOffset := 0
var (
gray *image.Gray
@@ -308,7 +315,6 @@
bitsPerPixel = d.depth
paletted = image.NewPaletted(image.Rect(0, 0, d.width, d.height),
d.palette)
img = paletted
- maxPalette = uint8(len(d.palette) - 1)
case cbTCA8:
bitsPerPixel = 32
nrgba = image.NewNRGBA(image.Rect(0, 0, d.width, d.height))
@@ -421,8 +427,8 @@
b := cdat[x/8]
for x2 := 0; x2 < 8 && x+x2 < d.width; x2++ {
idx := b >> 7
- if idx > maxPalette {
- return nil, FormatError("palette index out of range")
+ if len(paletted.Palette) <= int(idx) {
+ paletted.Palette = paletted.Palette[:int(idx)+1]
}
paletted.SetColorIndex(x+x2, y, idx)
b <<= 1
@@ -433,8 +439,8 @@
b := cdat[x/4]
for x2 := 0; x2 < 4 && x+x2 < d.width; x2++ {
idx := b >> 6
- if idx > maxPalette {
- return nil, FormatError("palette index out of range")
+ if len(paletted.Palette) <= int(idx) {
+ paletted.Palette = paletted.Palette[:int(idx)+1]
}
paletted.SetColorIndex(x+x2, y, idx)
b <<= 2
@@ -445,18 +451,18 @@
b := cdat[x/2]
for x2 := 0; x2 < 2 && x+x2 < d.width; x2++ {
idx := b >> 4
- if idx > maxPalette {
- return nil, FormatError("palette index out of range")
+ if len(paletted.Palette) <= int(idx) {
+ paletted.Palette = paletted.Palette[:int(idx)+1]
}
paletted.SetColorIndex(x+x2, y, idx)
b <<= 4
}
}
case cbP8:
- if maxPalette != 255 {
+ if len(paletted.Palette) != 255 {
for x := 0; x < d.width; x++ {
- if cdat[x] > maxPalette {
- return nil, FormatError("palette index out of range")
+ if len(paletted.Palette) <= int(cdat[x]) {
+ paletted.Palette = paletted.Palette[:int(cdat[x])+1]
}
}
}

Search Discussions

  • Nigel Tao at Nov 1, 2012 at 12:35 am
    Oops, s/transparent black/opaque black/.
  • R at Nov 1, 2012 at 12:36 am
    http://codereview.appspot.com/6822065/diff/5001/src/pkg/image/png/reader.go
    File src/pkg/image/png/reader.go (right):

    http://codereview.appspot.com/6822065/diff/5001/src/pkg/image/png/reader.go#newcode206
    src/pkg/image/png/reader.go:206: }
    isn't this loop a no-op? the slice is already zeroed.

    http://codereview.appspot.com/6822065/diff/5001/src/pkg/image/png/reader.go#newcode430
    src/pkg/image/png/reader.go:430: if len(paletted.Palette) <= int(idx) {
    the code would be more readable if len(paletted.Pallete) was pulled out,
    and also to have the type of idx (uint8)

    http://codereview.appspot.com/6822065/
  • R at Nov 1, 2012 at 12:36 am
    also - do we want to document this behavior in the package docs?

    http://codereview.appspot.com/6822065/
  • Nigeltao at Nov 1, 2012 at 12:39 am
    I don't think we need to document this. AIUI the spec doesn't, uh,
    specify what to do here.


    https://codereview.appspot.com/6822065/diff/5001/src/pkg/image/png/reader.go
    File src/pkg/image/png/reader.go (right):

    https://codereview.appspot.com/6822065/diff/5001/src/pkg/image/png/reader.go#newcode206
    src/pkg/image/png/reader.go:206: }
    On 2012/11/01 00:36:00, r wrote:
    isn't this loop a no-op? the slice is already zeroed.
    It's already zeroed to a nil color.Color (an interface), not a zero
    color.RGBA (a struct).

    https://codereview.appspot.com/6822065/diff/5001/src/pkg/image/png/reader.go#newcode430
    src/pkg/image/png/reader.go:430: if len(paletted.Palette) <= int(idx) {
    On 2012/11/01 00:36:00, r wrote:
    the code would be more readable if len(paletted.Pallete) was pulled out, and
    also to have the type of idx (uint8)
    I don't understand "pulled out". Also, len(paletted.Palette) can be 256,
    which obviously overflows a uint8.

    https://codereview.appspot.com/6822065/
  • R at Nov 1, 2012 at 12:41 am
    LGTM


    https://codereview.appspot.com/6822065/diff/9001/src/pkg/image/png/reader.go
    File src/pkg/image/png/reader.go (right):

    https://codereview.appspot.com/6822065/diff/9001/src/pkg/image/png/reader.go#newcode205
    src/pkg/image/png/reader.go:205: d.palette[i] = color.RGBA{0x00, 0x00,
    0x00, 0xff}
    aha, that's better. that's not what was here before.

    https://codereview.appspot.com/6822065/
  • Nigeltao at Nov 1, 2012 at 12:43 am
    https://codereview.appspot.com/6822065/diff/9001/src/pkg/image/png/reader.go
    File src/pkg/image/png/reader.go (right):

    https://codereview.appspot.com/6822065/diff/9001/src/pkg/image/png/reader.go#newcode205
    src/pkg/image/png/reader.go:205: d.palette[i] = color.RGBA{0x00, 0x00,
    0x00, 0xff}
    On 2012/11/01 00:41:17, r wrote:
    aha, that's better. that's not what was here before.
    Yeah, s/transparent black/opaque black/, since libpng's struct png_color
    is RGB but no A.

    https://codereview.appspot.com/6822065/
  • Nigeltao at Nov 1, 2012 at 12:46 am
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=58206fbe45a9 ***

    image/png: degrade gracefully for palette index values that aren't
    defined by the PLTE chunk. Such pixels decode to opaque black,
    which matches what libpng does.

    Fixes issue 4319.

    On my reading, the PNG spec isn't clear whether palette index values
    outside of those defined by the PLTE chunk is an error, and if not,
    what to do.


    Libpng 1.5.3 falls back to opaque black. png_set_PLTE says:

    /* Changed in libpng-1.2.1 to allocate PNG_MAX_PALETTE_LENGTH instead
    * of num_palette entries, in case of an invalid PNG file that has
    * too-large sample values.
    */
    png_ptr->palette = (png_colorp)png_calloc(png_ptr,
    PNG_MAX_PALETTE_LENGTH * png_sizeof(png_color));


    ImageMagick 6.5.7 returns an error:

    $ convert -version
    Version: ImageMagick 6.5.7-8 2012-08-17 Q16 http://www.imagemagick.org
    Copyright: Copyright (C) 1999-2009 ImageMagick Studio LLC
    Features: OpenMP
    $ convert packetloss.png x.bmp
    convert: Invalid colormap index `packetloss.png' @
    image.c/SyncImage/3849.

    R=r
    CC=golang-dev
    http://codereview.appspot.com/6822065


    http://codereview.appspot.com/6822065/
  • Glennrp at Nov 1, 2012 at 4:15 pm
    On Wednesday, October 31, 2012 8:30:31 PM UTC-4, Nigel Tao wrote:

    On my reading, the PNG spec isn't clear whether palette index values
    outside of those defined by the PLTE chunk is an error, and if not,
    what to do.
    I think it's pretty clear:

    Clause 11.2.3 PLTE palette
    end of 5th paragraph:
    It is permissible to have fewer entries than the bit depth would allow. In
    that case, any out-of-range pixel value found in the image data is an error.

    Clause 13.2 Error handling, second paragraph, item 2:
    Recover from the error, if possible; otherwise fail gracefully.

    Glenn
  • Nigel Tao at Nov 2, 2012 at 2:30 am

    On Fri, Nov 2, 2012 at 2:20 AM, wrote:
    Clause 11.2.3 PLTE palette
    end of 5th paragraph:
    It is permissible to have fewer entries than the bit depth would allow. In
    that case, any out-of-range pixel value found in the image data is an error.
    Thanks for that. I'm not sure how I missed it. I'll fix the reader.go comment.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 1, '12 at 12:30a
activeNov 2, '12 at 2:30a
posts10
users3
websitegolang.org

3 users in discussion

Nigel Tao: 6 posts R: 3 posts Glennrp: 1 post

People

Translate

site design / logo © 2021 Grokbase