SkPDF: draw{Image,Bitmap} always serializes early

Before this change, the PDFCanon held a map from BitmapKeys
to SkImages for de-duping bitmaps.  Even if the PDFDocument
serialized images early, the Canon still held a ref to that
image inside the map.  With this change, the Canon holds a
single map from BitmapKeys to PDFObjects.  Now, Images are
only held by the PDFObject, which the document serializes
and drops early.

This change also:

-   Moves SkBitmapKey into its own header (for possible
    reuse); it now can operate with images as well as
    bitmaps.

-   Creates SkImageBitmap, which wraps a pointer to a bitmap
    or an image and abstracts out some common tasks so that
    drawBitmap and drawImage behave the same.

-   Modifies SkPDFCreateBitmapObject to take and return a
    sk_sp<T>, not a T*.

-   Refactors SkPDFDevice::internalDrawImage to use bitmaps
    or images (via a SkImageBitmap).

-   Turns on pre-serialization of all images.

BUG=skia:5087

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1829693002

Review URL: https://codereview.chromium.org/1829693002

bench/PDFBench.cpp

@@ -26,12 +26,12 @@ struct NullWStream : public SkWStream {
     size_t fN;
 };
 
-static void test_pdf_object_serialization(SkPDFObject* object) {
+static void test_pdf_object_serialization(const sk_sp<SkPDFObject> object) {
     // SkDebugWStream wStream;
     NullWStream wStream;
     SkPDFSubstituteMap substitutes;
     SkPDFObjNumMap objNumMap;
-    objNumMap.addObjectRecursively(object, substitutes);
+    objNumMap.addObjectRecursively(object.get(), substitutes);
     for (int i = 0; i < objNumMap.objects().count(); ++i) {
         SkPDFObject* object = objNumMap.objects()[i].get();
         wStream.writeDecAsText(i + 1);
@@ -67,8 +67,7 @@ protected:
             return;
         }
         while (loops-- > 0) {
-            SkAutoTUnref<SkPDFObject> object(
+            auto object = SkPDFCreateBitmapObject(fImage, nullptr);
-                    SkPDFCreateBitmapObject(fImage.get(), nullptr));
             SkASSERT(object);
             if (!object) {
                 return;
@@ -94,7 +93,7 @@ protected:
     void onDelayedSetup() override {
         sk_sp<SkImage> img(GetResourceAsImage("mandrill_512_q075.jpg"));
         if (!img) { return; }
-        SkAutoTUnref<SkData> encoded(img->refEncoded());
+        sk_sp<SkData> encoded(img->refEncoded());
         SkASSERT(encoded);
         if (!encoded) { return; }
         fImage = img;
@@ -105,8 +104,7 @@ protected:
             return;
         }
         while (loops-- > 0) {
-            SkAutoTUnref<SkPDFObject> object(
+            auto object = SkPDFCreateBitmapObject(fImage, nullptr);
-                    SkPDFCreateBitmapObject(fImage.get(), nullptr));
             SkASSERT(object);
             if (!object) {
                 return;
@@ -138,7 +136,7 @@ protected:
         SkASSERT(fAsset);
         if (!fAsset) { return; }
         while (loops-- > 0) {
-            SkAutoTUnref<SkPDFObject> object(
+            sk_sp<SkPDFObject> object(
                     new SkPDFSharedStream(fAsset->duplicate()));
             test_pdf_object_serialization(object);
         }

gyp/pdf.gypi

@@ -11,6 +11,7 @@
 #
 {
     'sources': [
+        '<(skia_src_path)/pdf/SkBitmapKey.h',
         '<(skia_src_path)/pdf/SkDeflate.cpp',
         '<(skia_src_path)/pdf/SkDeflate.h',
         '<(skia_src_path)/pdf/SkJpegInfo.cpp',

src/pdf/SkBitmapKey.h

@@ -0,0 +1,66 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+#ifndef SkBitmapKey_DEFINED
+#define SkBitmapKey_DEFINED
+
+#include "SkBitmap.h"
+#include "SkImage.h"
+#include "SkCanvas.h"
+
+class SkBitmapKey {
+public:
+    SkBitmapKey() : fSubset(SkIRect::MakeEmpty()), fID(0) {}
+    explicit SkBitmapKey(const SkBitmap& bm)
+        : fSubset(bm.getSubset()), fID(bm.getGenerationID()) {}
+    explicit SkBitmapKey(const SkImage* img)
+        : fSubset(img ? img->bounds() : SkIRect::MakeEmpty())
+        , fID(img ? img->uniqueID() : 0) {}
+    explicit SkBitmapKey(const sk_sp<SkImage> img)
+        : fSubset(img->bounds()), fID(img->uniqueID()) {}
+    bool operator==(const SkBitmapKey& rhs) const {
+        return fID == rhs.fID && fSubset == rhs.fSubset;
+    }
+    bool operator!=(const SkBitmapKey& rhs) const { return !(*this == rhs); }
+
+private:
+    SkIRect fSubset;
+    uint32_t fID;
+};
+
+/**
+   This wraps a thing that could either be a bitmap or a image and
+   abstracts out some common tasks.
+ */
+class SkImageBitmap {
+public:
+    explicit SkImageBitmap(const SkBitmap& b) : fBitmap(b), fImage(nullptr) {}
+    explicit SkImageBitmap(SkImage* i) : fImage(i) { SkASSERT(fImage); }
+    SkIRect bounds() const { return fImage ? fImage->bounds() : fBitmap.bounds(); }
+    SkISize dimensions() const {
+        return fImage ? fImage->dimensions() : fBitmap.dimensions();
+    }
+    sk_sp<SkImage> makeImage() const {
+        return fImage ? sk_ref_sp(fImage) : SkImage::MakeFromBitmap(fBitmap);
+    }
+    SkBitmapKey getKey() const {
+        return fImage ? SkBitmapKey(fImage) : SkBitmapKey(fBitmap);
+    }
+    void draw(SkCanvas* canvas, SkPaint* paint) const {
+        if (fImage) {
+            canvas->drawImage(fImage, 0, 0, paint);
+        } else {
+            canvas->drawBitmap(fBitmap, 0, 0, paint);
+        }
+    }
+
+private:
+    SkBitmap fBitmap;
+    SkImage* fImage; // non-owning; when drawImage starts passing a sk_sp<>,
+                     // we can take a const ref to that sk_sp<>.
+};
+
+#endif  // SkBitmapKey_DEFINED

src/pdf/SkPDFBitmap.cpp

@@ -382,7 +382,7 @@ namespace {
 // This SkPDFObject only outputs the alpha layer of the given bitmap.
 class PDFAlphaBitmap final : public SkPDFObject {
 public:
-    PDFAlphaBitmap(const SkImage* image) : fImage(SkRef(image)) { SkASSERT(image); }
+    PDFAlphaBitmap(sk_sp<SkImage> image) : fImage(std::move(image)) { SkASSERT(fImage); }
     void emitObject(SkWStream*  stream,
                     const SkPDFObjNumMap& objNumMap,
                     const SkPDFSubstituteMap& subs) const override {
@@ -392,7 +392,7 @@ public:
     void drop() override { fImage = nullptr; }
 
 private:
-    sk_sp<const SkImage> fImage;
+    sk_sp<SkImage> fImage;
 };
 
 }  // namespace
@@ -418,11 +418,11 @@ public:
         }
     }
     void drop() override { fImage = nullptr; fSMask = nullptr; }
-    PDFDefaultBitmap(const SkImage* image, SkPDFObject* smask)
+    PDFDefaultBitmap(sk_sp<SkImage> image, sk_sp<SkPDFObject> smask)
-        : fImage(SkRef(image)), fSMask(smask) { SkASSERT(fImage); }
+        : fImage(std::move(image)), fSMask(std::move(smask)) { SkASSERT(fImage); }
 
 private:
-    sk_sp<const SkImage> fImage;
+    sk_sp<SkImage> fImage;
     sk_sp<SkPDFObject> fSMask;
 };
 }  // namespace
@@ -474,8 +474,9 @@ void PDFJpegBitmap::emitObject(SkWStream* stream,
 
 ////////////////////////////////////////////////////////////////////////////////
 
-SkPDFObject* SkPDFCreateBitmapObject(const SkImage* image,
+sk_sp<SkPDFObject> SkPDFCreateBitmapObject(sk_sp<SkImage> image,
-                                     SkPixelSerializer* pixelSerializer) {
+                                           SkPixelSerializer* pixelSerializer) {
+    SkASSERT(image);
     sk_sp<SkData> data(image->refEncoded());
     SkJFIFInfo info;
     if (data && SkIsJFIF(data.get(), &info) &&
@@ -490,28 +491,30 @@ SkPDFObject* SkPDFCreateBitmapObject(const SkImage* image,
             #ifdef SK_PDF_IMAGE_STATS
             gJpegImageObjects.fetch_add(1);
             #endif
-            return new PDFJpegBitmap(info.fSize, data.get(), yuv);
+            return sk_make_sp<PDFJpegBitmap>(info.fSize, data.get(), yuv);
         }
     }
 
     if (pixelSerializer) {
         SkBitmap bm;
         SkAutoPixmapUnlock apu;
-        if (as_IB(image)->getROPixels(&bm) && bm.requestLock(&apu)) {
+        if (as_IB(image.get())->getROPixels(&bm) && bm.requestLock(&apu)) {
             data.reset(pixelSerializer->encode(apu.pixmap()));
             if (data && SkIsJFIF(data.get(), &info)) {
                 bool yuv = info.fType == SkJFIFInfo::kYCbCr;
                 if (info.fSize == image->dimensions()) {  // Sanity check.
-                    return new PDFJpegBitmap(info.fSize, data.get(), yuv);
+                    return sk_make_sp<PDFJpegBitmap>(info.fSize, data.get(), yuv);
                 }
             }
         }
     }
 
-    SkPDFObject* smask =
+    sk_sp<SkPDFObject> smask;
-            image_compute_is_opaque(image) ? nullptr : new PDFAlphaBitmap(image);
+    if (!image_compute_is_opaque(image.get())) {
+        smask = sk_make_sp<PDFAlphaBitmap>(image);
+    }
     #ifdef SK_PDF_IMAGE_STATS
     gRegularImageObjects.fetch_add(1);
     #endif
-    return new PDFDefaultBitmap(image, smask);
+    return sk_make_sp<PDFDefaultBitmap>(std::move(image), std::move(smask));
 }

src/pdf/SkPDFBitmap.h

@@ -16,6 +16,7 @@ class SkImage;
  * It is designed to use a minimal amout of memory, aside from refing
  * the image, and its emitObject() does not cache any data.
  */
-SkPDFObject* SkPDFCreateBitmapObject(const SkImage*, SkPixelSerializer*);
+sk_sp<SkPDFObject> SkPDFCreateBitmapObject(sk_sp<SkImage>,
+                                           SkPixelSerializer*);
 
 #endif  // SkPDFBitmap_DEFINED

src/pdf/SkPDFCanon.cpp

@@ -27,11 +27,7 @@ void SkPDFCanon::reset() {
     fGraphicStateRecords.foreach ([](WrapGS w) { w.fPtr->unref(); });
     fGraphicStateRecords.reset();
 
-    fBitmapToImageMap.foreach(
+    fPDFBitmapMap.foreach([](SkBitmapKey, SkPDFObject** p) { (*p)->unref(); });
-            [](SkBitmapKey, const SkImage** p) { SkSafeUnref(*p); });
-    fBitmapToImageMap.reset();
-
-    fPDFBitmapMap.foreach([](uint32_t, SkPDFObject** p) { SkSafeUnref(*p); });
     fPDFBitmapMap.reset();
 }
 
@@ -127,28 +123,16 @@ void SkPDFCanon::addGraphicState(const SkPDFGraphicState* state) {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-SkPDFObject* SkPDFCanon::findPDFBitmap(const SkImage* image) const {
+sk_sp<SkPDFObject> SkPDFCanon::findPDFBitmap(SkBitmapKey key) const {
-    SkPDFObject** ptr = fPDFBitmapMap.find(image->uniqueID());
+    SkPDFObject** ptr = fPDFBitmapMap.find(key);
-    return ptr ? *ptr : nullptr;
+    return ptr ? sk_ref_sp(*ptr) : sk_sp<SkPDFObject>();
 }
 
-void SkPDFCanon::addPDFBitmap(uint32_t imageUniqueID, SkPDFObject* pdfBitmap) {
+void SkPDFCanon::addPDFBitmap(SkBitmapKey key, sk_sp<SkPDFObject> pdfBitmap) {
-    fPDFBitmapMap.set(imageUniqueID, SkRef(pdfBitmap));
+    fPDFBitmapMap.set(key, pdfBitmap.release());
 }
 
-const SkImage* SkPDFCanon::bitmapToImage(const SkBitmap& bm) {
+////////////////////////////////////////////////////////////////////////////////
-    // reference remains owned by the fBitmapToImageMap!
-    SkBitmapKey key(bm);
-    if (const SkImage** img = fBitmapToImageMap.find(key)) {
-        return *img;
-    }
-    if (SkImage* image = SkImage::MakeFromBitmap(bm).release()) {
-        return *fBitmapToImageMap.set(key, image);
-    }
-    SkBitmap n32bitmap;  // SkImage::NewFromBitmap can be finicky.
-    bm.copyTo(&n32bitmap, kN32_SkColorType);
-    return *fBitmapToImageMap.set(key, SkImage::MakeFromBitmap(n32bitmap).release());
-}
 
 sk_sp<SkPDFStream> SkPDFCanon::makeInvertFunction() {
     if (fInvertFunction) {

src/pdf/SkPDFCanon.h

@@ -7,39 +7,26 @@
 #ifndef SkPDFCanon_DEFINED
 #define SkPDFCanon_DEFINED
 
-#include "SkBitmap.h"
 #include "SkPDFGraphicState.h"
 #include "SkPDFShader.h"
 #include "SkPixelSerializer.h"
 #include "SkTDArray.h"
 #include "SkTHash.h"
+#include "SkBitmapKey.h"
 
 class SkPDFFont;
 class SkPaint;
 class SkImage;
 
-class SkBitmapKey {
-public:
-    SkBitmapKey() : fSubset(SkIRect::MakeEmpty()), fGenID(0) {}
-    explicit SkBitmapKey(const SkBitmap& bm)
-        : fSubset(bm.getSubset()), fGenID(bm.getGenerationID()) {}
-    bool operator==(const SkBitmapKey& rhs) const {
-        return fGenID == rhs.fGenID && fSubset == rhs.fSubset;
-    }
-
-private:
-    SkIRect fSubset;
-    uint32_t fGenID;
-};
-
 /**
- *  The SkPDFCanon canonicalizes objects across PDF pages(SkPDFDevices).
+ *  The SkPDFCanon canonicalizes objects across PDF pages
+ *  (SkPDFDevices) and across draw calls.
  *
  *  The PDF backend works correctly if:
  *  -  There is no more than one SkPDFCanon for each thread.
  *  -  Every SkPDFDevice is given a pointer to a SkPDFCanon on creation.
  *  -  All SkPDFDevices in a document share the same SkPDFCanon.
- *  The SkDocument_PDF class makes this happen by owning a single
+ *  The SkPDFDocument class makes this happen by owning a single
  *  SkPDFCanon.
  *
  *  The addFoo() methods will ref the Foo; the canon's destructor will
@@ -75,9 +62,8 @@ public:
     const SkPDFGraphicState* findGraphicState(const SkPDFGraphicState&) const;
     void addGraphicState(const SkPDFGraphicState*);
 
-    SkPDFObject* findPDFBitmap(const SkImage* image) const;
+    sk_sp<SkPDFObject> findPDFBitmap(SkBitmapKey key) const;
-    void addPDFBitmap(uint32_t imageUniqueID, SkPDFObject*);
+    void addPDFBitmap(SkBitmapKey key, sk_sp<SkPDFObject>);
-    const SkImage* bitmapToImage(const SkBitmap&);
 
     SkTHashMap<uint32_t, bool> fCanEmbedTypeface;
 
@@ -119,8 +105,8 @@ private:
     };
     SkTHashSet<WrapGS, WrapGS::Hash> fGraphicStateRecords;
 
-    SkTHashMap<SkBitmapKey, const SkImage*> fBitmapToImageMap;
+    // TODO(halcanary): make SkTHashMap<K, sk_sp<V>> work correctly.
-    SkTHashMap<uint32_t /*ImageUniqueID*/, SkPDFObject*> fPDFBitmapMap;
+    SkTHashMap<SkBitmapKey, SkPDFObject*> fPDFBitmapMap;
 
     sk_sp<SkPixelSerializer> fPixelSerializer;
     sk_sp<SkPDFStream> fInvertFunction;

src/pdf/SkPDFDevice.cpp

@@ -1058,12 +1058,9 @@ void SkPDFDevice::drawBitmap(const SkDraw& d,
 
     SkMatrix transform = matrix;
     transform.postConcat(*d.fMatrix);
-    const SkImage* image = fDocument->canon()->bitmapToImage(bitmap);
+    SkImageBitmap imageBitmap(bitmap);
-    if (!image) {
+    this->internalDrawImage(
-        return;
+            transform, d.fClipStack, *d.fClip, imageBitmap, paint);
-    }
-    this->internalDrawImage(transform, d.fClipStack, *d.fClip, image, nullptr,
-                            paint);
 }
 
 void SkPDFDevice::drawSprite(const SkDraw& d,
@@ -1082,12 +1079,9 @@ void SkPDFDevice::drawSprite(const SkDraw& d,
 
     SkMatrix matrix;
     matrix.setTranslate(SkIntToScalar(x), SkIntToScalar(y));
-    const SkImage* image = fDocument->canon()->bitmapToImage(bitmap);
+    SkImageBitmap imageBitmap(bitmap);
-    if (!image) {
+    this->internalDrawImage(
-        return;
+            matrix, d.fClipStack, *d.fClip, imageBitmap, paint);
-    }
-    this->internalDrawImage(matrix, d.fClipStack, *d.fClip, image, nullptr,
-                            paint);
 }
 
 void SkPDFDevice::drawImage(const SkDraw& draw,
@@ -1107,8 +1101,9 @@ void SkPDFDevice::drawImage(const SkDraw& draw,
     }
     SkMatrix transform = SkMatrix::MakeTrans(x, y);
     transform.postConcat(*draw.fMatrix);
-    this->internalDrawImage(transform, draw.fClipStack, *draw.fClip, image,
+    SkImageBitmap imageBitmap(const_cast<SkImage*>(image));
-                            nullptr, paint);
+    this->internalDrawImage(
+            transform, draw.fClipStack, *draw.fClip, imageBitmap, paint);
 }
 
 void SkPDFDevice::drawImageRect(const SkDraw& draw,
@@ -2112,43 +2107,37 @@ static SkSize rect_to_size(const SkRect& r) {
     return SkSize::Make(r.width(), r.height());
 }
 
-static const SkImage* color_filter(const SkImage* image, SkColorFilter* colorFilter) {
+static sk_sp<SkImage> color_filter(const SkImageBitmap& imageBitmap,
-    auto surface(SkSurface::MakeRaster(SkImageInfo::MakeN32Premul(image->dimensions())));
+                                   SkColorFilter* colorFilter) {
-    if (!surface) {
+    auto surface = 
-        return image;
+        SkSurface::MakeRaster(SkImageInfo::MakeN32Premul(imageBitmap.dimensions()));
-    }
+    SkASSERT(surface);
     SkCanvas* canvas = surface->getCanvas();
     canvas->clear(SK_ColorTRANSPARENT);
     SkPaint paint;
     paint.setColorFilter(sk_ref_sp(colorFilter));
-    canvas->drawImage(image, 0, 0, &paint);
+    imageBitmap.draw(canvas, &paint);
     canvas->flush();
-    return surface->makeImageSnapshot().release();
+    return surface->makeImageSnapshot();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 void SkPDFDevice::internalDrawImage(const SkMatrix& origMatrix,
                                     const SkClipStack* clipStack,
                                     const SkRegion& origClipRegion,
-                                    const SkImage* image,
+                                    SkImageBitmap imageBitmap,
-                                    const SkIRect* srcRect,
                                     const SkPaint& paint) {
-    SkASSERT(image);
+    if (imageBitmap.dimensions().isZero()) {
+        return;
+    }
     #ifdef SK_PDF_IMAGE_STATS
     gDrawImageCalls.fetch_add(1);
     #endif
     SkMatrix matrix = origMatrix;
     SkRegion perspectiveBounds;
     const SkRegion* clipRegion = &origClipRegion;
-    sk_sp<const SkImage> autoImageUnref;
+    sk_sp<SkImage> autoImageUnref;
 
-    if (srcRect) {
-        autoImageUnref = image->makeSubset(*srcRect);
-        if (!autoImageUnref) {
-            return;
-        }
-        image = autoImageUnref.get();
-    }
     // Rasterize the bitmap using perspective in a new bitmap.
     if (origMatrix.hasPerspective()) {
         if (fRasterDpi == 0) {
@@ -2157,7 +2146,7 @@ void SkPDFDevice::internalDrawImage(const SkMatrix& origMatrix,
         // Transform the bitmap in the new space, without taking into
         // account the initial transform.
         SkPath perspectiveOutline;
-        SkRect imageBounds = SkRect::Make(image->bounds());
+        SkRect imageBounds = SkRect::Make(imageBitmap.bounds());
         perspectiveOutline.addRect(imageBounds);
         perspectiveOutline.transform(origMatrix);
 
@@ -2208,7 +2197,7 @@ void SkPDFDevice::internalDrawImage(const SkMatrix& origMatrix,
         // Translate the draw in the new canvas, so we perfectly fit the
         // shape in the bitmap.
         canvas->setMatrix(offsetMatrix);
-        canvas->drawImage(image, 0, 0, nullptr);
+        imageBitmap.draw(canvas, nullptr);
         // Make sure the final bits are in the bitmap.
         canvas->flush();
 
@@ -2219,10 +2208,9 @@ void SkPDFDevice::internalDrawImage(const SkMatrix& origMatrix,
 
         perspectiveBounds.setRect(bounds.roundOut());
         clipRegion = &perspectiveBounds;
-        srcRect = nullptr;
 
-        autoImageUnref.reset(surface->makeImageSnapshot().release());
+        autoImageUnref = surface->makeImageSnapshot();
-        image = autoImageUnref.get();
+        imageBitmap = SkImageBitmap(autoImageUnref.get());
     }
 
     SkMatrix scaled;
@@ -2230,12 +2218,12 @@ void SkPDFDevice::internalDrawImage(const SkMatrix& origMatrix,
     scaled.setScale(SK_Scalar1, -SK_Scalar1);
     scaled.postTranslate(0, SK_Scalar1);
     // Scale the image up from 1x1 to WxH.
-    SkIRect subset = image->bounds();
+    SkIRect subset = imageBitmap.bounds();
-    scaled.postScale(SkIntToScalar(image->width()),
+    scaled.postScale(SkIntToScalar(imageBitmap.dimensions().width()),
-                     SkIntToScalar(image->height()));
+                     SkIntToScalar(imageBitmap.dimensions().height()));
     scaled.postConcat(matrix);
     ScopedContentEntry content(this, clipStack, *clipRegion, scaled, paint);
-    if (!content.entry() || (srcRect && !subset.intersect(*srcRect))) {
+    if (!content.entry()) {
         return;
     }
     if (content.needShape()) {
@@ -2254,26 +2242,28 @@ void SkPDFDevice::internalDrawImage(const SkMatrix& origMatrix,
         // drawBitmap*()/drawImage*() calls amd ImageFilters (which
         // rasterize a layer on this backend).  Fortuanely, this seems
         // to be how Chromium impements most color-filters.
-        autoImageUnref.reset(color_filter(image, colorFilter));
+        autoImageUnref = color_filter(imageBitmap, colorFilter);
-        image = autoImageUnref.get();
+        imageBitmap = SkImageBitmap(autoImageUnref.get());
         // TODO(halcanary): de-dupe this by caching filtered images.
         // (maybe in the resource cache?)
     }
-    sk_sp<SkPDFObject> pdfimage(SkSafeRef(fDocument->canon()->findPDFBitmap(image)));
+
+    SkBitmapKey key = imageBitmap.getKey();
+    sk_sp<SkPDFObject> pdfimage = fDocument->canon()->findPDFBitmap(key);
     if (!pdfimage) {
-        pdfimage.reset(SkPDFCreateBitmapObject(
+        auto img = imageBitmap.makeImage();
-                               image, fDocument->canon()->getPixelSerializer()));
+        if (!img) {
+            return;
+        }
+        pdfimage = SkPDFCreateBitmapObject(
+                std::move(img), fDocument->canon()->getPixelSerializer());
         if (!pdfimage) {
             return;
         }
-        #if SK_PDF_SERIALIZE_IMAGES_EARLY // TODO(halcanary): enable.
+        fDocument->serialize(pdfimage);  // serialize images early.
-        sk_sp<SkData> encodedImage(image->refEncodedData());
+        fDocument->canon()->addPDFBitmap(key, pdfimage);
-        if (!encodedImage) {
-            fDocument->serialize(pdfimage);
-        }
-        #endif
-        fDocument->canon()->addPDFBitmap(image->uniqueID(), pdfimage.get());
     }
+    // TODO(halcanary): addXObjectResource() should take a sk_sp<SkPDFObject>
     SkPDFUtils::DrawFormXObject(this->addXObjectResource(pdfimage.get()),
                                 &content.entry()->fContent);
 }

src/pdf/SkPDFDevice.h

@@ -9,6 +9,7 @@
 #define SkPDFDevice_DEFINED
 
 #include "SkBitmap.h"
+#include "SkBitmapKey.h"
 #include "SkCanvas.h"
 #include "SkClipStack.h"
 #include "SkData.h"
@@ -317,11 +318,11 @@ private:
     int getFontResourceIndex(SkTypeface* typeface, uint16_t glyphID);
 
     void internalDrawPaint(const SkPaint& paint, ContentEntry* contentEntry);
-    void internalDrawImage(const SkMatrix& matrix,
+
+    void internalDrawImage(const SkMatrix& origMatrix,
                            const SkClipStack* clipStack,
-                           const SkRegion& clipRegion,
+                           const SkRegion& origClipRegion,
-                           const SkImage* image,
+                           SkImageBitmap imageBitmap,
-                           const SkIRect* srcRect,
                            const SkPaint& paint);
 
     /** Helper method for copyContentToData. It is responsible for copying the

src/pdf/SkPDFDocument.cpp

@@ -14,11 +14,16 @@
 #include "SkPDFUtils.h"
 #include "SkStream.h"
 
-
 SkPDFObjectSerializer::SkPDFObjectSerializer() : fBaseOffset(0), fNextToBeSerialized(0) {}
 
 template <class T> static void renew(T* t) { t->~T(); new (t) T; }
 
+SkPDFObjectSerializer::~SkPDFObjectSerializer() {
+    for (int i = 0; i < fObjNumMap.objects().count(); ++i) {
+        fObjNumMap.objects()[i]->drop();
+    }
+}
+
 void SkPDFObjectSerializer::addObjectRecursively(const sk_sp<SkPDFObject>& object) {
     fObjNumMap.addObjectRecursively(object.get(), fSubstituteMap);
 }

src/pdf/SkPDFDocument.h

@@ -31,6 +31,7 @@ struct SkPDFObjectSerializer : SkNoncopyable {
     int32_t fNextToBeSerialized;  // index in fObjNumMap
 
     SkPDFObjectSerializer();
+    ~SkPDFObjectSerializer();
     void addObjectRecursively(const sk_sp<SkPDFObject>&);
     void serializeHeader(SkWStream*, const SkPDFMetadata&);
     void serializeObjects(SkWStream*);