Bug 50905

Created attachment 76358 [details] patch

Comment on attachment 76358 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=76358&action=review Otherwise as far as I can tell this looks fine. > WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:78 > + png_set_IHDR(png, info, imageSize.width(), imageSize.height(), > + 8, PNG_COLOR_TYPE_RGB_ALPHA, 0, 0, 0); Why is this change OK?

(In reply to comment #2) > > WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:78 > > + png_set_IHDR(png, info, imageSize.width(), imageSize.height(), > > + 8, PNG_COLOR_TYPE_RGB_ALPHA, 0, 0, 0); > > Why is this change OK? The 0, 0, 0 I assume? That selects the default (aka basic) encoder. All libpngs must support the basic encoder, the (0) values won't ever change.

Created attachment 76832 [details] patch update changelog description

Comment on attachment 76832 [details] patch update changelog description 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. > 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). > 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)? Also why not move Vector<unsigned char> row; to here? (You could even pass this size to the constructor.)

(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.

(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+.

Comment on attachment 76832 [details] patch update changelog description Clearing flags on attachment: 76832 Committed r74320: <http://trac.webkit.org/changeset/74320>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 76832 [details]: fast/preloader/script.html bug 50879 (author: abarth@webkit.org) fast/images/load-img-with-empty-src.html bug 50855 (author: mitz@webkit.org) The commit-queue is continuing to process your patch.

(In reply to comment #7) >... > The previous code had this: > int outputColorComponents = 4; > int pngOutputColorType = PNG_COLOR_TYPE_RGB_ALPHA; >.... Noel and I discussed this outside of the bug and the code seems fine as landed.