Add an option to create unpremultiplied bitmaps.

Add an option to create unpremultiplied bitmaps.
Currently they cannot be used directly by Skia, but
the pixels can be used elsewhere.

SkImageDecoder:
Add functions to require unpremultiplied output
and query the presence of the requirement

SkImageDecoder_libpng:
SkImageDecoder_libwebp:
SkImageDecoder_WIC:
Respect the requirement for unpremultiplied output.
TODO: Fix SkImageDecoder_CG.

SkScaledBitmapSampler:
Add procs to skip premultiplication and a boolean
parameter to use those procs.

ImageDecodingTest:
Test unpremultiplied bitmap decoding.

SampleUnpremul:
Add a sample which allows visually comparing between the
unpremultiplied version (copied into a premultiplied bitmap,
since drawing unpremultiplied is not currently supported)
and a premultiplied version of image files.

gm.h:
Add a getter for the resource path, so Samples can use it.

As of patch set 13, https://codereview.chromium.org/16816016/
and https://codereview.chromium.org/16983004/, which were
approved separately.

R=reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9612

[scroggo, 2013-06-12 17:12:01]

Unpremultiplied bitmaps for Android, ready for review at patch set 6.

[djsollen, 2013-06-12 17:59:21]

I figure there has already been discussion on this but I have to ask. :)

https://codereview.chromium.org/16410009/diff/18001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/16410009/diff/18001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:266: bool premul = true);
why not just add a new config for unpremultiplied 8888?

[scroggo, 2013-06-12 18:24:42]

https://codereview.chromium.org/16410009/diff/18001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/16410009/diff/18001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:266: bool premul = true);
On 2013/06/12 17:59:21, djsollen wrote:
> why not just add a new config for unpremultiplied 8888?

That was my first approach, but it was brought up that ultimately we may want
independent choices, more like SkImage. Adding this new config would tie
unpremultiplied to the native color ordering and 8888.

Maybe that's okay though?

[reed1, 2013-06-12 20:20:00]

can we only checkin the codec-related changes for now, and just park the others
changes until we decide if we want to *really* support this beast?

https://codereview.chromium.org/16410009/diff/18001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/16410009/diff/18001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:111: bool premultiplied() const { return 0 == (fFlags &
kColorsAreNotPremultiplied_Flag); }
isPremultiplied() ?
isUnpremultiplied() ?

https://codereview.chromium.org/16410009/diff/18001/include/core/SkColor.h
File include/core/SkColor.h (right):

