Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(92)

Issue 24449003: Make Jpeg decoding more fault resistant. (Closed)

Created:
7 years, 2 months ago by hal.canary
Modified:
7 years, 2 months ago
Reviewers:
scroggo
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Make image decoding more fault resistant, less verbose. This change address what happens when a jpeg is partially downloaded before failing. Many browsers will render it anyway: we want Skia to do the same. The JpegTest takes a perfectly cromulent jpeg file and only passes into the ImageDecoder the first half of the image. We then verify that the image decoder returns a valid bitmap of the correct dimensions. We also fixed some png library errors, including issue 1691. Also, suppressed the majority of warnings from using libpng and libjpeg. By default, most warnings are *not* suppressed in debug mode. If you have a debug binary and wish to suppress warnings, set the following environment variables to true skia_images_png_suppressDecoderWarnings skia_images_jpeg_suppressDecoderWarnings or from within a program that links to Skia: #if defined(SK_DEBUG) #include "SkRTConf.h" SK_CONF_SET("images.jpeg.suppressDecoderWarnings", true); SK_CONF_SET("images.png.suppressDecoderWarnings", true); #endif I tested this, before (control) and after these changes (test), on 364,295 skps from the cluster telemetry. - number of errors+warnings in control = 2804 - number of errors+warnings fixed = 2283 - number of PNG verbosity fixed = 2152 - number of PNG error fixed = 4 - number of PNG segfault fixed = 3 - number of PNG errors changed to warnings = 62 - number of JPG verbosity fixed = 26 - number of JPG error fixed = 91 Not all errors and warning have been fixed. These numbers were generated using the find_bad_images_in_skps.py program. This program may be useful going forward for testing image-decoding libraries on skp files from the cluster telemetry. find_bad_images_in_skps.py depends on the test_image_decoder program, which simply executes the SkImageDecoder::DecodeFile function and uses its exit status to report success or failure. BUG=skia:1649 BUG=skia:1691 BUG=skia:1680 R=scroggo@google.com Committed: https://code.google.com/p/skia/source/detail?r=11597

Patch Set 1 #

Total comments: 8

Patch Set 2 : Make Jpeg decoding more fault resistant #

Patch Set 3 : cleanup gyp #

Patch Set 4 : suppress warnings; better testing #

Patch Set 5 : lint changes, bugfix, description changed #

Total comments: 17

Patch Set 6 : Use SkRTConf.h, style issues. #

Patch Set 7 : description change #

Total comments: 5

Patch Set 8 : JPEG decoder no longer leaves memory uninitialized. #

Patch Set 9 : remove unreachable code, add to tests, tested on Android. #

Patch Set 10 : final rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+798 lines, -13 lines) Patch
M gyp/tests.gyp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gyp/tools.gyp View 1 2 2 chunks +11 lines, -0 lines 0 comments Download
M src/images/SkImageDecoder_libjpeg.cpp View 1 2 3 4 5 6 7 8 8 chunks +54 lines, -6 lines 0 comments Download
M src/images/SkImageDecoder_libpng.cpp View 1 2 3 4 5 6 7 8 6 chunks +47 lines, -7 lines 0 comments Download
A tests/JpegTest.cpp View 1 2 3 4 5 6 7 8 1 chunk +456 lines, -0 lines 0 comments Download
A tools/find_bad_images_in_skps.py View 1 2 3 4 5 1 chunk +197 lines, -0 lines 0 comments Download
A tools/test_image_decoder.cpp View 1 2 3 4 5 1 chunk +32 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
hal.canary
7 years, 2 months ago (2013-09-24 22:40:23 UTC) #1
scroggo
https://codereview.chromium.org/24449003/diff/1/src/images/SkImageDecoder_libjpeg.cpp File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/24449003/diff/1/src/images/SkImageDecoder_libjpeg.cpp#newcode289 src/images/SkImageDecoder_libjpeg.cpp:289: #if defined(SK_DEBUG) && defined(SK_DEBUG_JPEG_ERRORS) Like I said in the ...
7 years, 2 months ago (2013-09-24 23:01:07 UTC) #2
scroggo
https://codereview.chromium.org/24449003/diff/1/tests/JpegTest.cpp File tests/JpegTest.cpp (right): https://codereview.chromium.org/24449003/diff/1/tests/JpegTest.cpp#newcode9 tests/JpegTest.cpp:9: I mentioned this in person, but this file should ...
7 years, 2 months ago (2013-09-25 20:45:59 UTC) #3
hal.canary
please take a look!
7 years, 2 months ago (2013-10-01 20:49:27 UTC) #4
scroggo
https://codereview.chromium.org/24449003/diff/22001/src/images/SkImageDecoder_libjpeg.cpp File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/24449003/diff/22001/src/images/SkImageDecoder_libjpeg.cpp#newcode57 src/images/SkImageDecoder_libjpeg.cpp:57: static void do_nothing(jpeg_common_struct*, int) { /* do nothing */ ...
7 years, 2 months ago (2013-10-01 22:04:13 UTC) #5
hal.canary
https://codereview.chromium.org/24449003/diff/22001/tools/find_bad_images_in_skps.py File tools/find_bad_images_in_skps.py (right): https://codereview.chromium.org/24449003/diff/22001/tools/find_bad_images_in_skps.py#newcode138 tools/find_bad_images_in_skps.py:138: sys.stderr.write('{0} is a repeat.\n'.format(image_name)) On 2013/10/01 22:04:14, scroggo wrote: ...
7 years, 2 months ago (2013-10-02 16:20:05 UTC) #6
scroggo
Everything else looks good, but the jpeg decoder still leaves memory uninitialized. https://codereview.chromium.org/24449003/diff/35001/src/images/SkImageDecoder_libjpeg.cpp File src/images/SkImageDecoder_libjpeg.cpp ...
7 years, 2 months ago (2013-10-02 17:05:04 UTC) #7
hal.canary
I think we're close. Take a look!
7 years, 2 months ago (2013-10-03 19:29:22 UTC) #8
scroggo
lgtm
7 years, 2 months ago (2013-10-03 20:19:21 UTC) #9
hal.canary
7 years, 2 months ago (2013-10-04 12:46:49 UTC) #10
Message was sent while issue was closed.
Committed patchset #10 manually as r11597 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698