Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(69)

Issue 6822065: code review 6822065: image/png: degrade gracefully for palette index values ... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 2 months ago by nigeltao
Modified:
9 years, 2 months ago
Reviewers:
glennrp
CC:
r, golang-dev
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r e489928c34ef https://code.google.com/p/go #

Patch Set 3 : diff -r e489928c34ef https://code.google.com/p/go #

Total comments: 4

Patch Set 4 : diff -r e489928c34ef https://code.google.com/p/go #

Total comments: 2

Patch Set 5 : diff -r a9da8308dcab https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -14 lines) Patch
M src/pkg/image/png/reader.go View 1 2 3 7 chunks +20 lines, -14 lines 0 comments Download

Messages

Total messages: 10
nigeltao
Hello r@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
9 years, 2 months ago (2012-11-01 00:30:28 UTC) #1
nigeltao
Oops, s/transparent black/opaque black/.
9 years, 2 months ago (2012-11-01 00:35:14 UTC) #2
r
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 ...
9 years, 2 months ago (2012-11-01 00:36:00 UTC) #3
r
also - do we want to document this behavior in the package docs?
9 years, 2 months ago (2012-11-01 00:36:22 UTC) #4
nigeltao
I don't think we need to document this. AIUI the spec doesn't, uh, specify what ...
9 years, 2 months ago (2012-11-01 00:39:01 UTC) #5
r
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 ...
9 years, 2 months ago (2012-11-01 00:41:17 UTC) #6
nigeltao
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, ...
9 years, 2 months ago (2012-11-01 00:43:18 UTC) #7
nigeltao
*** Submitted as http://code.google.com/p/go/source/detail?r=58206fbe45a9 *** image/png: degrade gracefully for palette index values that aren't defined ...
9 years, 2 months ago (2012-11-01 00:46:17 UTC) #8
glennrp_gmail.com
On Wednesday, October 31, 2012 8:30:31 PM UTC-4, Nigel Tao wrote: > > On my ...
9 years, 2 months ago (2012-11-01 15:20:50 UTC) #9
nigeltao
9 years, 2 months ago (2012-11-02 02:25:05 UTC) #10
On Fri, Nov 2, 2012 at 2:20 AM,  <glennrp@gmail.com> 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b