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

Issue 24269006: Add an option on SkImageDecoder to skip writing 0s. (Closed)

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

Description

Add an option on SkImageDecoder to skip writing 0s. Only implemented for PNG. Add a getter and setter, and sets the default to false in the constructor. Also copies the setting in copyFieldsToOther. Fix an indpendent bug where fDitherImage was not being copied in copyFieldsToOther. In SkScaledBitmapSampler::begin, consolidate the settings passed in by passing a const reference to the decoder. The decoder can be referenced for its settings of dither, unpremultiplied, and now skipping writing zeroes. Update callers to use the new API. In png decoder, rather than passing around a pointer to an initial read of getDitherImage, and potentially changing it, look at the field on the decoder itself, and modify it directly. This is a change in behavior - now if that same decoder is used to decode a different image, the dither setting has changed. I think this is okay because A) the typical use case is to use a new decoder for each decode, B) we do not make any promises that a decode does not change the decoder and C) it makes the code in SkScaledBitmapSampler much cleaner. In SkScaledBitmapScampler, add new row procs for skipping zeroes. Now that choosing the row proc has five dimensions (src config, dst config, dither, skip writing zeroes, unpremultiplied), use a new method: each src/dst combination has a function for choosing the right proc depending on the decoder. SkScaledBitmapScampler::RowProc is now public for convenience. Remove Sample_Gray_D8888_Unpremul, which is effectively no different from Sample_Gray_D8888. In cases where unpremultiplied was trivial, such as 565 and when sampling from gray, decoding may now succeed. Add a benchmark (currently disabled) for comparing the speed of skipping writing zeroes versus not skipping. For this particular image, which is mostly transparent pixels, normal decoding took about 3.6 milliseconds, while skipping zeroes in the decode took only about 2.5 milliseconds (this is on a Nexus 4). Presumably it would be slower on an image with a small amount of transparency, but there will be no slowdown for an image which reports that it has no transparency. In SkImageRef_ashmem, always skip writing zeroes, since ashmem memory is guaranteed to be initialized to 0. Add a flag to skip writing zeroes in skimage. Add a regression test for choosing the rowproc to ensure I did not change any behavior accidentally. BUG=skia:1661 R=reed@google.com Committed: https://code.google.com/p/skia/source/detail?r=11558

Patch Set 1 #

Total comments: 13

Patch Set 2 : Rebase, plus update the benchmark #

Unified diffs Side-by-side diffs Delta from patch set Stats (+474 lines, -133 lines) Patch
M bench/SkipZeroesBench.cpp View 1 2 chunks +2 lines, -10 lines 0 comments Download
M include/core/SkImageDecoder.h View 1 2 chunks +18 lines, -0 lines 0 comments Download
M src/images/SkImageDecoder.cpp View 1 2 chunks +3 lines, -0 lines 0 comments Download
M src/images/SkImageDecoder_libbmp.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M src/images/SkImageDecoder_libjpeg.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/images/SkImageDecoder_libpng.cpp View 1 8 chunks +7 lines, -12 lines 0 comments Download
M src/images/SkImageRef_ashmem.cpp View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/images/SkScaledBitmapSampler.h View 4 chunks +13 lines, -7 lines 0 comments Download
M src/images/SkScaledBitmapSampler.cpp View 19 chunks +417 lines, -100 lines 0 comments Download
M tests/ImageDecodingTest.cpp View 1 1 chunk +6 lines, -1 line 0 comments Download
M tools/skimage_main.cpp View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
scroggo
https://codereview.chromium.org/24269006/diff/1/src/images/SkScaledBitmapSampler.cpp File src/images/SkScaledBitmapSampler.cpp (left): https://codereview.chromium.org/24269006/diff/1/src/images/SkScaledBitmapSampler.cpp#oldcode303 src/images/SkScaledBitmapSampler.cpp:303: static bool Sample_Gray_D8888_Unpremul(void* SK_RESTRICT dstRow, This function was redundant, ...
7 years, 3 months ago (2013-09-19 18:43:42 UTC) #1
djsollen
https://codereview.chromium.org/24269006/diff/1/bench/SkipZeroesBench.cpp File bench/SkipZeroesBench.cpp (right): https://codereview.chromium.org/24269006/diff/1/bench/SkipZeroesBench.cpp#newcode104 bench/SkipZeroesBench.cpp:104: //DEF_BENCH( return SkNEW_ARGS(SkipZeroesBench, ("/sdcard/skia/images/arrow.png", true))); seems like we should ...
7 years, 3 months ago (2013-09-19 19:41:17 UTC) #2
reed1
feature creep lives! lgtm In the future... - I will add native support for unpremul ...
7 years, 3 months ago (2013-09-19 19:59:42 UTC) #3
scroggo
https://codereview.chromium.org/24269006/diff/1/bench/SkipZeroesBench.cpp File bench/SkipZeroesBench.cpp (right): https://codereview.chromium.org/24269006/diff/1/bench/SkipZeroesBench.cpp#newcode104 bench/SkipZeroesBench.cpp:104: //DEF_BENCH( return SkNEW_ARGS(SkipZeroesBench, ("/sdcard/skia/images/arrow.png", true))); On 2013/09/19 19:41:17, djsollen ...
7 years, 3 months ago (2013-09-19 21:07:20 UTC) #4
scroggo
On 2013/09/19 21:07:20, scroggo wrote: > https://codereview.chromium.org/24269006/diff/1/bench/SkipZeroesBench.cpp > File bench/SkipZeroesBench.cpp (right): > > https://codereview.chromium.org/24269006/diff/1/bench/SkipZeroesBench.cpp#newcode104 > ...
7 years, 3 months ago (2013-09-24 21:50:21 UTC) #5
scroggo
7 years, 2 months ago (2013-10-01 17:28:26 UTC) #6
Message was sent while issue was closed.
Committed patchset #2 manually as r11558 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698