[Webkit-unassigned] [Bug 50905] [chromium] Simplify the PNG encoder.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50905





--- Comment #7 from David Levin <levin at chromium.org>  2010-12-18 17:08:33 PST ---
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 76832 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=76832&action=review
> > > WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:82
> > > +    row.resize(imageSize.width() * bitmap.bytesPerPixel());
> > 
> > Why use bitmap.bytesPerPixel() when preMultipliedBGRAtoRGBA will always use 4 bytes per pixel (4 unsigned char)?
> 
> Bad anticipation on my part.  I had used 4 originally, but based on the recent jpeg
> encoder reviews, I anticipated a reviewer asking me not to use "magic numbers" :)
> 
> I'd be happy to change it back to 4, if you think that's better, but perhaps in a
> follow-up CL?

I agree about magic numbers not being great. However, in this case, bitmap.bytesPerPixel() seems incorrect because the allocation is for the destination, but and bytesPerPixel() from the source. These two happen to be the same for the code, but that doesn't seem like it needs to be the case. For example, if the code were changed to support bitmaps that were RGB as input, this would underallocate.

The previous code had this:
 int outputColorComponents = 4;
 int pngOutputColorType = PNG_COLOR_TYPE_RGB_ALPHA;
which make sense to me.

Anyway, this is ok for now, so r+.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.