Commit b43ad19e authored by msarett's avatar msarett Committed by Commit bot

Fix performance regression in png encoding caused by libpng update

libpng calculates "filter heuristics" to choose which png
filter to apply.  Chrome need not calculate these "filter
heuristics" because we always choose to use the SUB filter,
for speed.

libpng 1.6 refactors the SUB filter to share code, also
forcing us to calculate the filter heuristics, even though
we don't need to.  This caused a performance regression.

This CL cherry picks a fix from upstream that:
(1) Passes PNG_SIZE_MAX to the filter code.  This allows
the compiler to optimize out the heuristic calculation
code, fixing the performance regression.
(2) Fixes overflow handling in the calculation of filter
heuristics.  This won't affect Chrome.

BUG=619850
BUG=599917

Review-Url: https://codereview.chromium.org/2062423002
Cr-Commit-Position: refs/heads/master@{#401298}
parent 5eeb7648
...@@ -34,6 +34,8 @@ ...@@ -34,6 +34,8 @@
#include "wtf/PtrUtil.h" #include "wtf/PtrUtil.h"
#include <memory> #include <memory>
#include "zlib.h"
namespace blink { namespace blink {
PNGImageEncoderState::~PNGImageEncoderState() PNGImageEncoderState::~PNGImageEncoderState()
...@@ -60,14 +62,19 @@ std::unique_ptr<PNGImageEncoderState> PNGImageEncoderState::create(const IntSize ...@@ -60,14 +62,19 @@ std::unique_ptr<PNGImageEncoderState> PNGImageEncoderState::create(const IntSize
// Optimize compression for speed. // Optimize compression for speed.
// The parameters are the same as what libpng uses by default for RGB and RGBA images, except: // The parameters are the same as what libpng uses by default for RGB and RGBA images, except:
// - the zlib compression level is 3 instead of 6, to avoid the lazy Ziv-Lempel match searching; // The zlib compression level is set to 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. png_set_compression_level(png, 3);
// The zlib memory level (8) and strategy (Z_FILTERED) will be set inside libpng.
// // The zlib memory level is set to 8. This actually matches the default, we are just future-proofing.
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. // Avoid the zlib strategies Z_HUFFMAN_ONLY or Z_RLE.
// Although they are the fastest for poorly-compressible images (e.g. photographs), // 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) // 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 PNG_FILTER_SUB instead of PNG_ALL_FILTERS, to reduce the filter computations.
png_set_filter(png, PNG_FILTER_TYPE_BASE, PNG_FILTER_SUB); png_set_filter(png, PNG_FILTER_TYPE_BASE, PNG_FILTER_SUB);
png_set_write_fn(png, output, writeOutput, 0); png_set_write_fn(png, output, writeOutput, 0);
......
...@@ -24,7 +24,6 @@ config("libpng_config") { ...@@ -24,7 +24,6 @@ config("libpng_config") {
# appear after this, and turn it back on). # appear after this, and turn it back on).
config("clang_warnings") { config("clang_warnings") {
if (is_clang) { if (is_clang) {
# Upstream uses self-assignment to avoid warnings.
cflags = [ cflags = [
# libpng checks that the width is not greater than PNG_SIZE_MAX. # libpng checks that the width is not greater than PNG_SIZE_MAX.
# On platforms where size_t is 64-bits, this comparison will always # On platforms where size_t is 64-bits, this comparison will always
...@@ -61,6 +60,7 @@ source_set("libpng_sources") { ...@@ -61,6 +60,7 @@ source_set("libpng_sources") {
] ]
defines = [] defines = []
cflags = []
if (current_cpu == "x86" || current_cpu == "x64") { if (current_cpu == "x86" || current_cpu == "x64") {
sources += [ sources += [
...@@ -82,6 +82,11 @@ source_set("libpng_sources") { ...@@ -82,6 +82,11 @@ source_set("libpng_sources") {
configs -= [ "//build/config/compiler:chromium_code" ] configs -= [ "//build/config/compiler:chromium_code" ]
configs += [ "//build/config/compiler:no_chromium_code" ] configs += [ "//build/config/compiler:no_chromium_code" ]
if (is_win) {
# Unary minus applied to unsigned type.
cflags += [ "/wd4146" ]
}
if (is_win && is_component_build) { if (is_win && is_component_build) {
defines += [ "PNG_BUILD_DLL" ] defines += [ "PNG_BUILD_DLL" ]
} }
......
...@@ -21,3 +21,6 @@ Updated to 1.6.22, stripped all unneeded files. ...@@ -21,3 +21,6 @@ Updated to 1.6.22, stripped all unneeded files.
- Fix for handling empty first IDAT chunk from upstream: - Fix for handling empty first IDAT chunk from upstream:
https://github.com/glennrp/libpng/commit/3f46c67c6989f480bd932428aa1705f6625dbabf https://github.com/glennrp/libpng/commit/3f46c67c6989f480bd932428aa1705f6625dbabf
https://github.com/glennrp/libpng/commit/81f0273d54aa9de663253b197b3c8228d659cc36 https://github.com/glennrp/libpng/commit/81f0273d54aa9de663253b197b3c8228d659cc36
- Fix performance regression in png encoding (and overflow handling in filter heuristic)
from upstream:
https://github.com/glennrp/libpng/commit/9c04f57cabbf736e91b831858d0eeecca703eabf
...@@ -47,8 +47,10 @@ ...@@ -47,8 +47,10 @@
'export_dependent_settings': [ 'export_dependent_settings': [
'../zlib/zlib.gyp:zlib', '../zlib/zlib.gyp:zlib',
], ],
# TODO(jschuh): http://crbug.com/167187 'msvs_disabled_warnings': [
'msvs_disabled_warnings': [ 4267 ], 4267, # TODO(jschuh): http://crbug.com/167187
4146, # Unary minus applied to unsigned type.
],
'conditions': [ 'conditions': [
# Disable ARM optimizations on IOS. Can't find a way to get gyp to even try # Disable ARM optimizations on IOS. Can't find a way to get gyp to even try
# to compile the optimization files. This works fine on GN. # to compile the optimization files. This works fine on GN.
......
...@@ -2397,7 +2397,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info) ...@@ -2397,7 +2397,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info)
#ifndef PNG_WRITE_FILTER_SUPPORTED #ifndef PNG_WRITE_FILTER_SUPPORTED
png_write_filtered_row(png_ptr, png_ptr->row_buf, row_info->rowbytes+1); png_write_filtered_row(png_ptr, png_ptr->row_buf, row_info->rowbytes+1);
#else #else
png_byte filter_to_do = png_ptr->do_filter; unsigned int filter_to_do = png_ptr->do_filter;
png_bytep row_buf; png_bytep row_buf;
png_bytep best_row; png_bytep best_row;
png_uint_32 bpp; png_uint_32 bpp;
...@@ -2443,27 +2443,24 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info) ...@@ -2443,27 +2443,24 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info)
*/ */
best_row = png_ptr->row_buf; best_row = png_ptr->row_buf;
if (PNG_SIZE_MAX/128 <= row_bytes)
if ((filter_to_do & PNG_FILTER_NONE) != 0 && filter_to_do != PNG_FILTER_NONE) {
/* Overflow can occur in the calculation, just select the lowest set
* filter.
*/
filter_to_do &= -filter_to_do;
}
else if ((filter_to_do & PNG_FILTER_NONE) != 0 &&
filter_to_do != PNG_FILTER_NONE)
{ {
/* Overflow not possible and multiple filters in the list, including the
* 'none' filter.
*/
png_bytep rp; png_bytep rp;
png_size_t sum = 0; png_size_t sum = 0;
png_size_t i; png_size_t i;
int v; int v;
if (PNG_SIZE_MAX/128 <= row_bytes)
{
for (i = 0, rp = row_buf + 1; i < row_bytes; i++, rp++)
{
/* Check for overflow */
if (sum > PNG_SIZE_MAX/128 - 256)
break;
v = *rp;
sum += (v < 128) ? v : 256 - v;
}
}
else /* Overflow is not possible */
{ {
for (i = 0, rp = row_buf + 1; i < row_bytes; i++, rp++) for (i = 0, rp = row_buf + 1; i < row_bytes; i++, rp++)
{ {
...@@ -2479,7 +2476,10 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info) ...@@ -2479,7 +2476,10 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info)
if (filter_to_do == PNG_FILTER_SUB) if (filter_to_do == PNG_FILTER_SUB)
/* It's the only filter so no testing is needed */ /* It's the only filter so no testing is needed */
{ {
(void) png_setup_sub_row(png_ptr, bpp, row_bytes, mins); /* Passing PNG_SIZE_MAX here and below prevents the 'setup' function
* breaking out of the loop when lmins is exceeded.
*/
(void) png_setup_sub_row(png_ptr, bpp, row_bytes, PNG_SIZE_MAX);
best_row = png_ptr->try_row; best_row = png_ptr->try_row;
} }
...@@ -2505,7 +2505,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info) ...@@ -2505,7 +2505,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info)
/* Up filter */ /* Up filter */
if (filter_to_do == PNG_FILTER_UP) if (filter_to_do == PNG_FILTER_UP)
{ {
(void) png_setup_up_row(png_ptr, row_bytes, mins); (void) png_setup_up_row(png_ptr, row_bytes, PNG_SIZE_MAX);
best_row = png_ptr->try_row; best_row = png_ptr->try_row;
} }
...@@ -2531,7 +2531,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info) ...@@ -2531,7 +2531,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info)
/* Avg filter */ /* Avg filter */
if (filter_to_do == PNG_FILTER_AVG) if (filter_to_do == PNG_FILTER_AVG)
{ {
(void) png_setup_avg_row(png_ptr, bpp, row_bytes, mins); (void) png_setup_avg_row(png_ptr, bpp, row_bytes, PNG_SIZE_MAX);
best_row = png_ptr->try_row; best_row = png_ptr->try_row;
} }
...@@ -2557,7 +2557,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info) ...@@ -2557,7 +2557,7 @@ png_write_find_filter(png_structrp png_ptr, png_row_infop row_info)
/* Paeth filter */ /* Paeth filter */
if ((filter_to_do == PNG_FILTER_PAETH) != 0) if ((filter_to_do == PNG_FILTER_PAETH) != 0)
{ {
(void) png_setup_paeth_row(png_ptr, bpp, row_bytes, mins); (void) png_setup_paeth_row(png_ptr, bpp, row_bytes, PNG_SIZE_MAX);
best_row = png_ptr->try_row; best_row = png_ptr->try_row;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment