Refactor SkPDFImage

src/pdf/SkPDFImage.cpp

 /*
  * Copyright 2010 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPDFImage.h"
 
 #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 {

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

Looks like you can probably remove the anonymous namespace (since everything in
it is converted to static) - Skia convention.

[ducky, 2013/08/22 03:48:06]

Done.

 
-void extractImageData(const SkBitmap& bitmap, const SkIRect& srcRect,
-                      SkStream** imageData, SkStream** alphaData) {

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

We talked about the different cases the other day:

1) No alpha
1a) Current jpeg compression/ DCT compression, use passed Bitmap
1b) zlib compression, 
2) Alpha in image (need to extra alpha channel)
2a) Put unpremultiplied pixels into a bitmap for DCT compression.
2b) zlib compression, extract color channels and unpremultiply

We should be able to use SkBitmap.isOpaque to determine if we're in case 1 or 2.

In case 1, we don't need to traverse the image until we find out that DCT
compression didn't work out. Can we make an unpremultiplied SkBitmap first, see
if the DCT compression works out to decided if we're in case 2a or 2b, and then
extract the alpha or alpha and image bytes in one pass?

[ducky, 2013/08/22 03:48:06]

Done. I think the merged one is less elegant, but I do see the benefits in
maintainability...

For the sake of other reviewers: to avoid extracting the image twice (once in
extract_image_data, again in DCT encoding if requested), an attempt at DCT
encoding has to happen first. However, we don't know whether compression is
requested until populate(), and by then it's too late to create new objects.
Therefore, extracting alpha and image data happen at different times - image in
populate() and alpha when the SkPDFImage is first created.

-    SkMemoryStream* image = NULL;
-    SkMemoryStream* alpha = NULL;
-    bool hasAlpha = false;
-    bool isTransparent = false;
+#define kNoColorTransform 0

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

const int kNoColorTransform = 0;

[ducky, 2013/08/22 03:48:06]

Done.

 
+static bool skip_compression(SkPDFCatalog* catalog) {
+    return SkToBool(catalog->getDocumentFlags() &
+                    SkPDFDocument::kFavorSpeedOverSize_Flags);
+}
+
+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:
+        case SkBitmap::kARGB_4444_Config:
+        case SkBitmap::kRGB_565_Config:
+        case SkBitmap::kARGB_8888_Config:
+            return get_row_bytes(bitmap, srcRect) * srcRect.height();
+        case SkBitmap::kA1_Config:
+        case SkBitmap::kA8_Config:
+            return 1;
+        default:
+            SkASSERT(false);
+            return 0;
+    }
+}
+
+static SkStream* extract_image_data(const SkBitmap& bitmap,
+                                    const SkIRect& srcRect) {
+    const int rowBytes = get_row_bytes(bitmap, srcRect);
+    SkMemoryStream* image = new SkMemoryStream(get_uncompressed_size(bitmap,
+                                                                     srcRect));
     bitmap.lockPixels();
     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;
             }
             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) |

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

UnPreMultiply?

[ducky, 2013/08/22 03:48:06]

I'm going to do unpremul on the bitmap instead of here.
A future optimization could do unpremul here so you don't have to create a new
bitmap, but that case only happens when DCT encoding is not requested.

                         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) {
                     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++;
                 }
             }
             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;
                 }
             }
             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) {
+                }
+            }
+            break;
+        }
+        case SkBitmap::kA1_Config:
+        case SkBitmap::kA8_Config: {
+            ((uint8_t*)image->getMemoryBase())[0] = 0;

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

Sk_ColorBLACK?

[ducky, 2013/08/22 03:48:06]

No can do... SK_ColorBLACK is a 4-byte value containing alpha. This is just one
byte.

+            break;
+        }
+        default:
+            SkASSERT(false);
+    }
+    bitmap.unlockPixels();
+
+    return image;
+}
+
+// Extract the alpha data from a SkBitmap and output it in a SkStream.
+// alphaData may be NULL if there was no alpha data to extract (image
+// completely opaque).
+// isTransparent outputs true if the alpha is completely transparent.
+static SkStream* extract_alpha_data(const SkBitmap& bitmap,
+                                    const SkIRect& srcRect,
+                                    bool* isTransparent) {
+    SkMemoryStream* alpha = NULL;
+    bool hasAlpha = false;
+    *isTransparent = true;
+
+    bitmap.lockPixels();
+    switch (bitmap.getConfig()) {
+        case SkBitmap::kARGB_4444_Config: {
+            const int alphaRowBytes = (srcRect.width() + 1) / 2;
+            alpha = new SkMemoryStream(alphaRowBytes * srcRect.height());
+            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) {
+                    alphaDst[0] = (SkGetPackedA4444(src[x]) << 4) |
+                        SkGetPackedA4444(src[x + 1]);
+                    if (alphaDst[0] != SK_AlphaOPAQUE) {
                         hasAlpha = true;
                     }
-                    if (alphaDst[0]) {
-                        isTransparent = false;
+                    if (alphaDst[0] != SK_AlphaTRANSPARENT) {
+                        *isTransparent = false;
                     }
                     alphaDst++;
                 }
+                if (srcRect.width() & 1) {
+                    alphaDst[0] = (SkGetPackedA4444(src[x]) << 4);
+                    if (alphaDst[0] != (SK_AlphaOPAQUE & 0xF0)) {
+                        hasAlpha = true;
+                    }
+                    if (alphaDst[0] != (SK_AlphaTRANSPARENT & 0xF0)) {
+                        *isTransparent = false;
+                    }
+                    alphaDst++;
+                }
+            }
+            break;
+        }
+        case SkBitmap::kARGB_8888_Config: {
+            alpha = new SkMemoryStream(srcRect.width() * srcRect.height());
+            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++) {
+                    alphaDst[0] = SkGetPackedA32(src[x]);
+                    if (alphaDst[0] != SK_AlphaOPAQUE) {
+                        hasAlpha = true;
+                    }
+                    if (alphaDst[0] != SK_AlphaTRANSPARENT) {
+                        *isTransparent = false;
+                    }
+                    alphaDst++;
+                }
             }
             break;
         }
         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) {
+                    if (x + 7 < srcRect.fRight &&
+                            alphaDst[0] != SK_AlphaOPAQUE) {
                         hasAlpha = true;
                     }
-                    if (x + 7 < srcRect.fRight && alphaDst[0]) {
-                        isTransparent = false;
+                    if (x + 7 < srcRect.fRight &&
+                            alphaDst[0] != SK_AlphaTRANSPARENT) {
+                        *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) {
+                if (srcRect.width() % 8 &&
+                        ((alphaDst[-1] & mask) != (SK_AlphaOPAQUE & mask))) {
                     hasAlpha = true;
                 }
-                if (srcRect.width() % 8 && (alphaDst[-1] & mask)) {
-                    isTransparent = false;
+                if (srcRect.width() % 8 &&
+                        ((alphaDst[-1] & mask) != (SK_AlphaTRANSPARENT & mask))) {
+                    *isTransparent = false;
                 }
             }
             break;
         }
         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) {
+                    if (alphaDst[0] != SK_AlphaOPAQUE) {
                         hasAlpha = true;
                     }
-                    if (alphaDst[0]) {
-                        isTransparent = false;
+                    if (alphaDst[0] != SK_AlphaTRANSPARENT) {
+                        *isTransparent = false;
                     }
                     alphaDst++;
                 }
             }
             break;
         }
+        case SkBitmap::kRGB_565_Config:
+        case SkBitmap::kIndex8_Config: {
+            *isTransparent = false;
+            break;
+        }
         default:
             SkASSERT(false);
     }
     bitmap.unlockPixels();
 
-    if (isTransparent) {
-        SkSafeUnref(image);
+    if (!hasAlpha || *isTransparent) {
+        SkSafeUnref(alpha);
+        return NULL;
     } else {
-        *imageData = image;
-    }
-
-    if (isTransparent || !hasAlpha) {
-        SkSafeUnref(alpha);
-    } else {
-        *alphaData = alpha;
+        return alpha;
     }
 }
 
 SkPDFArray* makeIndexedColorSpace(SkColorTable* table) {
     SkPDFArray* result = new SkPDFArray();
     result->reserve(4);
     result->appendName("Indexed");
     result->appendName("DeviceRGB");
     result->appendInt(table->count() - 1);
 
@@ skipping 14 matching lines @@
 
 };  // namespace
 
 // static
 SkPDFImage* SkPDFImage::CreateImage(const SkBitmap& bitmap,
                                     const SkIRect& srcRect,
                                     EncodeToDCTStream encoder) {
     if (bitmap.getConfig() == SkBitmap::kNo_Config) {
         return NULL;
     }
+    SkPDFImage* image = SkNEW_ARGS(SkPDFImage, (bitmap, srcRect, encoder));
 
-    SkStream* imageData = NULL;
-    SkStream* alphaData = NULL;
-    extractImageData(bitmap, srcRect, &imageData, &alphaData);
-    SkAutoUnref unrefImageData(imageData);
-    SkAutoUnref unrefAlphaData(alphaData);
-    if (!imageData) {
-        SkASSERT(!alphaData);
+    if (image->isEmpty()) {

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

isEmpty just checks srcRect.  1) I think that's a change in behavior, and 2) it
could just be done here and doesn't need to be a member of SkPDFImage.

Actually if you change this to use one of the InitFrom methods, you can return a
boolean.

[ducky, 2013/08/22 03:48:06]

Good point, it probably does change behavior.
What do you think about just skipping the isTransparent optimization? It seems
like a very edge case - lots of machinery for little gain.
Additionally, if SkBitmap::isOpaque is reliable (I hope it is), we can also
remove the alpha stream generation from the main image constructor (since we can
use isOpaque() to check if we need an SMask, rather than generate the alpha data
and not want to chuck it). This means we can also unify all stream generation in
populate, so the code is a bit more elegant.

On 2013/08/21 22:37:17, vandebo wrote:

+        image->unref();
         return NULL;
+    } else {
+        return image;
     }
-
-    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();
-    }
-    return image;
 }
 
 SkPDFImage::~SkPDFImage() {
     fResources.unrefAll();
 }
 
 SkPDFImage* SkPDFImage::addSMask(SkPDFImage* mask) {
     fResources.push(mask);
     mask->ref();
     insert("SMask", new SkPDFObjRef(mask))->unref();
     return mask;
 }
 
 void SkPDFImage::getResources(const SkTSet<SkPDFObject*>& knownResourceObjects,
                               SkTSet<SkPDFObject*>* newResourceObjects) {
     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) {
+    bool isTransparent;
+    SkAutoTUnref<SkStream> alphaData(extract_alpha_data(bitmap, srcRect,
+                                                        &isTransparent));
+    if (isTransparent) {
+        fSrcRect = SkIRect::MakeEmpty();
+        return;
+    }
+    if (alphaData.get() != NULL) {
+        addSMask(SkNEW_ARGS(SkPDFImage,
+                            (alphaData.get(), bitmap, srcRect)))->unref();
+    }
+
+    initImageParams(false);
+}
+
+SkPDFImage::SkPDFImage(SkStream* stream, const SkBitmap& bitmap,
+                       const SkIRect& srcRect)
+    : fBitmap(bitmap),
+      fSrcRect(srcRect),
+      fEncoder(NULL) {
+    setData(stream);
+    insertInt("Length", getData()->getLength());
+    setState(kNoCompression_State);
+
+    initImageParams(true);
+}
+
+SkPDFImage::SkPDFImage(SkPDFImage& pdfImage)
+    : SkPDFStream(pdfImage),
+      fBitmap(pdfImage.fBitmap),
+      fSrcRect(pdfImage.fSrcRect),
+      fEncoder(pdfImage.fEncoder){
+    // Nothing to do here - the image params are already copied in SkPDFStream's

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

Remove comment.

[ducky, 2013/08/22 03:48:06]

I think the comment about SkPDFStream is helpful - it exposes non-obvious
behavior (that the stuff created in initImageParams is copied by SkPDFStream's
copy constructor).

+    // constructor, and the bitmap will be regenerated and re-encoded in
+    // 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
         SkAutoTUnref<SkPDFScalar> scale6Val(
                 new SkPDFScalar(SkFloatToScalar(4.0476f)));  // 255/2^6-1
         SkAutoTUnref<SkPDFArray> decodeValue(new SkPDFArray());
         decodeValue->reserve(6);
         decodeValue->append(zeroVal.get());
         decodeValue->append(scale5Val.get());
         decodeValue->append(zeroVal.get());
         decodeValue->append(scale6Val.get());
         decodeValue->append(zeroVal.get());
         decodeValue->append(scale5Val.get());
         insert("Decode", decodeValue.get());
     }
 }
+
+SkStream* SkPDFImage::getCompressedStream() {
+    SkDynamicMemoryWStream dctCompressedWStream;
+    if (fEncoder && fEncoder(&dctCompressedWStream, fBitmap, fSrcRect)) {

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

This doesn't handle the case where the image has alpha.

[ducky, 2013/08/22 03:48:06]

Yeah, this regressed a quite a few tests... fixed.
I've added a fStreamValid flag to indicate whether there's already an alpha
stream, so it doesn't attempt to extract the image data and clobber the alpha.
It's kind of ugly how image and alpha extraction needs to happen in two
different places...

On 2013/08/21 22:37:17, vandebo wrote:

+        // Ensure compressed version is smaller than the uncompressed version
+        if (dctCompressedWStream.getOffset() <
+            get_uncompressed_size(fBitmap, fSrcRect)) {

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

if get_uncompressed_size is 1, you probably shouldn't even bother calling
fEncoder

[ducky, 2013/08/22 03:48:06]

Done.

+            SkData* data = dctCompressedWStream.copyToData();
+            SkMemoryStream* stream = SkNEW_ARGS(SkMemoryStream, (data));
+            data->unref();
+            return stream;
+        }
+    }
+    return NULL;
+}
+
+bool SkPDFImage::populate(SkPDFCatalog* catalog) {
+    if (getState() == kUnused_State) {
+        // Initializing image data for the first time.
+        if (!skip_compression(catalog)) {
+            SkAutoTUnref<SkStream> stream(getCompressedStream());
+            if (stream.get() != NULL) {
+                setData(stream.get());
+                insertName("Filter", "DCTDecode");
+                insertInt("ColorTransform", kNoColorTransform);
+                insertInt("Length", getData()->getLength());
+                setState(kCompressed_State);
+            }
+        }
+
+        // Fallback if it doesn't work.
+        if (getState() == kUnused_State) {
+            SkAutoTUnref<SkStream> stream(extract_image_data(fBitmap,
+                                                             fSrcRect));
+            setData(stream.get());
+            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 a new Catalog would want it compressed.

[vandebo (ex-Chrome), 2013/08/21 22:37:17]

...created, but the new catalog wants it compressed.

[ducky, 2013/08/22 03:48:06]

Done.

+        if (!getSubstitute()) {
+            SkPDFStream* substitute = SkNEW_ARGS(SkPDFImage, (*this));
+            setSubstitute(substitute);
+            catalog->setSubstitute(this, substitute);
+        }
+        return false;
+    }
+    return true;
+}