store SkAlphaType inside SkBitmap, on road to support unpremul

src/core/SkBitmap.cpp

Index: src/core/SkBitmap.cpp
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 63a760c4cf7672a15068f8b801efc7a36d7d530e..ae19fdbb9208ecdd23c653ddd7f7201bd90c5988 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -266,33 +266,88 @@ void SkBitmap::getBounds(SkIRect* bounds) const {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-void SkBitmap::setConfig(Config c, int width, int height, size_t rowBytes) {
-    this->freePixels();
-
+bool SkBitmap::setConfig(Config config, int width, int height, size_t rowBytes,
+                         SkAlphaType alphaType) {
     if ((width | height) < 0) {
-        goto err;
+        goto ERROR;
     }
-
     if (rowBytes == 0) {
-        rowBytes = SkBitmap::ComputeRowBytes(c, width);
-        if (0 == rowBytes && kNo_Config != c) {
-            goto err;
+        rowBytes = SkBitmap::ComputeRowBytes(config, width);
+        if (0 == rowBytes && kNo_Config != config) {
+            goto ERROR;
         }
     }
 
-    fConfig     = SkToU8(c);
+    // check for legal/supported config+alphaType combinations
+    //
+    switch (config) {

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

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.

+        case kNo_Config:
+            alphaType = kIgnore_SkAlphaType;    // canonicalize
+            break;
+        case kA1_Config:
+        case kA8_Config:
+            if (kUnpremul_SkAlphaType == alphaType) {
+                alphaType = kPremul_SkAlphaType;
+            }
+            // fall-through
+        case kIndex8_Config:
+        case kARGB_4444_Config:
+        case kARGB_8888_Config:
+            if (kIgnore_SkAlphaType == alphaType) {
+                goto ERROR;   // not supported yet
+            }
+            break;
+        case kRGB_565_Config:
+            alphaType = kOpaque_SkAlphaType;    // canonicalize
+            break;
+    }
+
+    this->freePixels();
+
+    fConfig     = SkToU8(config);
+    fAlphaType  = SkToU8(alphaType);
     fWidth      = width;
     fHeight     = height;
     fRowBytes   = SkToU32(rowBytes);
 
-    fBytesPerPixel = (uint8_t)ComputeBytesPerPixel(c);
+    fBytesPerPixel = (uint8_t)ComputeBytesPerPixel(config);
 
     SkDEBUGCODE(this->validate();)
-    return;
+    return true;
 
     // if we got here, we had an error, so we reset the bitmap to empty
-err:
+ERROR:
     this->reset();
+    return false;
+}
+
+void SkBitmap::setAlphaType(SkAlphaType alphaType) {
+    // check for legal/supported config+alphaType combinations
+    //
+    switch (this->config()) {
+        case kNo_Config:
+            alphaType = kIgnore_SkAlphaType;    // canonicalize
+            break;
+        case kA1_Config:
+        case kA8_Config:
+            if (kUnpremul_SkAlphaType == alphaType) {
+                alphaType = kPremul_SkAlphaType;
+            }
+            // fall-through
+        case kIndex8_Config:
+        case kARGB_4444_Config:
+        case kARGB_8888_Config:
+            if (kIgnore_SkAlphaType == alphaType) {
+                SkDEBUGFAIL("bad alphaType for existing config");
+                return;
+            }
+            break;
+        case kRGB_565_Config:
+            alphaType = kOpaque_SkAlphaType;    // canonicalize
+            break;
+    }
+    
+    fAlphaType = SkToU8(alphaType);
 }
 
 void SkBitmap::updatePixelsFromRef() const {
@@ -525,6 +580,9 @@ void SkBitmap::setImmutable() {
 }
 
 bool SkBitmap::isOpaque() const {
+    return kOpaque_SkAlphaType == fAlphaType ||
+           kIgnore_SkAlphaType == fAlphaType;
+#if 0

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

Any reason not to delete this code?

     switch (fConfig) {
         case kNo_Config:
             return true;
@@ -551,8 +609,10 @@ bool SkBitmap::isOpaque() const {
             SkDEBUGFAIL("unknown bitmap config pased to isOpaque");
             return false;
     }
+#endif
 }
 
+#if 0

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

Any reason not to delete this code?

 void SkBitmap::setIsOpaque(bool isOpaque) {
     /*  we record this regardless of fConfig, though it is ignored in
         isOpaque() for configs that can't support per-pixel alpha.
@@ -563,6 +623,7 @@ void SkBitmap::setIsOpaque(bool isOpaque) {
         fFlags &= ~kImageIsOpaque_Flag;
     }
 }
+#endif
 
 bool SkBitmap::isVolatile() const {
     return (fFlags & kImageIsVolatile_Flag) != 0;
@@ -958,9 +1019,10 @@ bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
         SkPixelRef* pixelRef = fPixelRef->deepCopy(this->config(), &subset);
         if (pixelRef != NULL) {
             SkBitmap dst;
-            dst.setConfig(this->config(), subset.width(), subset.height());
+            dst.setConfig(this->config(), subset.width(), subset.height(), 0,
+                          this->isOpaque() ?

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

Why not use fAlphaType?

+                          kOpaque_SkAlphaType : kPremul_SkAlphaType);
             dst.setIsVolatile(this->isVolatile());
-            dst.setIsOpaque(this->isOpaque());
             dst.setPixelRef(pixelRef)->unref();
             SkDEBUGCODE(dst.validate());
             result->swap(dst);
@@ -979,9 +1041,9 @@ bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
     }
 
     SkBitmap dst;
-    dst.setConfig(this->config(), r.width(), r.height(), this->rowBytes());
+    dst.setConfig(this->config(), r.width(), r.height(), this->rowBytes(),
+                  this->isOpaque() ? kOpaque_SkAlphaType : kPremul_SkAlphaType);

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

fAlphaType?

     dst.setIsVolatile(this->isVolatile());
-    dst.setIsOpaque(this->isOpaque());
 
     if (fPixelRef) {
         // share the pixelref with a custom offset
@@ -1130,7 +1192,8 @@ bool SkBitmap::copyTo(SkBitmap* dst, Config dstConfig, Allocator* alloc) const {
         canvas.drawBitmap(*src, 0, 0, &paint);
     }
 
-    tmpDst.setIsOpaque(src->isOpaque());
+    tmpDst.setAlphaType(src->isOpaque() ?

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

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).

+                        kOpaque_SkAlphaType : kPremul_SkAlphaType);
 
     dst->swap(tmpDst);
     return true;
@@ -1603,8 +1666,9 @@ void SkBitmap::unflatten(SkFlattenableReadBuffer& buffer) {
     int rowBytes = buffer.readInt();
     int config = buffer.readInt();
 
-    this->setConfig((Config)config, width, height, rowBytes);
-    this->setIsOpaque(buffer.readBool());
+    bool isOpaque = buffer.readBool();

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

Why not update flatten to write the alpha type and then set it directly here?

+    this->setConfig((Config)config, width, height, rowBytes,
+                    isOpaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType);
 
     int reftype = buffer.readInt();
     switch (reftype) {