Revert "Revert "simplify api to bitmapcache""

Remove (for now) overly agressive assert

This reverts commit dd1b4e94c4298213f1f238a8634471086d6749fb.

Bug: skia:
Change-Id: Ibdcf0b2e92cf108ae710cd180d4fa359f81176d1
Reviewed-on: https://skia-review.googlesource.com/10129
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Reed <reed@google.com>

8 files changed, 101 insertions, 92 deletions

diff --git a/src/core/SkBitmapCache.cpp b/src/core/SkBitmapCache.cpp
index 085d0952a6..064fc46938 100644
--- a/src/core/SkBitmapCache.cpp
+++ b/src/core/SkBitmapCache.cpp
@@ -50,23 +50,45 @@ 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);
+
+    // If the dimensions are the same, should we set them to 0,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 +96,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 +146,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 +169,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 +228,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 +249,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;
     }
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index 66fc813d45..b4ec0649f2 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -63,7 +63,8 @@ SkImageInfo SkImage_Gpu::onImageInfo() const {
 }
 
 bool SkImage_Gpu::getROPixels(SkBitmap* dst, SkColorSpace* dstColorSpace, CachingHint chint) const {
-    if (SkBitmapCache::Find(this->uniqueID(), dst)) {
+    const auto desc = SkBitmapCacheDesc::Make(this);
+    if (SkBitmapCache::Find(desc, dst)) {
         SkASSERT(dst->getGenerationID() == this->uniqueID());
         SkASSERT(dst->isImmutable());
         SkASSERT(dst->getPixels());
@@ -86,7 +87,7 @@ bool SkImage_Gpu::getROPixels(SkBitmap* dst, SkColorSpace* dstColorSpace, Cachin
 
     dst->pixelRef()->setImmutableWithID(this->uniqueID());
     if (kAllow_CachingHint == chint) {
-        SkBitmapCache::Add(this->uniqueID(), *dst);
+        SkBitmapCache::Add(desc, *dst);
         fAddedRasterVersionToCache.store(true);
     }
     return true;
diff --git a/tests/ImageTest.cpp b/tests/ImageTest.cpp
index ab2215b17b..ba49000d9a 100644
--- a/tests/ImageTest.cpp
+++ b/tests/ImageTest.cpp
@@ -411,6 +411,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(c, reporter, ctxInfo) {
     SkImageInfo info = SkImageInfo::MakeN32(20, 20, kOpaque_SkAlphaType);
     sk_sp<SkImage> image(create_gpu_image(ctxInfo.grContext()));
     const uint32_t uniqueID = image->uniqueID();
+    const auto desc = SkBitmapCacheDesc::Make(image.get());
 
     auto surface(SkSurface::MakeRaster(info));
 
@@ -418,13 +419,13 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(c, reporter, ctxInfo) {
 
     {
         SkBitmap cachedBitmap;
-        REPORTER_ASSERT(reporter, !SkBitmapCache::Find(uniqueID, &cachedBitmap));
+        REPORTER_ASSERT(reporter, !SkBitmapCache::Find(desc, &cachedBitmap));
     }
 
     surface->getCanvas()->drawImage(image, 0, 0);
     {
         SkBitmap cachedBitmap;
-        if (SkBitmapCache::Find(uniqueID, &cachedBitmap)) {
+        if (SkBitmapCache::Find(desc, &cachedBitmap)) {
             REPORTER_ASSERT(reporter, cachedBitmap.getGenerationID() == uniqueID);
             REPORTER_ASSERT(reporter, cachedBitmap.isImmutable());
             REPORTER_ASSERT(reporter, cachedBitmap.getPixels());
@@ -438,7 +439,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(c, reporter, ctxInfo) {
     image.reset(nullptr);
     {
         SkBitmap cachedBitmap;
-        REPORTER_ASSERT(reporter, !SkBitmapCache::Find(uniqueID, &cachedBitmap));
+        REPORTER_ASSERT(reporter, !SkBitmapCache::Find(desc, &cachedBitmap));
     }
 }
 
diff --git a/tests/SkResourceCacheTest.cpp b/tests/SkResourceCacheTest.cpp
index 542d80efff..b260d89503 100644
--- a/tests/SkResourceCacheTest.cpp
+++ b/tests/SkResourceCacheTest.cpp
@@ -89,8 +89,8 @@ static void test_mipmap_notify(skiatest::Reporter* reporter, SkResourceCache* ca
     }
 
     for (int i = 0; i < N; ++i) {
-        const SkMipMap* mipmap = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src[i]),
-                                                           colorMode, cache);
+        const auto desc = SkBitmapCacheDesc::Make(src[i]);
+        const SkMipMap* mipmap = SkMipMapCache::FindAndRef(desc, colorMode, cache);
         if (cache) {
             // if cache is null, we're working on the global cache, and other threads might purge
             // it, making this check fragile.
@@ -100,7 +100,7 @@ static void test_mipmap_notify(skiatest::Reporter* reporter, SkResourceCache* ca
 
         src[i].reset(); // delete the underlying pixelref, which *should* remove us from the cache
 
-        mipmap = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src[i]), colorMode, cache);
+        mipmap = SkMipMapCache::FindAndRef(desc, colorMode, cache);
         REPORTER_ASSERT(reporter, !mipmap);
     }
 }
@@ -156,14 +156,14 @@ static void test_discarded_image(skiatest::Reporter* reporter, const SkMatrix& t
         canvas->concat(transform);
         canvas->drawImage(image, 0, 0, &paint);
 
-        auto imageId = image->uniqueID();
+        const auto desc = SkBitmapCacheDesc::Make(image.get());
 
         // delete the image
         image.reset(nullptr);
 
         // all resources should have been purged
         SkBitmap result;
-        REPORTER_ASSERT(reporter, !SkBitmapCache::Find(imageId, &result));
+        REPORTER_ASSERT(reporter, !SkBitmapCache::Find(desc, &result));
     }
 }