Add unpremultiply support and GM

src/pdf/SkPDFImage.cpp

Index: src/pdf/SkPDFImage.cpp
diff --git a/src/pdf/SkPDFImage.cpp b/src/pdf/SkPDFImage.cpp
index 9bc07d6994bc998a76322f9cfe4cb77ae96b628b..f345dfe6f5f0b4f78d855acbc5fca1c4eb53faed 100644
--- a/src/pdf/SkPDFImage.cpp
+++ b/src/pdf/SkPDFImage.cpp
@@ -367,6 +367,166 @@ static SkPDFArray* makeIndexedColorSpace(SkColorTable* table) {
     return result;
 }
 
+/**
+ * Unpremultiply an ARGB color, keeping the output in the same format
+ * as the input.
+ */
+static uint32_t unpremultiply_argb8888(uint32_t src) {

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

nit: src -> pm_color

[ducky, 2013/08/23 08:06:11]

Done.

+    uint8_t a = SkGetPackedA32(src);
+    SkUnPreMultiply::Scale scale = SkUnPreMultiply::GetScale(a);
+    return SkPackARGB32NoCheck(
+            SK_AlphaOPAQUE,
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedR32(src)),

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

I think it's nicer to hide the scale stuff, so on line 376, SkColor color =
PMColorToColor(src);

Then here SkColorGetR(color), SkColorGetG(color), SkColorGetB(color)...

[ducky, 2013/08/23 08:06:11]

Done.

+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedG32(src)),
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedB32(src)));
+}
+
+static uint16_t unpremultiply_argb4444(uint16_t src) {
+    // Unpack and transform the alpha values from 4 bits to 8 bits.
+    // This is necessary since the unpremultiply functions expect to work in
+    // 8-bit space, but we are passing in 4-bit values. Since we scale up
+    // the alpha, we scale down the amount the value is increased by, so that
+    // the results are correct for 4-bit color components.
+    uint8_t alpha = SkGetPackedA4444(src);

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

While it might be a bit wasteful, the simplest thing here is.  Sorry if I've
gone back and forth on this.  Since 4444 is probably going away, the simplest
thing is probably best.

SkPixel32ToPixel4444(unpremultiply_argb8888(SkPixel4444ToPixel32(src)));

[ducky, 2013/08/23 08:06:11]

Ehhhhhh - it would make more sense if the code hadn't been written yet, but
since it's already there, why not leave it?
And is 4444 going away really soon, or is it going to be this half-dead thing
for a while?

[vandebo (ex-Chrome), 2013/08/23 16:09:17]

Just because it's already written is not a good reason to keep it. My suggestion
is simpler to understand with minimal performance overhead.

[ducky, 2013/08/23 18:49:17]

Done.
Though for performance overhead, it could also be argued that this is part of a
fast inner loop, but unless people are generating ridiculously large PDFs with
ridiculously large images, it's probably not noticeable anyways...

On 2013/08/23 16:09:17, vandebo wrote:

+    alpha = (alpha << 4) | alpha;
+    SkUnPreMultiply::Scale scale = SkUnPreMultiply::GetScale(alpha);
+    return SkPackARGB4444(SK_AlphaOPAQUE & 0x0F,
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedR4444(src)),
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedG4444(src)),
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedB4444(src)));
+}
+
+static void argb8888_pixel_sum(uint32_t pixel, uint8_t* count,

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

Since this is only used in one place, I think it'll be simpler to inline it.

[ducky, 2013/08/23 08:06:11]

Ok. Though the nested loops doesn't that look nice either.

+                               uint16_t* r, uint16_t* g, uint16_t* b) {
+    if (SkGetPackedA32(pixel) != SK_AlphaTRANSPARENT) {
+        uint32_t color = unpremultiply_argb8888(pixel);
+        *r += SkGetPackedR32(color);
+        *g += SkGetPackedG32(color);
+        *b += SkGetPackedB32(color);
+        (*count)++;
+    }
+}
+
+static void argb4444_pixel_sum(uint16_t pixel, uint8_t* count,
+                               uint8_t* r, uint8_t* g, uint8_t* b) {
+    if ((SkGetPackedA4444(pixel) & 0x0F) != SK_AlphaTRANSPARENT) {
+        uint16_t color = unpremultiply_argb4444(pixel);
+        *r += SkGetPackedR4444(color);
+        *g += SkGetPackedG4444(color);
+        *b += SkGetPackedB4444(color);
+        (*count)++;
+    }
+}
+
+static uint32_t get_argb8888_neighbor_color(const SkBitmap& bitmap,
+                                            int xOrig, int yOrig) {
+    uint8_t count = 0;
+    uint16_t r = 0;
+    uint16_t g = 0;
+    uint16_t b = 0;
+
+    for (int y = yOrig - 1; y <= yOrig + 1; y++) {
+        if (y < 0 || y >= bitmap.height()) {
+            continue;
+        }
+        uint32_t* src = bitmap.getAddr32(0, y);
+        for (int x = xOrig - 1; x <= xOrig + 1; x++) {
+            if (x < 0 || x >= bitmap.width()) {
+                continue;
+            }
+            argb8888_pixel_sum(src[x], &count, &r, &g, &b);
+        }
+    }
+
+    if (count == 0) {
+        return SkPackARGB32NoCheck(SK_AlphaOPAQUE, 0, 0, 0);

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

return SK_ColorBLACK

[ducky, 2013/08/23 08:06:11]

SkColor packing doesn't necessarily match ARGB8888.

+    } else {
+        return SkPackARGB32NoCheck(SK_AlphaOPAQUE,
+                                   r / count, g / count, b / count);
+    }
+}
+
+static uint16_t get_argb4444_neighbor_color(const SkBitmap& bitmap,

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

nit: ...neighbor_color_avg

[ducky, 2013/08/23 08:06:11]

Done.

+                                            int xOrig, int yOrig) {
+    uint8_t count = 0;
+    uint8_t r = 0;
+    uint8_t g = 0;
+    uint8_t b = 0;
+
+    for (int y = yOrig - 1; y <= yOrig + 1; y++) {
+        if (y < 0 || y >= bitmap.height()) {
+            continue;
+        }
+        uint16_t* src = bitmap.getAddr16(0, y);
+        for (int x = xOrig - 1; x <= xOrig + 1; x++) {
+            if (x < 0 || x >= bitmap.width()) {
+                continue;
+            }
+            argb4444_pixel_sum(src[x], &count, &r, &g, &b);
+        }
+    }
+
+    if (count == 0) {
+        return SkPackARGB4444(SK_AlphaOPAQUE & 0x0F, 0, 0, 0);
+    } else {
+        return SkPackARGB4444(SK_AlphaOPAQUE & 0x0F,
+                                   r / count, g / count, b / count);
+    }
+}
+
+static SkBitmap unpremultiply_bitmap(const SkBitmap& bitmap,
+                                     const SkIRect& srcRect) {
+    SkBitmap outBitmap;
+    outBitmap.setConfig(bitmap.config(), srcRect.width(), srcRect.height());
+    SkASSERT(outBitmap.allocPixels());
+    size_t dstRow = 0;
+
+    outBitmap.lockPixels();
+    bitmap.lockPixels();
+    switch (bitmap.config()) {
+        case SkBitmap::kARGB_4444_Config: {
+            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

y++, dstRow++ ?

[ducky, 2013/08/23 08:06:11]

It's kind of nice to keep the loop self-contained and consistent, though. Also
easier to understand, since I think most loops are styled to touch only one
variable, so the dstRow++ would be less noticeable at a glance.

+                uint16_t* dst = outBitmap.getAddr16(0, dstRow);
+                uint16_t* src = bitmap.getAddr16(0, y);
+                for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

x++, dst++

[ducky, 2013/08/23 08:06:11]

Same as above.

+                    uint8_t a = SkGetPackedA4444(src[x]);
+                    if ((a & 0x0F) == SK_AlphaTRANSPARENT) {

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

if (a == (SK_AlphaTRANSPARENT & 0x0F))

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

A big comment here about why we need to do this would be good.

[ducky, 2013/08/23 08:06:11]

Done.

[ducky, 2013/08/23 08:06:11]

Done.

[vandebo (ex-Chrome), 2013/08/23 16:09:17]

Sorry, I didn't mean a comment about masking the bottle 8-bits.  I meant a
comment about why we do the neighbor averaging.

[ducky, 2013/08/23 18:49:17]

Done.

+                        *dst = get_argb4444_neighbor_color(bitmap, x, y);
+                    } else {
+                        *dst = unpremultiply_argb4444(src[x]);
+                    }
+                    dst++;
+                }
+                dstRow++;
+            }
+        } break;

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

