From 5fa3d6d4b15fd6417de877561f5e4bca422e383c Mon Sep 17 00:00:00 2001 From: Mike Reed Date: Sat, 25 Mar 2017 09:51:00 -0400 Subject: [PATCH] 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 Commit-Queue: Mike Reed --- src/core/SkBitmapCache.cpp | 116 +++++++++++++++----------------- src/core/SkBitmapCache.h | 37 ++++++---- src/core/SkBitmapController.cpp | 4 +- src/core/SkImageCacherator.cpp | 9 ++- src/core/SkSpecialImage.cpp | 5 +- src/image/SkImage_Gpu.cpp | 5 +- tests/ImageTest.cpp | 7 +- tests/SkResourceCacheTest.cpp | 10 +-- 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(colorMode)), fBounds(bounds) + MipMapKey(uint32_t imageID, const SkIRect& subset, SkDestinationSurfaceColorMode colorMode) + : fImageID(imageID) + , fColorMode(static_cast(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 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)); } }