simplify api to bitmapcache

Force all Find callers to make a bitmpacachedesc, which now
has more rigid validation.

Goal is to ensure we never make two desc (which turn into keys)
that look different but represent the same image/transformation.

BUG=skia:

Change-Id: I8571837ee4754a69acc99e949bee9a465fa25f2b
Reviewed-on: https://skia-review.googlesource.com/10114
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

5 files changed, 87 insertions, 82 deletions

diff --git a/src/core/SkBitmapCache.cpp b/src/core/SkBitmapCache.cpp
index 085d0952a6..41818d5895 100644
--- a/src/core/SkBitmapCache.cpp
+++ b/src/core/SkBitmapCache.cpp
@@ -50,23 +50,43 @@ static SkIRect get_bounds_from_bitmap(const SkBitmap& bm) {
  *  return that subset (see get_bounds_from_bitmap).
  */
 static SkIRect get_bounds_from_image(const SkImage* image) {
+    SkASSERT(image->width() > 0 && image->height() > 0);
     return SkIRect::MakeWH(image->width(), image->height());
 }
 
-SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkBitmap& bm, int width, int height) {
-    return { bm.getGenerationID(), width, height, get_bounds_from_bitmap(bm) };
+SkBitmapCacheDesc SkBitmapCacheDesc::Make(uint32_t imageID, int origWidth, int origHeight) {
+    SkASSERT(imageID);
+    SkASSERT(origWidth > 0 && origHeight > 0);
+    return { imageID, 0, 0, {0, 0, origWidth, origHeight} };
+}
+
+SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkBitmap& bm, int scaledWidth, int scaledHeight) {
+    SkASSERT(bm.width() > 0 && bm.height() > 0);
+    SkASSERT(scaledWidth > 0 && scaledHeight > 0);
+    SkASSERT(scaledWidth != bm.width() || scaledHeight != bm.height());
+
+    return { bm.getGenerationID(), scaledWidth, scaledHeight, get_bounds_from_bitmap(bm) };
 }
 
 SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkBitmap& bm) {
-    return Make(bm, bm.width(), bm.height());
+    SkASSERT(bm.width() > 0 && bm.height() > 0);
+    SkASSERT(bm.pixelRefOrigin() == SkIPoint::Make(0, 0));
+
+    return { bm.getGenerationID(), 0, 0, get_bounds_from_bitmap(bm) };
 }
 
-SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkImage* image, int width, int height) {
-    return { image->uniqueID(), width, height, get_bounds_from_image(image) };
+SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkImage* image, int scaledWidth, int scaledHeight) {
+    SkASSERT(image->width() > 0 && image->height() > 0);
+    SkASSERT(scaledWidth > 0 && scaledHeight > 0);
+    SkASSERT(scaledWidth != image->width() || scaledHeight != image->height());
+
+    return { image->uniqueID(), scaledWidth, scaledHeight, get_bounds_from_image(image) };
 }
 
 SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkImage* image) {
-    return Make(image, image->width(), image->height());
+    SkASSERT(image->width() > 0 && image->height() > 0);
+
+    return { image->uniqueID(), 0, 0, get_bounds_from_image(image) };
 }
 
 namespace {
@@ -74,36 +94,21 @@ static unsigned gBitmapKeyNamespaceLabel;
 
 struct BitmapKey : public SkResourceCache::Key {
 public:
-    BitmapKey(uint32_t genID, int width, int height, const SkIRect& bounds)
-        : fDesc({ genID, width, height, bounds })
-    {
-        this->init(&gBitmapKeyNamespaceLabel, SkMakeResourceCacheSharedIDForBitmap(fDesc.fImageID),
-                   sizeof(fDesc));
-    }
-
     BitmapKey(const SkBitmapCacheDesc& desc) : fDesc(desc) {
         this->init(&gBitmapKeyNamespaceLabel, SkMakeResourceCacheSharedIDForBitmap(fDesc.fImageID),
                    sizeof(fDesc));
     }
 
     void dump() const {
-        SkDebugf("-- add [%d %d] %d [%d %d %d %d]\n", fDesc.fWidth, fDesc.fHeight, fDesc.fImageID,
-            fDesc.fBounds.x(), fDesc.fBounds.y(), fDesc.fBounds.width(), fDesc.fBounds.height());
+        SkDebugf("-- add [%d %d] %d [%d %d %d %d]\n",
+                 fDesc.fScaledWidth, fDesc.fScaledHeight, fDesc.fImageID,
+             fDesc.fSubset.x(), fDesc.fSubset.y(), fDesc.fSubset.width(), fDesc.fSubset.height());
     }
 
     const SkBitmapCacheDesc fDesc;
 };
 
 struct BitmapRec : public SkResourceCache::Rec {
-    BitmapRec(uint32_t genID, int width, int height, const SkIRect& bounds, const SkBitmap& result)
-        : fKey(genID, width, height, bounds)
-        , fBitmap(result)
-    {
-#ifdef TRACE_NEW_BITMAP_CACHE_RECS
-        fKey.dump();
-#endif
-    }
-
     BitmapRec(const SkBitmapCacheDesc& desc, const SkBitmap& result)
         : fKey(desc)
         , fBitmap(result)
@@ -139,41 +144,21 @@ private:
 #define CHECK_LOCAL(localCache, localName, globalName, ...) \
     ((localCache) ? localCache->localName(__VA_ARGS__) : SkResourceCache::globalName(__VA_ARGS__))
 
-bool SkBitmapCache::FindWH(const SkBitmapCacheDesc& desc, SkBitmap* result,
-                           SkResourceCache* localCache) {
-    if (0 == desc.fWidth || 0 == desc.fHeight) {
-        // degenerate
-        return false;
-    }
+bool SkBitmapCache::Find(const SkBitmapCacheDesc& desc, SkBitmap* result,
+                         SkResourceCache* localCache) {
+    desc.validate();
     return CHECK_LOCAL(localCache, find, Find, BitmapKey(desc), BitmapRec::Finder, result);
 }
 
-bool SkBitmapCache::AddWH(const SkBitmapCacheDesc& desc, const SkBitmap& result,
-                          SkResourceCache* localCache) {
-    if (0 == desc.fWidth || 0 == desc.fHeight) {
-        // degenerate, and the key we use for mipmaps
-        return false;
-    }
+bool SkBitmapCache::Add(const SkBitmapCacheDesc& desc, const SkBitmap& result,
+                        SkResourceCache* localCache) {
+    desc.validate();
     SkASSERT(result.isImmutable());
     BitmapRec* rec = new BitmapRec(desc, result);
     CHECK_LOCAL(localCache, add, Add, rec);
     return true;
 }
 
-bool SkBitmapCache::Find(uint32_t genID, SkBitmap* result, SkResourceCache* localCache) {
-    BitmapKey key(genID, SK_Scalar1, SK_Scalar1, SkIRect::MakeEmpty());
-
-    return CHECK_LOCAL(localCache, find, Find, key, BitmapRec::Finder, result);
-}
-
-void SkBitmapCache::Add(uint32_t genID, const SkBitmap& result, SkResourceCache* localCache) {
-    SkASSERT(result.isImmutable());
-
-    BitmapRec* rec = new BitmapRec(genID, 1, 1, SkIRect::MakeEmpty(), result);
-
-    CHECK_LOCAL(localCache, add, Add, rec);
-}
-
 //////////////////////////////////////////////////////////////////////////////////////////
 //////////////////////////////////////////////////////////////////////////////////////////
 
@@ -182,21 +167,26 @@ static unsigned gMipMapKeyNamespaceLabel;
 
 struct MipMapKey : public SkResourceCache::Key {
 public:
-    MipMapKey(uint32_t genID, SkDestinationSurfaceColorMode colorMode, const SkIRect& bounds)
-        : fGenID(genID), fColorMode(static_cast<uint32_t>(colorMode)), fBounds(bounds)
+    MipMapKey(uint32_t imageID, const SkIRect& subset, SkDestinationSurfaceColorMode colorMode)
+        : fImageID(imageID)
+        , fColorMode(static_cast<uint32_t>(colorMode))
+        , fSubset(subset)
     {
-        this->init(&gMipMapKeyNamespaceLabel, SkMakeResourceCacheSharedIDForBitmap(genID),
-                   sizeof(fGenID) + sizeof(fColorMode) + sizeof(fBounds));
+        SkASSERT(fImageID);
+        SkASSERT(!subset.isEmpty());
+        this->init(&gMipMapKeyNamespaceLabel, SkMakeResourceCacheSharedIDForBitmap(fImageID),
+                   sizeof(fImageID) + sizeof(fColorMode) + sizeof(fSubset));
     }
 
-    uint32_t    fGenID;
+    uint32_t    fImageID;
     uint32_t    fColorMode;
-    SkIRect     fBounds;
+    SkIRect     fSubset;
 };
 
 struct MipMapRec : public SkResourceCache::Rec {
-    MipMapRec(const SkBitmap& src, SkDestinationSurfaceColorMode colorMode, const SkMipMap* result)
-        : fKey(src.getGenerationID(), colorMode, get_bounds_from_bitmap(src))
+    MipMapRec(uint32_t imageID, const SkIRect& subset, SkDestinationSurfaceColorMode colorMode,
+              const SkMipMap* result)
+        : fKey(imageID, subset, colorMode)
         , fMipMap(result)
     {
         fMipMap->attachToCacheAndRef();
@@ -236,8 +226,9 @@ private:
 const SkMipMap* SkMipMapCache::FindAndRef(const SkBitmapCacheDesc& desc,
                                           SkDestinationSurfaceColorMode colorMode,
                                           SkResourceCache* localCache) {
-    // Note: we ignore width/height from desc, just need id and bounds
-    MipMapKey key(desc.fImageID, colorMode, desc.fBounds);
+    SkASSERT(desc.fScaledWidth == 0);
+    SkASSERT(desc.fScaledHeight == 0);
+    MipMapKey key(desc.fImageID, desc.fSubset, colorMode);
     const SkMipMap* result;
 
     if (!CHECK_LOCAL(localCache, find, Find, key, MipMapRec::Finder, &result)) {
@@ -256,7 +247,8 @@ const SkMipMap* SkMipMapCache::AddAndRef(const SkBitmap& src,
                                          SkResourceCache* localCache) {
     SkMipMap* mipmap = SkMipMap::Build(src, colorMode, get_fact(localCache));
     if (mipmap) {
-        MipMapRec* rec = new MipMapRec(src, colorMode, mipmap);
+        MipMapRec* rec = new MipMapRec(src.getGenerationID(), get_bounds_from_bitmap(src),
+                                       colorMode, mipmap);
         CHECK_LOCAL(localCache, add, Add, rec);
         src.pixelRef()->notifyAddedToCache();
     }
diff --git a/src/core/SkBitmapCache.h b/src/core/SkBitmapCache.h
index e5160ae029..907a5468b6 100644
--- a/src/core/SkBitmapCache.h
+++ b/src/core/SkBitmapCache.h
@@ -19,15 +19,27 @@ uint64_t SkMakeResourceCacheSharedIDForBitmap(uint32_t bitmapGenID);
 void SkNotifyBitmapGenIDIsStale(uint32_t bitmapGenID);
 
 struct SkBitmapCacheDesc {
-    uint32_t    fImageID;
-    int32_t     fWidth;
-    int32_t     fHeight;
-    SkIRect     fBounds;
+    uint32_t    fImageID;       // != 0
+    int32_t     fScaledWidth;   // 0 for unscaled
+    int32_t     fScaledHeight;  // 0 for unscaled
+    SkIRect     fSubset;        // always set to a valid rect (entire or subset)
 
-    static SkBitmapCacheDesc Make(const SkBitmap&, int width, int height);
+    void validate() const {
+        SkASSERT(fImageID);
+        if (fScaledWidth || fScaledHeight) {
+            SkASSERT(fScaledWidth && fScaledHeight);
+        }
+        SkASSERT(fSubset.fLeft >= 0 && fSubset.fTop >= 0);
+        SkASSERT(fSubset.width() > 0 && fSubset.height() > 0);
+    }
+
+    static SkBitmapCacheDesc Make(const SkBitmap&, int scaledWidth, int scaledHeight);
     static SkBitmapCacheDesc Make(const SkBitmap&);
-    static SkBitmapCacheDesc Make(const SkImage*, int width, int height);
+    static SkBitmapCacheDesc Make(const SkImage*, int scaledWidth, int scaledHeight);
     static SkBitmapCacheDesc Make(const SkImage*);
+
+    // Use with care -- width/height must match the original bitmap/image
+    static SkBitmapCacheDesc Make(uint32_t genID, int origWidth, int origHeight);
 };
 
 class SkBitmapCache {
@@ -42,22 +54,19 @@ public:
      *  Search based on the desc. If found, returns true and
      *  result will be set to the matching bitmap with its pixels already locked.
      */
-    static bool FindWH(const SkBitmapCacheDesc&, SkBitmap* result,
-                       SkResourceCache* localCache = nullptr);
+    static bool Find(const SkBitmapCacheDesc&, SkBitmap* result,
+                    SkResourceCache* localCache = nullptr);
 
     /*
      *  result must be marked isImmutable()
      */
-    static bool AddWH(const SkBitmapCacheDesc&, const SkBitmap& result,
-                      SkResourceCache* localCache = nullptr);
-
-    static bool Find(uint32_t genID, SkBitmap* result, SkResourceCache* localCache = nullptr);
-    // todo: eliminate the need to specify ID, since it should == the bitmap's
-    static void Add(uint32_t genID, const SkBitmap&, SkResourceCache* localCache = nullptr);
+    static bool Add(const SkBitmapCacheDesc&, const SkBitmap& result,
+                    SkResourceCache* localCache = nullptr);
 };
 
 class SkMipMapCache {
 public:
+    // Note: the scaled width/height in desc must be 0, as any other value would not make sense.
     static const SkMipMap* FindAndRef(const SkBitmapCacheDesc&, SkDestinationSurfaceColorMode,
                                       SkResourceCache* localCache = nullptr);
     static const SkMipMap* AddAndRef(const SkBitmap& src, SkDestinationSurfaceColorMode,
diff --git a/src/core/SkBitmapController.cpp b/src/core/SkBitmapController.cpp
index 8bec804243..7ef0a99cb6 100644
--- a/src/core/SkBitmapController.cpp
+++ b/src/core/SkBitmapController.cpp
@@ -124,7 +124,7 @@ bool SkDefaultBitmapControllerState::processHQRequest(const SkBitmapProvider& pr
     const int dstH = SkScalarRoundToScalar(provider.height() / invScaleY);
     const SkBitmapCacheDesc desc = provider.makeCacheDesc(dstW, dstH);
 
-    if (!SkBitmapCache::FindWH(desc, &fResultBitmap)) {
+    if (!SkBitmapCache::Find(desc, &fResultBitmap)) {
         SkBitmap orig;
         if (!provider.asBitmap(&orig)) {
             return false;
@@ -141,7 +141,7 @@ bool SkDefaultBitmapControllerState::processHQRequest(const SkBitmapProvider& pr
         SkASSERT(fResultBitmap.getPixels());
         fResultBitmap.setImmutable();
         if (!provider.isVolatile()) {
-            if (SkBitmapCache::AddWH(desc, fResultBitmap)) {
+            if (SkBitmapCache::Add(desc, fResultBitmap)) {
                 provider.notifyAddedToCache();
             }
         }
diff --git a/src/core/SkImageCacherator.cpp b/src/core/SkImageCacherator.cpp
index f0bc9973a6..06d2279946 100644
--- a/src/core/SkImageCacherator.cpp
+++ b/src/core/SkImageCacherator.cpp
@@ -180,7 +180,8 @@ bool SkImageCacherator::directGeneratePixels(const SkImageInfo& info, void* pixe
 
 bool SkImageCacherator::lockAsBitmapOnlyIfAlreadyCached(SkBitmap* bitmap, CachedFormat format) {
     return kNeedNewImageUniqueID != fUniqueIDs[format] &&
-        SkBitmapCache::Find(fUniqueIDs[format], bitmap) &&
+        SkBitmapCache::Find(SkBitmapCacheDesc::Make(fUniqueIDs[format],
+                                                    fInfo.width(), fInfo.height()), bitmap) &&
         check_output_bitmap(*bitmap, fUniqueIDs[format]);
 }
 
@@ -199,7 +200,8 @@ bool SkImageCacherator::tryLockAsBitmap(SkBitmap* bitmap, const SkImage* client,
     }
     bitmap->pixelRef()->setImmutableWithID(fUniqueIDs[format]);
     if (SkImage::kAllow_CachingHint == chint) {
-        SkBitmapCache::Add(fUniqueIDs[format], *bitmap);
+        SkBitmapCache::Add(SkBitmapCacheDesc::Make(fUniqueIDs[format],
+                                                   fInfo.width(), fInfo.height()), *bitmap);
         if (client) {
             as_IB(client)->notifyAddedToCache();
         }
@@ -259,7 +261,8 @@ bool SkImageCacherator::lockAsBitmap(GrContext* context, SkBitmap* bitmap, const
 
     bitmap->pixelRef()->setImmutableWithID(fUniqueIDs[format]);
     if (SkImage::kAllow_CachingHint == chint) {
-        SkBitmapCache::Add(fUniqueIDs[format], *bitmap);
+        SkBitmapCache::Add(SkBitmapCacheDesc::Make(fUniqueIDs[format],
+                                                   fInfo.width(), fInfo.height()), *bitmap);
         if (client) {
             as_IB(client)->notifyAddedToCache();
         }
diff --git a/src/core/SkSpecialImage.cpp b/src/core/SkSpecialImage.cpp
index c0b50634fc..339341a8e8 100644
--- a/src/core/SkSpecialImage.cpp
+++ b/src/core/SkSpecialImage.cpp
@@ -401,7 +401,8 @@ public:
     }
 
     bool onGetROPixels(SkBitmap* dst) const override {
-        if (SkBitmapCache::Find(this->uniqueID(), dst)) {
+        const auto desc = SkBitmapCacheDesc::Make(this->uniqueID(), this->width(), this->height());
+        if (SkBitmapCache::Find(desc, dst)) {
             SkASSERT(dst->getGenerationID() == this->uniqueID());
             SkASSERT(dst->isImmutable());
             SkASSERT(dst->getPixels());
@@ -427,7 +428,7 @@ public:
         }
 
         dst->pixelRef()->setImmutableWithID(this->uniqueID());
-        SkBitmapCache::Add(this->uniqueID(), *dst);
+        SkBitmapCache::Add(desc, *dst);
         fAddedRasterVersionToCache.store(true);
         return true;
     }