store SkAlphaType inside SkBitmap, on road to support unpremul

store SkAlphaType inside SkBitmap, on road to support unpremul

BUG=
R=bsalomon@google.com, scroggo@google.com

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

[scroggo, 2013-09-30 19:01:27]

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

https://codereview.chromium.org/25275004/diff/1/include/core/SkBitmap.h#newco...
include/core/SkBitmap.h:685: uint8_t     fAlphaType;
This should be serialized, and we should bump the PICTURE_VERSION (I'm fine to
do it later for convenience/grouping, but the code should at least be there
behind a flag).

[bsalomon, 2013-09-30 19:15:08]

lgtm other than cerealizing.

[reed1, 2013-09-30 20:49:52]

Chrome still has occurances of setIsOpaque... I think most are harmless, but not
quite sure yet. My goal is to deprecate and then remove that API, and have
clients call setAlphaType instead (unless I can get them to *only* specify it in
setConfig).

The problem stems from this:

void setIsOpaque(bool opaque) {
    if (opaque) {
        fAlphaType = kOpaque_SkAlphaType;   // easy
    } else {
        ... do I set it to kPremul or kUnpremul ????
    }
}

[scroggo, 2013-09-30 22:20:04]

> Chrome still has occurances of setIsOpaque... I think most are harmless, but
not
> quite sure yet. My goal is to deprecate and then remove that API, and have
> clients call setAlphaType instead (unless I can get them to *only* specify it
in
> setConfig).
> 
> The problem stems from this:
> 
> void setIsOpaque(bool opaque) {
>     if (opaque) {
>         fAlphaType = kOpaque_SkAlphaType;   // easy
>     } else {
>         ... do I set it to kPremul or kUnpremul ????
>     }
> }

I think we should set to kPremul. kUnpremul doesn't mean anything in the old
world, and we are deprecating this function, so it makes sense that it would
set it to the only thing that made since in the old world.

Android also makes calls to setIsOpaque, which will need to be changed. For
the moment, though, I think it will be okay that setIsOpaque set it to premul,
since if they have something that is unpremul, we've told them it's not a valid
source anyway.

https://codereview.chromium.org/25275004/diff/1/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/25275004/diff/1/src/core/SkBitmap.cpp#newcode290
src/core/SkBitmap.cpp:290: alphaType = kPremul_SkAlphaType;
Why change it to Premul? For historical reasons/so we don't have to add another
type?

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libjpeg.cpp:746: kOpaque_SkAlphaType);
This is a bug in the existing code: this should use the same conditional as the
others (if A8_Config, it should be Premul_SkAlphaType)

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:440:
decodedBitmap->setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
This should check this->getRequireUnpremultipliedColors() and set to
kUnpremul_SkAlphaType if reallyHasAlpha.

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libpng.cpp:892:
decodedBitmap.setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
This should also check requireUnpremul

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageDecoder_...
src/images/SkImageDecoder_libwebp.cpp:297: fHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
This should check this->getRequireUnpremultipliedColors()

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageRef.cpp
File src/images/SkImageRef.cpp (right):

https://codereview.chromium.org/25275004/diff/7001/src/images/SkImageRef.cpp#...
src/images/SkImageRef.cpp:67: // what about colortables??????
It looks like you can check SkColorTable::getFlags()

https://codereview.chromium.org/25275004/diff/7001/src/lazy/SkBitmapFactory.cpp
File src/lazy/SkBitmapFactory.cpp (right):

https://codereview.chromium.org/25275004/diff/7001/src/lazy/SkBitmapFactory.c...
src/lazy/SkBitmapFactory.cpp:65: isOpaque ? kOpaque_SkAlphaType :
kPremul_SkAlphaType);
Why not use info.fAlphaType?

https://codereview.chromium.org/25275004/diff/7001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/25275004/diff/7001/src/lazy/SkLazyPixelRef.cp...
src/lazy/SkLazyPixelRef.cpp:160: isOpaque ? kOpaque_SkAlphaType :
kPremul_SkAlphaType)
Again, I think you can just use info.fAlphaType.

https://codereview.chromium.org/25275004/diff/7001/src/ports/SkImageDecoder_C...
File src/ports/SkImageDecoder_CG.cpp (left):

https://codereview.chromium.org/25275004/diff/7001/src/ports/SkImageDecoder_C...
src/ports/SkImageDecoder_CG.cpp:111: bm->computeAndSetOpaquePredicate();
Should we remove this function entirely?

[scroggo, 2013-10-02 18:49:58]

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

https://codereview.chromium.org/25275004/diff/40001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:440:
decodedBitmap->setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
In addition to checking for unpremultiply, this needs to check the config for
A8, as in https://codereview.chromium.org/25726004/

