Refactor SkPDFImage

src/pdf/SkPDFImage.cpp

Index: src/pdf/SkPDFImage.cpp
diff --git a/src/pdf/SkPDFImage.cpp b/src/pdf/SkPDFImage.cpp
index a5cb4c20d1b9e187948cc6654d590b219df4d686..9bc07d6994bc998a76322f9cfe4cb77ae96b628b 100644
--- a/src/pdf/SkPDFImage.cpp
+++ b/src/pdf/SkPDFImage.cpp
@@ -10,204 +10,342 @@
 #include "SkBitmap.h"
 #include "SkColor.h"
 #include "SkColorPriv.h"
+#include "SkData.h"
+#include "SkFlate.h"
 #include "SkPDFCatalog.h"
 #include "SkRect.h"
 #include "SkStream.h"
 #include "SkString.h"
 #include "SkUnPreMultiply.h"
 
-namespace {
+static const int kNoColorTransform = 0;
 
-void extractImageData(const SkBitmap& bitmap, const SkIRect& srcRect,
-                      SkStream** imageData, SkStream** alphaData) {
-    SkMemoryStream* image = NULL;
-    SkMemoryStream* alpha = NULL;
-    bool hasAlpha = false;
-    bool isTransparent = false;
+static bool skip_compression(SkPDFCatalog* catalog) {
+    return SkToBool(catalog->getDocumentFlags() &
+                    SkPDFDocument::kFavorSpeedOverSize_Flags);
+}
 
-    bitmap.lockPixels();
+static size_t get_row_bytes(const SkBitmap& bitmap,
+                            const SkIRect& srcRect) {
+    switch (bitmap.getConfig()) {
+        case SkBitmap::kIndex8_Config:
+            return srcRect.width();
+        case SkBitmap::kARGB_4444_Config:
+            return (srcRect.width() * 3 + 1) / 2;
+        case SkBitmap::kRGB_565_Config:
+            return srcRect.width() * 3;
+        case SkBitmap::kARGB_8888_Config:
+            return srcRect.width() * 3;
+        case SkBitmap::kA1_Config:
+        case SkBitmap::kA8_Config:
+            return 1;
+        default:
+            SkASSERT(false);
+            return 0;
+    }
+}
+
+static size_t get_uncompressed_size(const SkBitmap& bitmap,
+                                    const SkIRect& srcRect) {
     switch (bitmap.getConfig()) {
-        case SkBitmap::kIndex8_Config: {
-            const int rowBytes = srcRect.width();
-            image = new SkMemoryStream(rowBytes * srcRect.height());
-            uint8_t* dst = (uint8_t*)image->getMemoryBase();
-            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
-                memcpy(dst, bitmap.getAddr8(srcRect.fLeft, y), rowBytes);
-                dst += rowBytes;
+        case SkBitmap::kIndex8_Config:
+        case SkBitmap::kARGB_4444_Config:
+        case SkBitmap::kRGB_565_Config:
+        case SkBitmap::kARGB_8888_Config:
+            return get_row_bytes(bitmap, srcRect) * srcRect.height();

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

Here and extract_index8_image are the only place that get_row_bytes is used.  It
doesn't appear to be a useful abstraction.  Inline it here and at the other call
site.

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

Done.

+        case SkBitmap::kA1_Config:
+        case SkBitmap::kA8_Config:
+            return 1;
+        default:
+            SkASSERT(false);
+            return 0;
+    }
+}
+
+static SkStream* extract_index8_image(const SkBitmap& bitmap,
+                                      const SkIRect& srcRect) {
+    const int rowBytes = get_row_bytes(bitmap, srcRect);
+    SkStream* stream = SkNEW_ARGS(SkMemoryStream,
+                                  (rowBytes * srcRect.height()));
+    uint8_t* dst = (uint8_t*)stream->getMemoryBase();
+
+    for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
+        memcpy(dst, bitmap.getAddr8(srcRect.fLeft, y), rowBytes);
+        dst += rowBytes;
+    }
+    return stream;
+}
+
+static SkStream* extract_argb4444_data(const SkBitmap& bitmap,
+                                       const SkIRect& srcRect,
+                                       bool extractAlpha,
+                                       bool* hasAlpha,

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

If it doesn't have alpha, or is completely transparent, and we are trying to
extract alpha, then it should just return NULL.  I don't think you need this
args.

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

But they're necessary for the optimizations: you need to differentiate hasAlpha
from isTransparent - if its's transparent, you can skip creating the image at
all. However, if it just has no alpha, you only skip the SMask.

[vandebo (ex-Chrome), 2013/08/23 15:45:35]

Ok, but you don't need both of them. I would suggest keeping isOpaque

[ducky, 2013/08/23 17:59:50]

Both are here so that the final check if transparent or opaque and free the
stream can all be put into extract_image_data, instead of copies in all
functions with alpha.

+                                       bool* isTransparent) {
+    SkStream* stream;
+    uint8_t* dst = NULL;
+    if (extractAlpha) {
+        const int alphaRowBytes = (srcRect.width() + 1) / 2;
+        stream = SkNEW_ARGS(SkMemoryStream,
+                            (alphaRowBytes * srcRect.height()));
+    } else {
+        stream = SkNEW_ARGS(SkMemoryStream,
+                            (get_uncompressed_size(bitmap, srcRect)));
+    }
+    dst = (uint8_t*)stream->getMemoryBase();
+
+    for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
+        uint16_t* src = bitmap.getAddr16(0, y);
+        int x;
+        for (x = srcRect.fLeft; x + 1 < srcRect.fRight; x += 2) {
+            if (extractAlpha) {
+                dst[0] = (SkGetPackedA4444(src[x]) << 4) |
+                    SkGetPackedA4444(src[x + 1]);
+                if (dst[0] != SK_AlphaOPAQUE) {

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

This can probably made more streamlined....

hasAlpha |= dst[0] != SK_AlphaOPAQUE;
isTransparent &= dst[0] == SK_AlphaTRANSPARENT;

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

Nice. Will do.

+                    *hasAlpha = true;
+                }
+                if (dst[0] != SK_AlphaTRANSPARENT) {
+                    *isTransparent = false;
+                }
+                dst++;
+            } else {
+                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;
             }
-            break;
         }
-        case SkBitmap::kARGB_4444_Config: {
-            isTransparent = true;
-            const int rowBytes = (srcRect.width() * 3 + 1) / 2;
-            const int alphaRowBytes = (srcRect.width() + 1) / 2;
-            image = new SkMemoryStream(rowBytes * srcRect.height());
-            alpha = new SkMemoryStream(alphaRowBytes * srcRect.height());
-            uint8_t* dst = (uint8_t*)image->getMemoryBase();
-            uint8_t* alphaDst = (uint8_t*)alpha->getMemoryBase();
-            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
-                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) {
-                        hasAlpha = true;
-                    }
-                    if (alphaDst[0]) {
-                        isTransparent = false;
-                    }
-                    alphaDst++;
+        if (srcRect.width() & 1) {
+            if (extractAlpha) {
+                dst[0] = (SkGetPackedA4444(src[x]) << 4);
+                if (dst[0] != (SK_AlphaOPAQUE & 0xF0)) {
+                    *hasAlpha = true;

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

A better name for hasAlpha is probably the opposite notion - isOpaque

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

Ok. It does become more consistent with SkBitmap too. And it's cleaner with the
isTransparent code.

                 }
-                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);
-                    if (alphaDst[0] != 0xF0) {
-                        hasAlpha = true;
-                    }
-                    if (alphaDst[0] & 0xF0) {
-                        isTransparent = false;
-                    }
-                    alphaDst++;
+                if (dst[0] != (SK_AlphaTRANSPARENT & 0xF0)) {
+                    *isTransparent = false;
                 }
+                dst++;
+
+            } else {
+                dst[0] = (SkGetPackedR4444(src[x]) << 4) |
+                    SkGetPackedG4444(src[x]);
+                dst[1] = (SkGetPackedB4444(src[x]) << 4);
+                dst += 2;
             }
-            break;
         }
-        case SkBitmap::kRGB_565_Config: {
-            const int rowBytes = srcRect.width() * 3;
-            image = new SkMemoryStream(rowBytes * srcRect.height());
-            uint8_t* dst = (uint8_t*)image->getMemoryBase();
-            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
-                uint16_t* src = bitmap.getAddr16(0, y);
-                for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {
-                    dst[0] = SkGetPackedR16(src[x]);
-                    dst[1] = SkGetPackedG16(src[x]);
-                    dst[2] = SkGetPackedB16(src[x]);
-                    dst += 3;
+    }
+    return stream;
+}
+
+static SkStream* extract_rgb565_image(const SkBitmap& bitmap,
+                                      const SkIRect& srcRect) {
+    SkStream* stream = SkNEW_ARGS(SkMemoryStream,
+                                  (get_uncompressed_size(bitmap,
+                                                     srcRect)));
+    uint8_t* dst = (uint8_t*)stream->getMemoryBase();
+    for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
+        uint16_t* src = bitmap.getAddr16(0, y);
+        for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {
+            dst[0] = SkGetPackedR16(src[x]);
+            dst[1] = SkGetPackedG16(src[x]);
+            dst[2] = SkGetPackedB16(src[x]);
+            dst += 3;
+        }
+    }
+    return stream;
+}
+
+static SkStream* extract_argb8888_data(const SkBitmap& bitmap,
+                                       const SkIRect& srcRect,
+                                       bool extractAlpha,
+                                       bool* hasAlpha,
+                                       bool* isTransparent) {
+    SkStream* stream;
+    if (extractAlpha) {
+        stream = SkNEW_ARGS(SkMemoryStream,
+                            (srcRect.width() * srcRect.height()));
+    } else {
+        stream = SkNEW_ARGS(SkMemoryStream,
+                            (get_uncompressed_size(bitmap, srcRect)));
+    }
+    uint8_t* dst = (uint8_t*)stream->getMemoryBase();
+
+    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++) {
+            if (extractAlpha) {
+                dst[0] = SkGetPackedA32(src[x]);
+                if (dst[0] != SK_AlphaOPAQUE) {
+                    *hasAlpha = true;
                 }
+                if (dst[0] != SK_AlphaTRANSPARENT) {
+                    *isTransparent = false;
+                }
+                dst++;
+            } else {
+                dst[0] = SkGetPackedR32(src[x]);
+                dst[1] = SkGetPackedG32(src[x]);
+                dst[2] = SkGetPackedB32(src[x]);
+                dst += 3;
             }
-            break;
         }
-        case SkBitmap::kARGB_8888_Config: {
-            isTransparent = true;
-            const int rowBytes = srcRect.width() * 3;
-            image = new SkMemoryStream(rowBytes * srcRect.height());
-            alpha = new SkMemoryStream(srcRect.width() * srcRect.height());
-            uint8_t* dst = (uint8_t*)image->getMemoryBase();
-            uint8_t* alphaDst = (uint8_t*)alpha->getMemoryBase();
-            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) {
-                        hasAlpha = true;
-                    }
-                    if (alphaDst[0]) {
-                        isTransparent = false;
-                    }
-                    alphaDst++;
-                }
+    }
+    return stream;
+}
+
+static SkStream* extract_a1_alpha(const SkBitmap& bitmap,
+                                  const SkIRect& srcRect,
+                                  bool* hasAlpha,
+                                  bool* isTransparent) {
+    const int alphaRowBytes = (srcRect.width() + 7) / 8;
+    SkStream* stream = SkNEW_ARGS(SkMemoryStream,
+                                  (alphaRowBytes * srcRect.height()));
+    uint8_t* alphaDst = (uint8_t*)stream->getMemoryBase();
+
+    int offset1 = srcRect.fLeft % 8;
+    int offset2 = 8 - offset1;
+
+    for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
+        uint8_t* src = bitmap.getAddr1(0, y);
+        // This may read up to one byte after src, but the
+        // potentially invalid bits are never used for computation.
+        for (int x = srcRect.fLeft; x < srcRect.fRight; x += 8)  {
+            if (offset1) {
+                alphaDst[0] = src[x / 8] << offset1 |
+                    src[x / 8 + 1] >> offset2;
+            } else {
+                alphaDst[0] = src[x / 8];
             }
-            break;
+            if (x + 7 < srcRect.fRight && alphaDst[0] != SK_AlphaOPAQUE) {
+                *hasAlpha = true;
+            }
+            if (x + 7 < srcRect.fRight && alphaDst[0] != SK_AlphaTRANSPARENT) {
+                *isTransparent = false;
+            }
+            alphaDst++;
         }
-        case SkBitmap::kA1_Config: {
-            isTransparent = true;
-            image = new SkMemoryStream(1);
-            ((uint8_t*)image->getMemoryBase())[0] = 0;
-
-            const int alphaRowBytes = (srcRect.width() + 7) / 8;
-            alpha = new SkMemoryStream(alphaRowBytes * srcRect.height());
-            uint8_t* alphaDst = (uint8_t*)alpha->getMemoryBase();
-            int offset1 = srcRect.fLeft % 8;
-            int offset2 = 8 - offset1;
-            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
-                uint8_t* src = bitmap.getAddr1(0, y);
-                // This may read up to one byte after src, but the potentially
-                // invalid bits are never used for computation.
-                for (int x = srcRect.fLeft; x < srcRect.fRight; x += 8)  {
-                    if (offset1) {
-                        alphaDst[0] = src[x / 8] << offset1 |
-                            src[x / 8 + 1] >> offset2;
-                    } else {
-                        alphaDst[0] = src[x / 8];
-                    }
-                    if (x + 7 < srcRect.fRight && alphaDst[0] != 0xFF) {
-                        hasAlpha = true;
-                    }
-                    if (x + 7 < srcRect.fRight && alphaDst[0]) {
-                        isTransparent = false;
-                    }
-                    alphaDst++;
-                }
-                // Calculate the mask of bits we're interested in within the
-                // last byte of alphaDst.
-                // width mod 8  == 1 -> 0x80 ... width mod 8 == 7 -> 0xFE
-                uint8_t mask = ~((1 << (8 - (srcRect.width() % 8))) - 1);
-                if (srcRect.width() % 8 && (alphaDst[-1] & mask) != mask) {
-                    hasAlpha = true;
-                }
-                if (srcRect.width() % 8 && (alphaDst[-1] & mask)) {
-                    isTransparent = false;
-                }
+        // Calculate the mask of bits we're interested in within the
+        // last byte of alphaDst.
+        // width mod 8  == 1 -> 0x80 ... width mod 8 == 7 -> 0xFE
+        uint8_t mask = ~((1 << (8 - (srcRect.width() % 8))) - 1);
+        if (srcRect.width() % 8 &&
+                (alphaDst[-1] & mask) != (SK_AlphaOPAQUE & mask)) {
+            *hasAlpha = true;
+        }
+        if (srcRect.width() % 8 &&
+                (alphaDst[-1] & mask) != (SK_AlphaTRANSPARENT & mask)) {
+            *isTransparent = false;
+        }
+    }
+    return stream;
+}
+
+static SkStream* extract_a8_alpha(const SkBitmap& bitmap,
+                                  const SkIRect& srcRect,
+                                  bool* hasAlpha,
+                                  bool* isTransparent) {
+    const int alphaRowBytes = srcRect.width();
+    SkStream* stream = SkNEW_ARGS(SkMemoryStream,
+                                  (alphaRowBytes * srcRect.height()));
+    uint8_t* alphaDst = (uint8_t*)stream->getMemoryBase();
+
+    for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
+        uint8_t* src = bitmap.getAddr8(0, y);
+        for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {
+            alphaDst[0] = src[x];
+            if (alphaDst[0] != SK_AlphaOPAQUE) {
+                *hasAlpha = true;
             }
-            break;
+            if (alphaDst[0] != SK_AlphaTRANSPARENT) {
+                *isTransparent = false;
+            }
+            alphaDst++;
         }
-        case SkBitmap::kA8_Config: {
-            isTransparent = true;
-            image = new SkMemoryStream(1);
-            ((uint8_t*)image->getMemoryBase())[0] = 0;
-
-            const int alphaRowBytes = srcRect.width();
-            alpha = new SkMemoryStream(alphaRowBytes * srcRect.height());
-            uint8_t* alphaDst = (uint8_t*)alpha->getMemoryBase();
-            for (int y = srcRect.fTop; y < srcRect.fBottom; y++) {
-                uint8_t* src = bitmap.getAddr8(0, y);
-                for (int x = srcRect.fLeft; x < srcRect.fRight; x++) {
-                    alphaDst[0] = src[x];
-                    if (alphaDst[0] != 0xFF) {
-                        hasAlpha = true;
-                    }
-                    if (alphaDst[0]) {
-                        isTransparent = false;
-                    }
-                    alphaDst++;
-                }
+    }
+    return stream;
+}
+
+static SkStream* create_black_image() {
+    SkStream* stream = SkNEW_ARGS(SkMemoryStream, (1));
+    ((uint8_t*)stream->getMemoryBase())[0] = 0;
+    return stream;
+}
+
+/**
+ * Extract image data to a SkStream. Interlaced alpha is separated into alpha

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

Interlaced usually refers to lines/horizontal stripe interlacing...

"Interlaced alpha is..." => "Either the alpha component or the color data is
extracted into a pdf image stream." or something similar.

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

Done.

+ * and image streams.
+ * @param bitmap        Bitmap to extract data from.
+ * @param srcRect       Region in the bitmap to extract.
+ * @param extractAlpha  Set to true to extract the alpha data, or false to
+ *                      extract the color data.
+ * @param transparent   Whether the alpha is completely transparent. Only valid
+ *                      when extractAlpha == true.
+ * @return              Unencoded image data, or NULL if data was not available
+ *                      OR alpha data was requested but the image was entirely
+ *                      transparent or opaque.

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

If Null is returned when the image is transparent, then you don't need the
isTransparent flag.

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

Necessary to differentiate from opaque case (where we don't need the alpha, but
can't not create the color case).
I think the code is clean enough (or at least not intrusive enough) to keep that
optimization.

+ */
+static SkStream* extract_image_data(const SkBitmap& bitmap,
+                                    const SkIRect& srcRect,
+                                    bool extractAlpha, bool* isTransparent) {
+    SkStream* stream = NULL;
+    bool hasAlpha = false;
+    *isTransparent = true;
+

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

Probably easier to have an early return for the alpha request with a non-alpha
config case...

if (extractAlpha && (bitmap.getConfig() == SkBitmap::kIndex8_Config ||
bitmap.getConfig() == SkBitmap::kRGB_565_Config) {
  return NULL;
}

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

Ehhhhh - the current style uses consistent mechanisms for everything , which I
think is nice. There is a bit of overhead incurred by bitmap.lockPixels(),
though we eventually do need the pixels when we have to generate the color
channel (assuming that SkBitmap does some kind of caching, though).

[vandebo (ex-Chrome), 2013/08/23 15:45:35]

Disagree.  Not sure what you mean by consistent mechanisms, I assume you mean
the code look symmetrical in some way, but it's not - the 8888 and 4444 case are
already different.  If you are reading the code and interested in one particular
case, an early return makes it easier to understand - if it's one of those
cases, then you're done.  If it's not, then there's less code to read below.

[ducky, 2013/08/23 17:59:50]

I see - done.

On 2013/08/23 15:45:35, vandebo wrote:

+    bitmap.lockPixels();
+    switch (bitmap.getConfig()) {
+        case SkBitmap::kIndex8_Config:
+            if (!extractAlpha) {
+                stream = extract_index8_image(bitmap, srcRect);

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

Use of functions makes this one easier to follow.

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

Yeah, the old one was getting super bloated...

+            } else {
+                *isTransparent = false;
+            }
+            break;
+        case SkBitmap::kARGB_4444_Config:
+            stream = extract_argb4444_data(bitmap, srcRect, extractAlpha,
+                                           &hasAlpha, isTransparent);
+            break;
+        case SkBitmap::kRGB_565_Config:
+            if (!extractAlpha) {
+                stream = extract_rgb565_image(bitmap, srcRect);
+            } else {
+                *isTransparent = false;
+            }
+            break;
+        case SkBitmap::kARGB_8888_Config:
+            stream = extract_argb8888_data(bitmap, srcRect, extractAlpha,
+                                           &hasAlpha, isTransparent);
+            break;
+        case SkBitmap::kA1_Config:
+            if (extractAlpha) {
+                stream = extract_a1_alpha(bitmap, srcRect,
+                                          &hasAlpha, isTransparent);
+            } else {
+                stream =  create_black_image();
+            }
+            break;
+        case SkBitmap::kA8_Config:
+            if (extractAlpha) {
+                stream = extract_a8_alpha(bitmap, srcRect,
+                                          &hasAlpha, isTransparent);
+            } else {
+                stream =  create_black_image();
             }
             break;
-        }
         default:
             SkASSERT(false);
     }
     bitmap.unlockPixels();
 
-    if (isTransparent) {
-        SkSafeUnref(image);
-    } else {
-        *imageData = image;
-    }
-
-    if (isTransparent || !hasAlpha) {
-        SkSafeUnref(alpha);
-    } else {
-        *alphaData = alpha;
+    if (extractAlpha && (*isTransparent || !hasAlpha)) {
+        SkSafeUnref(stream);
+        return NULL;
     }
+    return stream;
 }
 
-SkPDFArray* makeIndexedColorSpace(SkColorTable* table) {
+static SkPDFArray* makeIndexedColorSpace(SkColorTable* table) {

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

static functions should have hacker case: make_indexed_color_space

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

Missed updating this one... done.

     SkPDFArray* result = new SkPDFArray();
     result->reserve(4);
     result->appendName("Indexed");
@@ -229,8 +367,6 @@ SkPDFArray* makeIndexedColorSpace(SkColorTable* table) {
     return result;
 }
 
-};  // namespace
-
 // static
 SkPDFImage* SkPDFImage::CreateImage(const SkBitmap& bitmap,
                                     const SkIRect& srcRect,
@@ -239,24 +375,20 @@ SkPDFImage* SkPDFImage::CreateImage(const SkBitmap& bitmap,
         return NULL;
     }
 
-    SkStream* imageData = NULL;
-    SkStream* alphaData = NULL;
-    extractImageData(bitmap, srcRect, &imageData, &alphaData);
-    SkAutoUnref unrefImageData(imageData);
-    SkAutoUnref unrefAlphaData(alphaData);
-    if (!imageData) {
-        SkASSERT(!alphaData);
+    bool isTransparent;
+    SkAutoTUnref<SkStream> alphaData(

[vandebo (ex-Chrome), 2013/08/23 05:45:10]

nit: if bitmap.isOpaque() is set, then you can skip extracting the alpha.

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

Done, along with cautionary comment.
Are there any actual guarantees on isOpaque()? By the interface, it seems quite
haphazard - especially since setIsOpaque is a public function...

+            extract_image_data(bitmap, srcRect, true, &isTransparent));
+    if (isTransparent) {
         return NULL;
     }
 
-    SkPDFImage* image =
-        SkNEW_ARGS(SkPDFImage, (imageData, bitmap, srcRect, false, encoder));
-
-    if (alphaData != NULL) {
-        // Don't try to use DCT compression with alpha because alpha is small
-        // anyway and it could lead to artifacts.
-        image->addSMask(SkNEW_ARGS(SkPDFImage, (alphaData, bitmap, srcRect, true, NULL)))->unref();
+    SkPDFImage* image = SkNEW_ARGS(SkPDFImage, (bitmap, srcRect, encoder));
+    if (alphaData.get() != NULL) {
+        SkAutoTUnref<SkPDFImage> mask(
+                SkNEW_ARGS(SkPDFImage, (alphaData.get(), bitmap, srcRect)));
+        image->addSMask(mask);
     }
+
     return image;
 }
 
@@ -276,51 +408,77 @@ void SkPDFImage::getResources(const SkTSet<SkPDFObject*>& knownResourceObjects,
     GetResourcesHelper(&fResources, knownResourceObjects, newResourceObjects);
 }
 
-SkPDFImage::SkPDFImage(SkStream* imageData,
-                       const SkBitmap& bitmap,
+SkPDFImage::SkPDFImage(const SkBitmap& bitmap,
                        const SkIRect& srcRect,
-                       bool doingAlpha,
                        EncodeToDCTStream encoder)
-        : SkPDFImageStream(imageData, bitmap, srcRect, encoder) {
-    SkBitmap::Config config = bitmap.getConfig();
-    bool alphaOnly = (config == SkBitmap::kA1_Config ||
-                      config == SkBitmap::kA8_Config);
+    : fBitmap(bitmap),
+      fSrcRect(srcRect),
+      fEncoder(encoder),
+      fStreamValid(false) {
+    initImageParams(false);
+}
+
+SkPDFImage::SkPDFImage(SkStream* stream, const SkBitmap& bitmap,

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

This constructor is effectively the same as the previous one.  How about a
constructor that takes a possibly NULL stream and a bool alpha flag?

That would also get rid of initImageParams which is kind of awkward as a method.

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

Done - it is cleaner in many regards. I've clarified the stream and bitmap
behavior in the header comments as well. However, I'm a bit worried about
parameter bloat.
I also don't think initImageParams was that bad - perhaps it might have been
cleaner to pass in a bitmap and srcRect.

+                       const SkIRect& srcRect)
+    : fBitmap(bitmap),
+      fSrcRect(srcRect),
+      fEncoder(NULL),
+      fStreamValid(true) {
+    setData(stream);
+    initImageParams(true);
+}
+
+SkPDFImage::SkPDFImage(SkPDFImage& pdfImage)
+    : SkPDFStream(pdfImage),
+      fBitmap(pdfImage.fBitmap),
+      fSrcRect(pdfImage.fSrcRect),
+      fEncoder(pdfImage.fEncoder),
+      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

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

"...regenerated and re-encoded..." => encoded

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

Done.

+    // populate.
+}
+
+void SkPDFImage::initImageParams(bool isAlpha) {
+    SkBitmap::Config config = fBitmap.getConfig();
 
     insertName("Type", "XObject");
     insertName("Subtype", "Image");
 
-    if (!doingAlpha && alphaOnly) {
+    bool alphaOnly = (config == SkBitmap::kA1_Config ||
+                      config == SkBitmap::kA8_Config);
+
+    if (!isAlpha && alphaOnly) {
         // For alpha only images, we stretch a single pixel of black for
         // the color/shape part.
         SkAutoTUnref<SkPDFInt> one(new SkPDFInt(1));
         insert("Width", one.get());
         insert("Height", one.get());
     } else {
-        insertInt("Width", srcRect.width());
-        insertInt("Height", srcRect.height());
+        insertInt("Width", fSrcRect.width());
+        insertInt("Height", fSrcRect.height());
     }
 
-    // if (!image mask) {
-    if (doingAlpha || alphaOnly) {
+    if (isAlpha || alphaOnly) {
         insertName("ColorSpace", "DeviceGray");
     } else if (config == SkBitmap::kIndex8_Config) {
-        SkAutoLockPixels alp(bitmap);
+        SkAutoLockPixels alp(fBitmap);
         insert("ColorSpace",
-               makeIndexedColorSpace(bitmap.getColorTable()))->unref();
+               makeIndexedColorSpace(fBitmap.getColorTable()))->unref();
     } else {
         insertName("ColorSpace", "DeviceRGB");
     }
-    // }
 
     int bitsPerComp = 8;
     if (config == SkBitmap::kARGB_4444_Config) {
         bitsPerComp = 4;
-    } else if (doingAlpha && config == SkBitmap::kA1_Config) {
+    } else if (isAlpha && config == SkBitmap::kA1_Config) {
         bitsPerComp = 1;
     }
     insertInt("BitsPerComponent", bitsPerComp);
 
     if (config == SkBitmap::kRGB_565_Config) {
+        SkASSERT(!isAlpha);
         SkAutoTUnref<SkPDFInt> zeroVal(new SkPDFInt(0));
         SkAutoTUnref<SkPDFScalar> scale5Val(
                 new SkPDFScalar(SkFloatToScalar(8.2258f)));  // 255/2^5-1
@@ -337,3 +495,46 @@ SkPDFImage::SkPDFImage(SkStream* imageData,
         insert("Decode", decodeValue.get());
     }
 }
+
+bool SkPDFImage::populate(SkPDFCatalog* catalog) {
+    if (getState() == kUnused_State) {
+        // Initializing image data for the first time.
+        SkDynamicMemoryWStream dctCompressedWStream;
+        if (!skip_compression(catalog) && fEncoder &&
+                get_uncompressed_size(fBitmap, fSrcRect) > 1 &&
+                fEncoder(&dctCompressedWStream, fBitmap, fSrcRect) &&
+                dctCompressedWStream.getOffset() <
+                            get_uncompressed_size(fBitmap, fSrcRect)) {
+            SkAutoTUnref<SkData> data(dctCompressedWStream.copyToData());
+            SkAutoTUnref<SkStream> stream(SkNEW_ARGS(SkMemoryStream, (data)));
+            setData(stream.get());
+
+            insertName("Filter", "DCTDecode");
+            insertInt("ColorTransform", kNoColorTransform);
+            insertInt("Length", getData()->getLength());
+            setState(kCompressed_State);
+            return true;
+        }
+        // Fallback method
+        if (!fStreamValid) {

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

Should this be a debug check?  You always expect this to be true, correct?

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

No - when the alpha channel is generated, the stream is saved and fStreamValid
is set to true. In that case, we don't want to extract the color data.
This is more useful when a substitute is created, which relies on going through
the populate process again to create a compressed stream. In that case, it will
skip creating a stream, and go straight to SkPDFStream::populate, which does
flate-encode and adds the length field.

+            bool dummy;

[vandebo (ex-Chrome), 2013/08/23 05:25:20]

dummy -> no_used, but generally I don't think you need this parameter.   If you
do need, it, the method should all passing in a NULL one.

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

Done. Updated code and comments to reflect that this field may be NULL when
alpha data is not requested.

[vandebo (ex-Chrome), 2013/08/23 15:45:35]

It should alway be able to be NULL.

[ducky, 2013/08/23 17:59:50]

Done.

+            SkAutoTUnref<SkStream> stream(
+                    extract_image_data(fBitmap, fSrcRect, false, &dummy));
+            setData(stream);
+            fStreamValid = true;
+        }
+        return INHERITED::populate(catalog);
+    } else if (getState() == kNoCompression_State &&
+            !skip_compression(catalog) &&
+            (SkFlate::HaveFlate() || fEncoder)) {
+        // Compression has not been requested when the stream was first created,
+        // but the new catalog wants it compressed.
+        if (!getSubstitute()) {
+            SkPDFStream* substitute = SkNEW_ARGS(SkPDFImage, (*this));
+            setSubstitute(substitute);
+            catalog->setSubstitute(this, substitute);
+        }
+        return false;
+    }
+    return true;
+}