Unpremultiply SkBitmaps for PDF output

src/pdf/SkPDFImage.cpp

Index: src/pdf/SkPDFImage.cpp
diff --git a/src/pdf/SkPDFImage.cpp b/src/pdf/SkPDFImage.cpp
index a5cb4c20d1b9e187948cc6654d590b219df4d686..d8ad96512b1146af4dc526dc4858cc543d4854c9 100644
--- a/src/pdf/SkPDFImage.cpp
+++ b/src/pdf/SkPDFImage.cpp
@@ -18,6 +18,32 @@
 
 namespace {
 
+/**
+ * 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/07 18:02:47]

Instead of needing two unpremultiply routines to deal with the multiple formats,
why not have one that deals with the output format:

unpremultiple_pixel(uint8_t alpha, uint8_t[3] rgb)

[ducky, 2013/08/08 03:35:11]

I'm not sure that will work - argb4444 is also stored in PDF as a 4-bit value.
The 4-bit alpha also needs to be scaled up to the 8 bits expected by
unpremultiply. Right now, this is all handled by stuff already in SkColorPriv,
so I assume it will be consistent with the way premultiply is done. 

Furthermore, this might turn into 3 functions: unpremul_argb888,
unpremul_argb4444_left, and unpremul_argb444_right (depending on whether the rgb
starts on the left or right half of the byte). Though it's more complex, it does
eliminate the extra packing and unpacking, but only in the argb8888 case (since
you still need to extract and scale up the 4-bit values in argb4444 into 8-bit
values expected by unpremultiply).

What do you think?

[vandebo (ex-Chrome), 2013/08/09 16:56:49]

Sorry I didn't look at it closely enough.  You are right.

I still don't like repacking the data.  How about combining extraction and
unpremultiply into one (per format) helper function that is always done.

void get_argb_values(uint32_t src, uint8_t* a, uint8_t* r, uint8_t* g, uint8_t*
b)

With this, the data doesn't need to be copied again.

+    uint8_t a = SkGetPackedA32(src);

[vandebo (ex-Chrome), 2013/08/09 16:56:49]

Is this currently just the same as PMColorToColor() ?

[ducky, 2013/08/09 21:45:14]

Not exactly. PMColorToColor turns the output into a SkColor, which may be
different from the ARGB 8888 format. This one is guaranteed to keep the
formatting consistent.

[vandebo (ex-Chrome), 2013/08/12 15:56:05]

SkColor.h says that the order of components in SkColor are fixed.

[ducky, 2013/08/12 20:51:58]

Ok. I think this was back from when I actually needed to keep it in the ARGB888
format, before the repacking was moved in here. Changed to just call
PMColorToColor now.

+    SkUnPreMultiply::Scale scale = SkUnPreMultiply::GetScale(a);
+    return SkPackARGB32NoCheck(
+            a,
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedR32(src)),
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedG32(src)),
+            SkUnPreMultiply::ApplyScale(scale, SkGetPackedB32(src)));
+}
+
+static uint16_t unpremultiply_argb4444(uint16_t src) {

[vandebo (ex-Chrome), 2013/08/09 16:56:49]

The 4444 case is more of a pain.  Maybe this should do two 4444 values at once. 
The edge case where we only have one can still use this function, just passing
in 0 for the second pixel.

[ducky, 2013/08/09 21:45:14]

Also may be problematic. On the edge case, it only increments the pointer by 2
bytes, so if we packed 3 bytes (2x 444) into it, it will run off the edge of the
array.

[vandebo (ex-Chrome), 2013/08/12 15:56:05]

Yes, for the edge case, you wouldn't pass in the final destination, pass in a
local variable and copy over the values you need.

[ducky, 2013/08/12 20:51:58]

Done.

+    uint32_t src32 = SkPixel4444ToPixel32(src);

[vandebo (ex-Chrome), 2013/08/09 16:56:49]

Do you need to convert to 8888 space to unpremultiply?  I suspect it will work
just as well to use the 4 bit values directly.  Can you check?  If not, this
should probably be implemented in terms of the other method.

+    uint8_t a = SkGetPackedA32(src32);
+    SkUnPreMultiply::Scale scale = SkUnPreMultiply::GetScale(a);
+    src32 = SkPackARGB32NoCheck(
+                a,
+                SkUnPreMultiply::ApplyScale(scale, SkGetPackedR32(src32)),
+                SkUnPreMultiply::ApplyScale(scale, SkGetPackedG32(src32)),
+                SkUnPreMultiply::ApplyScale(scale, SkGetPackedB32(src32)));
+    return SkPixel32ToPixel4444(src32);
+}
+
 void extractImageData(const SkBitmap& bitmap, const SkIRect& srcRect,
                       SkStream** imageData, SkStream** alphaData) {
     SkMemoryStream* image = NULL;
@@ -49,36 +75,50 @@ void extractImageData(const SkBitmap& bitmap, const SkIRect& srcRect,
                 uint16_t* src = bitmap.getAddr16(0, y);
                 int x;
                 for (x = srcRect.fLeft; x + 1 < srcRect.fRight; x += 2) {
-                    dst[0] = (SkGetPackedR4444(src[x]) << 4) |
-                        SkGetPackedG4444(src[x]);
-                    dst[1] = (SkGetPackedB4444(src[x]) << 4) |
-                        SkGetPackedR4444(src[x + 1]);
-                    dst[2] = (SkGetPackedG4444(src[x + 1]) << 4) |
-                        SkGetPackedB4444(src[x + 1]);
-                    dst += 3;
-                    alphaDst[0] = (SkGetPackedA4444(src[x]) << 4) |
-                        SkGetPackedA4444(src[x + 1]);
-                    if (alphaDst[0] != 0xFF) {
+                    uint16_t pixel0 = src[x], pixel1 = src[x + 1];
+
+                    alphaDst[0] = (SkGetPackedA4444(pixel0) << 4) |
+                                   SkGetPackedA4444(pixel1);
+                    if (SkGetPackedA4444(pixel0) != 0xF) {
+                        hasAlpha = true;
+                        pixel0 = unpremultiply_argb4444(pixel0);
+                    }
+                    if (SkGetPackedA4444(pixel1) != 0xF) {
                         hasAlpha = true;

[vandebo (ex-Chrome), 2013/08/07 18:02:47]

Your unpremultiply call could drop in right here.

+                        pixel1 = unpremultiply_argb4444(pixel1);
                     }
                     if (alphaDst[0]) {
                         isTransparent = false;
                     }
+
+                    dst[0] = (SkGetPackedR4444(pixel0) << 4) |
+                        SkGetPackedG4444(pixel0);
+                    dst[1] = (SkGetPackedB4444(pixel0) << 4) |
+                        SkGetPackedR4444(pixel1);
+                    dst[2] = (SkGetPackedG4444(pixel1) << 4) |
+                        SkGetPackedB4444(pixel1);
+
                     alphaDst++;
+                    dst += 3;
                 }
                 if (srcRect.width() & 1) {
-                    dst[0] = (SkGetPackedR4444(src[x]) << 4) |
-                        SkGetPackedG4444(src[x]);
-                    dst[1] = (SkGetPackedB4444(src[x]) << 4);
-                    dst += 2;
-                    alphaDst[0] = (SkGetPackedA4444(src[x]) << 4);
+                    uint16_t pixel = src[x];
+
+                    alphaDst[0] = (SkGetPackedA4444(pixel) << 4);
                     if (alphaDst[0] != 0xF0) {
                         hasAlpha = true;

[vandebo (ex-Chrome), 2013/08/07 18:02:47]

and here

+                        pixel = unpremultiply_argb4444(pixel);
                     }
                     if (alphaDst[0] & 0xF0) {
                         isTransparent = false;
                     }
+
+                    dst[0] = (SkGetPackedR4444(pixel) << 4) |
+                        SkGetPackedG4444(pixel);
+                    dst[1] = (SkGetPackedB4444(pixel) << 4);
+
                     alphaDst++;
+                    dst += 2;
                 }
             }
             break;
@@ -108,18 +148,22 @@ void extractImageData(const SkBitmap& bitmap, const SkIRect& srcRect,
             for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
                 uint32_t* src = bitmap.getAddr32(0, y);
                 for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {
-                    dst[0] = SkGetPackedR32(src[x]);
-                    dst[1] = SkGetPackedG32(src[x]);
-                    dst[2] = SkGetPackedB32(src[x]);
-                    dst += 3;
-                    alphaDst[0] = SkGetPackedA32(src[x]);
-                    if (alphaDst[0] != 0xFF) {
+                    uint32_t pixel = src[x];
+                    alphaDst[0] = SkGetPackedA32(pixel);
+                    if (alphaDst[0] != SK_AlphaOPAQUE) {
                         hasAlpha = true;

[vandebo (ex-Chrome), 2013/08/07 18:02:47]

and here

+                        pixel = unpremultiply_argb8888(pixel);
                     }
-                    if (alphaDst[0]) {
+                    if (alphaDst[0] != SK_AlphaTRANSPARENT) {
                         isTransparent = false;
                     }
+
+                    dst[0] = SkGetPackedR32(pixel);
+                    dst[1] = SkGetPackedG32(pixel);
+                    dst[2] = SkGetPackedB32(pixel);
+
                     alphaDst++;
+                    dst += 3;
                 }
             }
             break;