[reed1, 2013-10-03 08:22:53]

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

https://codereview.chromium.org/25275004/diff/40001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:440:
decodedBitmap->setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
On 2013/10/02 18:49:58, scroggo wrote:
> In addition to checking for unpremultiply, this needs to check the config for
> A8, as in https://codereview.chromium.org/25726004/

Not sure what the change to the code should be. If the png is A8, I presume
reallyHasAlpha will be true, in which case I'll store kPremul. Is that not
correct?

[scroggo, 2013-10-03 14:05:19]

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

https://codereview.chromium.org/25275004/diff/40001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:440:
decodedBitmap->setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
On 2013/10/03 08:22:54, reed1 wrote:
> On 2013/10/02 18:49:58, scroggo wrote:
> > In addition to checking for unpremultiply, this needs to check the config
for
> > A8, as in https://codereview.chromium.org/25726004/
> 
> Not sure what the change to the code should be. If the png is A8, I presume
> reallyHasAlpha will be true, in which case I'll store kPremul. Is that not
> correct?

It doesn't currently change reallyHasAlpha, but it could. My latest patch
changes reallyHasAlpha.

This code still needs to check this->getRequireUnpremultipliedColors(). If true,
the alpha type should be kUnpremul.

[reed1, 2013-10-18 15:27:05]

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

https://codereview.chromium.org/25275004/diff/40001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:440:
decodedBitmap->setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
On 2013/10/03 14:05:19, scroggo wrote:
> On 2013/10/03 08:22:54, reed1 wrote:
> > On 2013/10/02 18:49:58, scroggo wrote:
> > > In addition to checking for unpremultiply, this needs to check the config
> for
> > > A8, as in https://codereview.chromium.org/25726004/
> > 
> > Not sure what the change to the code should be. If the png is A8, I presume
> > reallyHasAlpha will be true, in which case I'll store kPremul. Is that not
> > correct?
> 
> It doesn't currently change reallyHasAlpha, but it could. My latest patch
> changes reallyHasAlpha.
> 
> This code still needs to check this->getRequireUnpremultipliedColors(). If
true,
> the alpha type should be kUnpremul.

Done.

