New issue
Advanced search Search tips

Issue 770795 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 771075



Sign in to add a comment

Skip checksums while decoding PNGs?

Project Member Reported by scroggo@chromium.org, Oct 2 2017

Issue description

Originally brought up in [1] (with more details in following comments). 

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=762564#c12

We could tell libpng to not compute checksums with the following line:

  png_set_crc_action(png_, PNG_CRC_QUIET_USE, PNG_CRC_QUIET_USE);

We already do this for fdAT (APNG) chunks (since we've modified them to treat them like IDAT chunks). Doing it always would mean we can skip doing some calculations.

Upgrading past version 1.6.26 will make the above line skip even more work, due to [2]. (Which means that we may see a benefit to decoding APNGs, where we already use that line, but I don't think we currently have any benchmarks measuring this.)

[2] https://github.com/glennrp/libpng/commit/89ea0814333c1e5bbbc33cf1dd0df7762aaa75e1

From [3]:
"""
By skipping adler32, you effectively increase the Pud (probability of undetected error) by some factor that will depend on the quality of the sub-network(s) and transport network(s) (802.x, ether, ATM AAL5, fiber, satellite, etc) connecting the chrome user to the server (see the older study in [1]).

[1] http://conferences.sigcomm.org/sigcomm/2000/conf/paper/sigcomm2000-9-1.pdf

PNG was being designed around that time, and the paper recommendations the use of application-level checksums.  On the other hand, what checksums are used in JPEG, or WebP?  What protects them?
"""
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=762564#c32
 
Blockedon: 771075
Just created companion issue for upgrading libpng to enable this experiment.
Just upgraded, so this is unblocked. I think to skip all the checks we'll need to add the following (i.e. in addition to calling png_set_crc_action), from pngtest.c

#ifdef PNG_IGNORE_ADLER32
      /* Turn off ADLER32 checking while reading */
      png_set_option(read_ptr, PNG_IGNORE_ADLER32, PNG_OPTION_ON);
#endif
Owner: cavalcantii@chromium.org
Status: Assigned (was: Untriaged)

Comment 5 by noel@chromium.org, Feb 15 2018

Anywhere in the PNGImageDecoder/Reader where we create the png object ...

     info_ = png_create_info_struct(png_);

one can add the line Leon suggested in #3 to disable crc32 and adler32 checking during PNG decoding, and measure with image_decode_bench with the PNG 140 corpus.

I tried this now all our improvs for zlib are submitted for Intel devices, and the result is: _no perf win at all_.  I conjecture that we have either 1) made adler32 and crc32 in zlib so fast that they are no longer an important bottleneck, or 2) that their presence has a side-effect on modern Intel CPU: they help bring in, or maintain, the image data we need to decompress in the CPU L1/L2/L3 caches.

<EOM>
Status: WontFix (was: Assigned)
Quite interesting results. Makes me wonder to *where* the bottleneck has moved (i.e. inflate_fast?) and if it is even possible to squeeze further performance from it.

Anyway, closing the issue as WONTFIX.

Sign in to add a comment