https://codereview.chromium.org/16410009/diff/18001/include/core/SkColor.h#ne...
include/core/SkColor.h:148: /** Like SkPMColor, this is a 32 bit ARGB
configuration dependent ordered color, but it is
On conservative side, lets not make this public (yet)

[scroggo, 2013-06-12 22:08:28]

Latest patch removes the changes to SkBitmap. Now SkImageDecoder returns an
unpremultiplied bitmap or fails. It is up to the caller to know that it is
unpremultiplied and they should not draw to/from it.

[bsalomon, 2013-06-13 12:24:59]

not really versed in the image code, so skimmed most of this.

https://codereview.chromium.org/16410009/diff/26001/src/core/SkColor.cpp
File src/core/SkColor.cpp (right):

https://codereview.chromium.org/16410009/diff/26001/src/core/SkColor.cpp#newc...
src/core/SkColor.cpp:26: SkPMColor SkPreMultiplyUnPMColor(SkPMColor c) {
Should we have a new type, SkUPMColor? It wouldn't provide any safety, just
documentation. Maybe there is too much code that operates on SkPMColor that we
want to also use with UPM bitmaps to make this a viable suggestion.

[reed1, 2013-06-13 13:51:39]

I had asked to contract the earlier CL to just the minimum need to support
Android's decoding needs for now.

At some point I expect we will fully embrace UPM as a feature, but the first CL
was not that either, and I was nervous to partially expose a feature we didn't
*really* support yet, or commit to any public API changes yet.

https://codereview.chromium.org/16410009/diff/26001/src/core/SkColor.cpp
File src/core/SkColor.cpp (right):

https://codereview.chromium.org/16410009/diff/26001/src/core/SkColor.cpp#newc...
src/core/SkColor.cpp:26: SkPMColor SkPreMultiplyUnPMColor(SkPMColor c) {
Who calls this? Is it declared in a public header? Should it be?

https://codereview.chromium.org/16410009/diff/26001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/16410009/diff/26001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:619: *colorPtr++ = proc(*trans++,
palette->red, palette->green, palette->blue);
Is there a measurable perf-hit with this change (from an inlineable function to
a function-ptr?

[scroggo, 2013-06-13 19:04:49]

https://codereview.chromium.org/16410009/diff/26001/src/core/SkColor.cpp
File src/core/SkColor.cpp (right):

https://codereview.chromium.org/16410009/diff/26001/src/core/SkColor.cpp#newc...
src/core/SkColor.cpp:26: SkPMColor SkPreMultiplyUnPMColor(SkPMColor c) {
On 2013/06/13 13:51:39, reed1 wrote:
> Who calls this? Is it declared in a public header? Should it be?

It is called by tests/ImageDecodingTest.cpp and samplecode/SampleUnpremul.cpp
(both added in this CL). I think that it should not be declared in a public
header since we are not publicly exposing UPM (except in SkImageDecoder).

https://codereview.chromium.org/16410009/diff/26001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/16410009/diff/26001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:619: *colorPtr++ = proc(*trans++,
palette->red, palette->green, palette->blue);
On 2013/06/13 13:51:39, reed1 wrote:
> Is there a measurable perf-hit with this change (from an inlineable function
to
> a function-ptr? 

We don't actually use an inlineable function (though SkPreMultiplyARGB calls an
inlineable function).

I added a benchmark in the next patch. It did not show a consistently
substantial difference among what's checked in, this change, or inlining:

old: what's checked in; new: this CL:
scroggo@scroggo-linux2:/usr/local/google/skia/trunk$ python
bench/bench_compare.py -o master.txt -n unpremul.txt 
     image_decode_hal_163x90.png 8888    c        234.07     257.81     -23.74  
-10.1% 
     image_decode_hal_163x90.png GPU     c        229.48     229.75      -0.27  
 -0.1% 
image_decode_box_19_top-left.png GPU     g          0.02       0.02      +0.00  
 +0.0% 
     image_decode_hal_163x90.png GPU     g          0.02       0.02      +0.00  
 +0.0% 
     image_decode_hal_163x90.png 565     c        230.64     230.11      +0.53  
 +0.2% 
     image_decode_hal_163x90.png NULLGPU c        231.38     230.36      +1.02  
 +0.4% 
image_decode_box_19_top-left.png GPU     c          7.74       7.68      +0.06  
 +0.8% 
image_decode_box_19_top-left.png NULLGPU c          7.86       7.77      +0.09  
 +1.1% 
image_decode_box_19_top-left.png 8888    c          7.85       7.64      +0.21  
 +2.7% 
image_decode_box_19_top-left.png 565     c          7.80       7.57      +0.23  
 +2.9% 

old: inline; new: this CL:
scroggo@scroggo-linux2:/usr/local/google/skia/trunk$ python
bench/bench_compare.py -o withInline.txt -n unpremul.txt 
     image_decode_hal_163x90.png 8888    c        240.63     257.81     -17.18  
 -7.1% 
image_decode_box_19_top-left.png NULLGPU c          7.73       7.77      -0.04  
 -0.5% 
image_decode_box_19_top-left.png 8888    c          7.62       7.64      -0.02  
 -0.3% 
image_decode_box_19_top-left.png GPU     g          0.02       0.02      +0.00  
 +0.0% 
     image_decode_hal_163x90.png GPU     g          0.02       0.02      +0.00  
 +0.0% 
     image_decode_hal_163x90.png GPU     c        230.31     229.75      +0.56  
 +0.2% 
image_decode_box_19_top-left.png 565     c          7.60       7.57      +0.03  
 +0.4% 
     image_decode_hal_163x90.png 565     c        234.79     230.11      +4.68  
 +2.0% 
image_decode_box_19_top-left.png GPU     c          7.92       7.68      +0.24  
 +3.0% 
     image_decode_hal_163x90.png NULLGPU c        244.67     230.36     +14.31  
 +5.8% 


I ran the benchmark on the only files I have locally that follow this path, but
I could try more. Note that all the configs should be the same since the test
does not do any drawing (the following patch sets fIsRendering to false), so the
variance in configs is just showing noise.

[scroggo, 2013-06-13 19:06:49]

https://codereview.chromium.org/16410009/diff/36001/bench/ImageDecodeBench.cpp
File bench/ImageDecodeBench.cpp (right):

https://codereview.chromium.org/16410009/diff/36001/bench/ImageDecodeBench.cp...
bench/ImageDecodeBench.cpp:90: //DEF_BENCH( return SkNEW_ARGS(ImageDecodeBench,
(p, "/usr/local/google/home/scroggo/Downloads/images/hal_163x90.png")); )
I commented these out because these are files from the web which we don't own so
we should not check in.

[reed1, 2013-06-13 19:33:07]

lgtm w/ one last parting shot on the secret color function

https://codereview.chromium.org/16410009/diff/36001/src/core/SkColor.cpp
File src/core/SkColor.cpp (right):

https://codereview.chromium.org/16410009/diff/36001/src/core/SkColor.cpp#newc...
src/core/SkColor.cpp:26: SkPMColor SkPreMultiplyUnPMColor(SkPMColor c) {
Since there is no header for this method, and it is pretty trivial (and there is
only one correct way to impl it), I propose we just copy/paste it into the two
testing/sample files that use it for now.

[scroggo, 2013-06-13 20:10:39]

https://codereview.chromium.org/16410009/diff/36001/src/core/SkColor.cpp
File src/core/SkColor.cpp (right):

https://codereview.chromium.org/16410009/diff/36001/src/core/SkColor.cpp#newc...
src/core/SkColor.cpp:26: SkPMColor SkPreMultiplyUnPMColor(SkPMColor c) {
On 2013/06/13 19:33:07, reed1 wrote:
> Since there is no header for this method, and it is pretty trivial (and there
is
> only one correct way to impl it), I propose we just copy/paste it into the two
> testing/sample files that use it for now.

Done.

[scroggo, 2013-06-14 15:33:32]

Committed patchset #14 manually as r9612 (presubmit successful).