[scroggo, 2013-10-18 19:32:40]

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:283: switch (config) {
Should this call setAlphaType so we don't have code duplication? The comment
"not supported yet" hints that we may be adding that feature in the future, at
which time we'll need to update both pieces of code.

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:585: #if 0
Any reason not to delete this code?

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:615: #if 0
Any reason not to delete this code?

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1023: this->isOpaque() ?
Why not use fAlphaType?

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1045: this->isOpaque() ? kOpaque_SkAlphaType :
kPremul_SkAlphaType);
fAlphaType?

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1195: tmpDst.setAlphaType(src->isOpaque() ?
It seems odd here to set the alpha type after we've drawn to it. It might need
to be specified individually for the different cases. If we did a straight
memcpy (i.e. if the configs are the same), we should be setting it to
fAlphaType. If we are copying from 8888 to 565, it is odd to potentially set it
to Premul (though setAlphaType will correct that for us). If we're doing some
other copy (8888 to 4444, if we'll still allow that), we'd want the dest to be
unpremul if the source was (assuming we copied it as unpremul).

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1669: bool isOpaque = buffer.readBool();
Why not update flatten to write the alpha type and then set it directly here?

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmapDevice....
src/core/SkBitmapDevice.cpp:30: fBitmap.setConfig(config, width, height, 0,
isOpaque ?
Should we update the parameter? Maybe in the future?

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmapScaler.cpp
File src/core/SkBitmapScaler.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/core/SkBitmapScaler....
src/core/SkBitmapScaler.cpp:291: source.isOpaque() ? kOpaque_SkAlphaType :
kPremul_SkAlphaType);
Why not set it to source.alphaType()?

https://codereview.chromium.org/25275004/diff/170001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:142: SkBitmap::Config config =
grConfig2skConfig(renderTarget->config(), &isOpaque);
Will rendertargets support alphatype at some point? Maybe a TODO here that
grConfig2skConfig should support it in the future?

https://codereview.chromium.org/25275004/diff/170001/src/image/SkImage_Raster...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/image/SkImage_Raster...
src/image/SkImage_Raster.cpp:89: SkBitmap::Config config =
SkImageInfoToBitmapConfig(info, &isOpaque);
I think this function should be updated. Since both SkBitmap and SkImage::Info
now have a field for alpha type, it seems odd to convert the alpha type to the
smaller space of isOpaque, the back to alpha type, losing information in the
process.

https://codereview.chromium.org/25275004/diff/170001/src/image/SkSurface_Rast...
File src/image/SkSurface_Raster.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/image/SkSurface_Rast...
src/image/SkSurface_Raster.cpp:89: fBitmap.setConfig(config, info.fWidth,
info.fHeight, rb, isOpaque ?
Again, these should use the alpha type from the info.

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
src/images/SkImageDecoder.cpp:248: dst->setConfig(src->getConfig(), w, h, 0,
src->isOpaque() ?
src->alphaType()

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
src/images/SkImageDecoder_libjpeg.cpp:584: SkBitmap::kA8_Config != config ?
kOpaque_SkAlphaType : kPremul_SkAlphaType);
nit: It's a little confusing that this call to setConfig negates the equality
check.

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
src/images/SkImageDecoder_libpng.cpp:951:
decodedBitmap.setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
This should also check for getRequireUnpremultiplied.

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageDecode...
src/images/SkImageDecoder_libwebp.cpp:297: fHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
This should check RequireUnpremultiplied

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageRef.cpp
File src/images/SkImageRef.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageRef.cp...
src/images/SkImageRef.cpp:64: bool SkImageRef::isOpaque(SkBitmap* bitmap) {
Doesn't necessarily need to be fixed here, but looking at the header, this
function does not return what it claims to be returning. This will may return
true even if fBitmap is not opaque. (I don't think anyone actually looks at the
return value, though.)

https://codereview.chromium.org/25275004/diff/170001/src/images/SkImageRef.cp...
src/images/SkImageRef.cpp:67: // what about colortables??????
Didn't you make colortables immutable so that you would not need to lock and
unlock? I think just checking fBitmap should be fine.

https://codereview.chromium.org/25275004/diff/170001/src/lazy/SkBitmapFactory...
File src/lazy/SkBitmapFactory.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/lazy/SkBitmapFactory...
src/lazy/SkBitmapFactory.cpp:65: isOpaque ? kOpaque_SkAlphaType :
kPremul_SkAlphaType);
info should have the actual alpha type.

https://codereview.chromium.org/25275004/diff/170001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/lazy/SkLazyPixelRef....
src/lazy/SkLazyPixelRef.cpp:160: isOpaque ? kOpaque_SkAlphaType :
kPremul_SkAlphaType)
info has the actual alpha type.

https://codereview.chromium.org/25275004/diff/170001/tests/BitmapCopyTest.cpp
File tests/BitmapCopyTest.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/tests/BitmapCopyTest.cpp...
tests/BitmapCopyTest.cpp:313: <<<<<<< HEAD
Needs merge.

[scroggo, 2013-10-18 20:04:33]

https://codereview.chromium.org/25275004/diff/170001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:142: SkBitmap::Config config =
grConfig2skConfig(renderTarget->config(), &isOpaque);
On 2013/10/18 19:32:40, scroggo wrote:
> Will rendertargets support alphatype at some point? Maybe a TODO here that
> grConfig2skConfig should support it in the future?

Thoughts?

https://codereview.chromium.org/25275004/diff/170001/src/image/SkImage_Raster...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/image/SkImage_Raster...
src/image/SkImage_Raster.cpp:89: SkBitmap::Config config =
SkImageInfoToBitmapConfig(info, &isOpaque);
On 2013/10/18 19:32:40, scroggo wrote:
> I think this function should be updated. Since both SkBitmap and SkImage::Info
> now have a field for alpha type, it seems odd to convert the alpha type to the
> smaller space of isOpaque, the back to alpha type, losing information in the
> process.

All classes of using SkImageInfoToBitmapInfo should be fixed.

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:284: switch (config) {
Thiis comment still holds - can this call setAlphaType to avoid code
duplication?

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapDevice....
src/core/SkBitmapDevice.cpp:30: fBitmap.setConfig(config, width, height, 0,
isOpaque ?
Should we update the parameter or perhaps add a TODO?

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapScaler.cpp
File src/core/SkBitmapScaler.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapScaler....
src/core/SkBitmapScaler.cpp:290: destSubset.width(), destSubset.height(), 0,
source.alphaType()

https://codereview.chromium.org/25275004/diff/140002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:146: isOpaque ? kOpaque_SkAlphaType :
kPremul_SkAlphaType);
This seems to not be very future proof once we actually support unpremul

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
src/images/SkImageDecoder.cpp:248: dst->setConfig(src->getConfig(), w, h, 0,
src->isOpaque() ?
src->alphaType()

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
src/images/SkImageDecoder_libpng.cpp:951:
decodedBitmap.setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
Needs to check unpremul

[reed1, 2013-10-18 21:37:34]

https://codereview.chromium.org/25275004/diff/170001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:142: SkBitmap::Config config =
grConfig2skConfig(renderTarget->config(), &isOpaque);
On 2013/10/18 20:04:34, scroggo wrote:
> On 2013/10/18 19:32:40, scroggo wrote:
> > Will rendertargets support alphatype at some point? Maybe a TODO here that
> > grConfig2skConfig should support it in the future?
> 
> Thoughts?

Done.

https://codereview.chromium.org/25275004/diff/170001/src/image/SkImage_Raster...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/25275004/diff/170001/src/image/SkImage_Raster...
src/image/SkImage_Raster.cpp:89: SkBitmap::Config config =
SkImageInfoToBitmapConfig(info, &isOpaque);
On 2013/10/18 19:32:40, scroggo wrote:
> I think this function should be updated. Since both SkBitmap and SkImage::Info
> now have a field for alpha type, it seems odd to convert the alpha type to the
> smaller space of isOpaque, the back to alpha type, losing information in the
> process.

Done.

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:284: switch (config) {
On 2013/10/18 20:04:34, scroggo wrote:
> Thiis comment still holds - can this call setAlphaType to avoid code
> duplication?

Done.

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapDevice.cpp
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapDevice....
src/core/SkBitmapDevice.cpp:30: fBitmap.setConfig(config, width, height, 0,
isOpaque ?
On 2013/10/18 20:04:34, scroggo wrote:
> Should we update the parameter or perhaps add a TODO?

Actually, the renderers (SW and GPU) can only support these two. kIgnore is just
weird, and kUnpremul isn't supportable.

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapScaler.cpp
File src/core/SkBitmapScaler.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/core/SkBitmapScaler....
src/core/SkBitmapScaler.cpp:290: destSubset.width(), destSubset.height(), 0,
On 2013/10/18 20:04:34, scroggo wrote:
> source.alphaType()

Done.

https://codereview.chromium.org/25275004/diff/140002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:146: isOpaque ? kOpaque_SkAlphaType :
kPremul_SkAlphaType);
On 2013/10/18 20:04:34, scroggo wrote:
> This seems to not be very future proof once we actually support unpremul

Render targets don't support unpremul

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
src/images/SkImageDecoder.cpp:248: dst->setConfig(src->getConfig(), w, h, 0,
src->isOpaque() ?
On 2013/10/18 20:04:34, scroggo wrote:
> src->alphaType()

Done.

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/25275004/diff/140002/src/images/SkImageDecode...
src/images/SkImageDecoder_libpng.cpp:951:
decodedBitmap.setAlphaType(reallyHasAlpha ? kPremul_SkAlphaType :
kOpaque_SkAlphaType);
On 2013/10/18 20:04:34, scroggo wrote:
> Needs to check unpremul

Done.

[scroggo, 2013-10-18 21:52:40]

lgtm

https://codereview.chromium.org/25275004/diff/420001/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/25275004/diff/420001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:46: fDevice->clear(0x0);
Not needed for this CL, obviously, but is this a candidate for calloc?

[reed1, 2013-10-21 12:24:24]

https://codereview.chromium.org/25275004/diff/420001/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/25275004/diff/420001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:46: fDevice->clear(0x0);
On 2013/10/18 21:52:41, scroggo wrote:
> Not needed for this CL, obviously, but is this a candidate for calloc?

The clear() turns into a GPU command, and does not write memory (at least from
calloc's perspective). However, it is worth asking brian if we can skip this by
somehow allocating the rendertarget/texture to be automatically zero'd...

[bsalomon, 2013-10-21 13:18:03]

https://codereview.chromium.org/25275004/diff/420001/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/25275004/diff/420001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:46: fDevice->clear(0x0);
On 2013/10/21 12:24:25, reed1 wrote:
> On 2013/10/18 21:52:41, scroggo wrote:
> > Not needed for this CL, obviously, but is this a candidate for calloc?
> 
> The clear() turns into a GPU command, and does not write memory (at least from
> calloc's perspective). However, it is worth asking brian if we can skip this
by
> somehow allocating the rendertarget/texture to be automatically zero'd...

Not that I know of. On some OSes newly created textures are zeroed, but we can't
assume it generally.

[reed1, 2013-10-21 14:00:22]

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

[scroggo, 2014-06-10 20:12:00]

https://codereview.chromium.org/25275004/diff/420001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/25275004/diff/420001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:279: alphaType = kPremul_SkAlphaType;
Looking back at this as I write tests for Android. (I know the code has changed
since then, but it's essentially the same with s/Config/ColorType and -A1.)

Why is A8 forced to premul, but only if we asked for unpremul? It appears that
A8 can be any of:

premul
opaque
ignore

A8 is sort of a strange one to have an alphatype, since it is ONLY alpha, but it
seems like it would make sense for it to have one of them all the time.