Put the break inside the braces.

[ducky, 2013/08/23 08:06:11]

Done.

+        case SkBitmap::kARGB_8888_Config: {
+            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
+                uint32_t* dst = outBitmap.getAddr32(0, dstRow);
+                uint32_t* src = bitmap.getAddr32(0, y);
+                for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {
+                    uint8_t a = SkGetPackedA32(src[x]);
+                    if (a == SK_AlphaTRANSPARENT) {
+                        *dst = get_argb8888_neighbor_color(bitmap, x, y);
+                    } else {
+                        *dst = unpremultiply_argb8888(src[x]);
+                    }
+                    dst++;
+                }
+                dstRow++;
+            }
+        } break;
+        default:
+            SkASSERT(false);
+    }
+    bitmap.unlockPixels();
+    outBitmap.unlockPixels();
+
+    outBitmap.setImmutable();
+
+    return outBitmap;
+}
+
 // static
 SkPDFImage* SkPDFImage::CreateImage(const SkBitmap& bitmap,
                                     const SkIRect& srcRect,
@@ -382,7 +542,17 @@ SkPDFImage* SkPDFImage::CreateImage(const SkBitmap& bitmap,
         return NULL;
     }
 
-    SkPDFImage* image = SkNEW_ARGS(SkPDFImage, (bitmap, srcRect, encoder));
+    SkPDFImage* image;
+    SkBitmap::Config config = bitmap.config();
+    if (alphaData.get() != NULL && (config == SkBitmap::kARGB_8888_Config ||
+            config == SkBitmap::kARGB_4444_Config)) {
+        SkBitmap unpremulBitmap = unpremultiply_bitmap(bitmap, srcRect);
+        SkIRect newSrcRect = srcRect;
+        newSrcRect.offset(-srcRect.left(), -srcRect.top());
+        image = SkNEW_ARGS(SkPDFImage, (unpremulBitmap, srcRect, encoder));

[vandebo (ex-Chrome), 2013/08/23 06:16:11]

You need to use newSrcRect here.

[ducky, 2013/08/23 08:06:11]

Done.

+    } else {
+        image = SkNEW_ARGS(SkPDFImage, (bitmap, srcRect, encoder));
+    }
     if (alphaData.get() != NULL) {
         SkAutoTUnref<SkPDFImage> mask(
                 SkNEW_ARGS(SkPDFImage, (alphaData.get(), bitmap, srcRect)));
@@ -411,20 +581,20 @@ void SkPDFImage::getResources(const SkTSet<SkPDFObject*>& knownResourceObjects,
 SkPDFImage::SkPDFImage(const SkBitmap& bitmap,
                        const SkIRect& srcRect,
                        EncodeToDCTStream encoder)
-    : fBitmap(bitmap),
-      fSrcRect(srcRect),
+    : fSrcRect(srcRect),
       fEncoder(encoder),
       fStreamValid(false) {
+    initBitmap(bitmap);
     initImageParams(false);
 }
 
 SkPDFImage::SkPDFImage(SkStream* stream, const SkBitmap& bitmap,
                        const SkIRect& srcRect)
-    : fBitmap(bitmap),
-      fSrcRect(srcRect),
+    : fSrcRect(srcRect),
       fEncoder(NULL),
       fStreamValid(true) {
     setData(stream);
+    initBitmap(bitmap);
     initImageParams(true);
 }
 
@@ -436,7 +606,16 @@ SkPDFImage::SkPDFImage(SkPDFImage& pdfImage)
       fStreamValid(pdfImage.fStreamValid) {
     // Nothing to do here - the image params are already copied in SkPDFStream's
     // constructor, and the bitmap will be regenerated and re-encoded in
-    // populate.
+    // populate. It is assumed that the source bitmap is immutable.
+}
+
+void SkPDFImage::initBitmap(const SkBitmap& bitmap) {
+    if (bitmap.isImmutable()) {
+        fBitmap = bitmap;
+    } else {
+        bitmap.deepCopyTo(&fBitmap, bitmap.config());
+        fBitmap.setImmutable();
+    }
 }
 
 void SkPDFImage::initImageParams(bool isAlpha) {