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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from noel gordon <noel.gordon at gmail.com>  2010-12-18 15:41:15 PST ---
(In reply to comment #5)
> (From update of attachment 76832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76832&action=review
> 
> Looks good. It could possibly be checked in as is but I have a concern about passing 0 to png_create_info_struct.
> 
> A few things to consider or respond to and then I'd be happy to r+ this.

Thanks for the good questions David.


> > WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:69
> > +    png_struct* png = png_create_write_struct(PNG_LIBPNG_VER_STRING, 0, 0, 0);
> 
> Why not just do 
> 
> if (!png)
>   return false;
> 
> as before?
> 
> This way NULL will never get passed to png_create_info_struct and it simplifies the call to png_destroy_write_struct (by getting rid of the check for png).

libpng is a robust library -- all its API entry points vet parameters.  If you pass
NULL to png_create_info_struct, it returns NULL.


> > 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?


> Also why not move Vector<unsigned char> row; to here? (You could even pass this size to the constructor.)

Plausible, but I don't recommend it, see bug 50439 for example.

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