Bug 50905 – [chromium] Simplify the PNG encoder.

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[noel gordon]

Simplify the PNG encoder.

[noel gordon]

Created attachment 76358 [details]
patch

[Eric Seidel (no email)]

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?

[noel gordon]

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

[noel gordon]

Created attachment 76832 [details]
patch update changelog description

[David Levin]

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

[noel gordon]

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

[David Levin]

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

[WebKit Commit Bot]

Comment on attachment 76832 [details]
patch update changelog description

Clearing flags on attachment: 76832

Committed r74320: <http://trac.webkit.org/changeset/74320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 76832 [details]:

fast/preloader/script.html bug 50879 (author: [e-mail])
fast/images/load-img-with-empty-src.html bug 50855 (author: [e-mail])
The commit-queue is continuing to process your patch.

[David Levin]

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