Fix performance regression in png encoding caused by libpng update

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

Index: third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp b/third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp
index cfaae3f159f23f773b9a2b2a62cf7b69dbd47bc1..5f94e2f16d305399f6b978f32de40bc5825f0f5d 100644
--- a/third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp
+++ b/third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp
@@ -33,6 +33,8 @@
 #include "platform/graphics/ImageBuffer.h"
 #include "wtf/OwnPtr.h"
 
+#include "zlib.h"
+
 namespace blink {
 
 PNGImageEncoderState::~PNGImageEncoderState()
@@ -58,15 +60,19 @@ PassOwnPtr<PNGImageEncoderState> PNGImageEncoderState::create(const IntSize& ima
     }
 
     // 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);