Fix performance regression in png encoding caused by libpng update

third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp

 /*
  * Copyright (c) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 15 matching lines @@
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "platform/image-encoders/PNGImageEncoder.h"
 
 #include "platform/graphics/ImageBuffer.h"
 #include "wtf/OwnPtr.h"
 
+#include "zlib.h"
+
 namespace blink {
 
 PNGImageEncoderState::~PNGImageEncoderState()
 {
     png_destroy_write_struct(&m_png, &m_info);
 }
 
 static void writeOutput(png_structp png, png_bytep data, png_size_t size)
 {
     static_cast<Vector<unsigned char>*>(png_get_io_ptr(png))->append(data, size);
 }
 
 PassOwnPtr<PNGImageEncoderState> PNGImageEncoderState::create(const IntSize& imageSize, Vector<unsigned char>* output)
 {
     if (imageSize.width() <= 0 || imageSize.height() <= 0)
         return nullptr;
 
     png_struct* png = png_create_write_struct(PNG_LIBPNG_VER_STRING, 0, 0, 0);
     png_info* info = png_create_info_struct(png);
     if (!png || !info || setjmp(png_jmpbuf(png))) {
         png_destroy_write_struct(png ? &png : 0, info ? &info : 0);
         return nullptr;
     }
 
     // Optimize compression for speed.
-    // The parameters are the same as what libpng uses by default for RGB and RGBA images, except:

[scroggo_chromium, 2016/06/21 17:39:43]

I think we've lost some information here. Why did you delete this line?

[msarett, 2016/06/21 19:06:34]

The first time I read it, I thought it was referring to the options below.  Now
that I look again it can be interpreted as referring to any other png encoding
options that aren't set here.  I've added it back in.

-    // - the zlib compression level is 3 instead of 6, to avoid the lazy Ziv-Lempel match searching;
-    // - the delta filter is 1 ("sub") instead of 5 ("all"), to reduce the filter computations.
-    // The zlib memory level (8) and strategy (Z_FILTERED) will be set inside libpng.
-    //
+    // The zlib compression level is set to 3 instead of the default, to avoid the lazy Ziv-Lempel match searching.

[scroggo_chromium, 2016/06/21 17:39:44]

Why not say "instead of 6", like we did before? With the line you deleted, it
makes it clear that 6 is the default.

[msarett, 2016/06/21 19:06:33]

Done.

+    png_set_compression_level(png, 3);
+
+    // The zlib memory level is set to 8, which matches the default.

[scroggo_chromium, 2016/06/21 17:39:44]

Why do we need to set if it is the default? Do we have to set everything if we
set some?

[msarett, 2016/06/21 19:06:33]

We now set everything that the past code comments deemed to be important.

An old comment says:

"// The zlib memory level (8) and strategy (Z_FILTERED) will be set inside
libpng."

Z_FILTERED is no longer the default in libpng 1.6 (so the update caused a
behavior change).  I think it makes sense to set everything we care about, so
future updates don't change behavior we were specifically expecting.

[scroggo_chromium, 2016/06/21 19:26:15]

+1. Comments can get out of date more easily than code, so this sounds good.

+    png_set_compression_mem_level(png, 8);
+
+    // The zlib strategy is set to Z_FILTERED, which does not match the default.
     // Avoid the zlib strategies Z_HUFFMAN_ONLY or Z_RLE.
     // Although they are the fastest for poorly-compressible images (e.g. photographs),
     // they are very slow for highly-compressible images (e.g. text, drawings or business graphics)
-    png_set_compression_level(png, 3);
+    png_set_compression_strategy(png, Z_FILTERED);
+
+    // The delta filter is 1 ("sub") instead of 5 ("all"), to reduce the filter computations.

[scroggo_chromium, 2016/06/21 17:39:44]

I think this comment was out of date before you got here. We no longer pass
numbers, and the names don't quite match the macro names.

I think this is clearer as

    // The delta filter is set to PNG_FILTER_SUB instead of PNG_ALL_FILTERS to
    // reduce the filter computations.

[msarett, 2016/06/21 19:06:33]

You're right.  Done.

     png_set_filter(png, PNG_FILTER_TYPE_BASE, PNG_FILTER_SUB);
 
     png_set_write_fn(png, output, writeOutput, 0);
     png_set_IHDR(png, info, imageSize.width(), imageSize.height(), 8, PNG_COLOR_TYPE_RGB_ALPHA, 0, 0, 0);
     png_write_info(png, info);
 
     return adoptPtr(new PNGImageEncoderState(png, info));
 }
 
 void PNGImageEncoder::writeOneRowToPng(unsigned char* pixels, PNGImageEncoderState* encoderState)
@@ skipping 25 matching lines @@
 
 bool PNGImageEncoder::encode(const ImageDataBuffer& imageData, Vector<unsigned char>* output)
 {
     if (!imageData.pixels())
         return false;
 
     return encodePixels(imageData.size(), imageData.pixels(), output);
 }
 
 } // namespace blink