diff options
author | Florin Malita <fmalita@chromium.org> | 2017-03-24 22:09:30 +0000 |
---|---|---|
committer | Skia Commit-Bot <skia-commit-bot@chromium.org> | 2017-03-24 22:09:40 +0000 |
commit | dd1b4e94c4298213f1f238a8634471086d6749fb (patch) | |
tree | 6da125f682710e984ec99bea88073a465f8c9a9a | |
parent | 51fe9710465d01823f3bd9ad856f6460a98db130 (diff) |
Revert "simplify api to bitmapcache"
This reverts commit 9f4b0ae91e1875831cc11f0629b5db998ad85438.
Reason for revert: Nanobench asserts.
../../../src/core/SkBitmapCache.cpp:81: fatal error: "assert(scaledWidth != image->width() || scaledHeight != image->height())"
Aborted
Command exited with code 134
step returned non-zero exit code: 134
https://chromium-swarm.appspot.com/task?id=351b1d10c7936310&refresh=10
Original change's description:
> 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>
>
TBR=brianosman@google.com,fmalita@chromium.org,reed@google.com,reviews@skia.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Change-Id: I21b3c8a5bae409ba740cfc28b352c3b970dcf5af
Reviewed-on: https://skia-review.googlesource.com/10171
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>
-rw-r--r-- | src/core/SkBitmapCache.cpp | 114 | ||||
-rw-r--r-- | src/core/SkBitmapCache.h | 37 | ||||
-rw-r--r-- | src/core/SkBitmapController.cpp | 4 | ||||
-rw-r--r-- | src/core/SkImageCacherator.cpp | 9 | ||||
-rw-r--r-- | src/core/SkSpecialImage.cpp | 5 | ||||
-rw-r--r-- | src/image/SkImage_Gpu.cpp | 5 | ||||
-rw-r--r-- | tests/ImageTest.cpp | 7 | ||||
-rw-r--r-- | tests/SkResourceCacheTest.cpp | 10 |
8 files changed, 92 insertions, 99 deletions
diff --git a/src/core/SkBitmapCache.cpp b/src/core/SkBitmapCache.cpp index 41818d5895..085d0952a6 100644 --- a/src/core/SkBitmapCache.cpp +++ b/src/core/SkBitmapCache.cpp @@ -50,43 +50,23 @@ 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(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, int width, int height) { + return { bm.getGenerationID(), width, height, get_bounds_from_bitmap(bm) }; } SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkBitmap& bm) { - SkASSERT(bm.width() > 0 && bm.height() > 0); - SkASSERT(bm.pixelRefOrigin() == SkIPoint::Make(0, 0)); - - return { bm.getGenerationID(), 0, 0, get_bounds_from_bitmap(bm) }; + return Make(bm, bm.width(), bm.height()); } -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, int width, int height) { + return { image->uniqueID(), width, height, get_bounds_from_image(image) }; } SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkImage* image) { - SkASSERT(image->width() > 0 && image->height() > 0); - - return { image->uniqueID(), 0, 0, get_bounds_from_image(image) }; + return Make(image, image->width(), image->height()); } namespace { @@ -94,21 +74,36 @@ 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.fScaledWidth, fDesc.fScaledHeight, fDesc.fImageID, - fDesc.fSubset.x(), fDesc.fSubset.y(), fDesc.fSubset.width(), fDesc.fSubset.height()); + 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()); } 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) @@ -144,21 +139,41 @@ private: #define CHECK_LOCAL(localCache, localName, globalName, ...) \ ((localCache) ? localCache->localName(__VA_ARGS__) : SkResourceCache::globalName(__VA_ARGS__)) -bool SkBitmapCache::Find(const SkBitmapCacheDesc& desc, SkBitmap* result, - SkResourceCache* localCache) { - desc.validate(); +bool SkBitmapCache::FindWH(const SkBitmapCacheDesc& desc, SkBitmap* result, + SkResourceCache* localCache) { + if (0 == desc.fWidth || 0 == desc.fHeight) { + // degenerate + return false; + } return CHECK_LOCAL(localCache, find, Find, BitmapKey(desc), BitmapRec::Finder, result); } -bool SkBitmapCache::Add(const SkBitmapCacheDesc& desc, const SkBitmap& result, - SkResourceCache* localCache) { - desc.validate(); +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; + } 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); +} + ////////////////////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////////////////////// @@ -167,26 +182,21 @@ static unsigned gMipMapKeyNamespaceLabel; struct MipMapKey : public SkResourceCache::Key { public: - MipMapKey(uint32_t imageID, const SkIRect& subset, SkDestinationSurfaceColorMode colorMode) - : fImageID(imageID) - , fColorMode(static_cast<uint32_t>(colorMode)) - , fSubset(subset) + MipMapKey(uint32_t genID, SkDestinationSurfaceColorMode colorMode, const SkIRect& bounds) + : fGenID(genID), fColorMode(static_cast<uint32_t>(colorMode)), fBounds(bounds) { - SkASSERT(fImageID); - SkASSERT(!subset.isEmpty()); - this->init(&gMipMapKeyNamespaceLabel, SkMakeResourceCacheSharedIDForBitmap(fImageID), - sizeof(fImageID) + sizeof(fColorMode) + sizeof(fSubset)); + this->init(&gMipMapKeyNamespaceLabel, SkMakeResourceCacheSharedIDForBitmap(genID), + sizeof(fGenID) + sizeof(fColorMode) + sizeof(fBounds)); } - uint32_t fImageID; + uint32_t fGenID; uint32_t fColorMode; - SkIRect fSubset; + SkIRect fBounds; }; struct MipMapRec : public SkResourceCache::Rec { - MipMapRec(uint32_t imageID, const SkIRect& subset, SkDestinationSurfaceColorMode colorMode, - const SkMipMap* result) - : fKey(imageID, subset, colorMode) + MipMapRec(const SkBitmap& src, SkDestinationSurfaceColorMode colorMode, const SkMipMap* result) + : fKey(src.getGenerationID(), colorMode, get_bounds_from_bitmap(src)) , fMipMap(result) { fMipMap->attachToCacheAndRef(); @@ -226,9 +236,8 @@ private: const SkMipMap* SkMipMapCache::FindAndRef(const SkBitmapCacheDesc& desc, SkDestinationSurfaceColorMode colorMode, SkResourceCache* localCache) { - SkASSERT(desc.fScaledWidth == 0); - SkASSERT(desc.fScaledHeight == 0); - MipMapKey key(desc.fImageID, desc.fSubset, colorMode); + // Note: we ignore width/height from desc, just need id and bounds + MipMapKey key(desc.fImageID, colorMode, desc.fBounds); const SkMipMap* result; if (!CHECK_LOCAL(localCache, find, Find, key, MipMapRec::Finder, &result)) { @@ -247,8 +256,7 @@ 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.getGenerationID(), get_bounds_from_bitmap(src), - colorMode, mipmap); + MipMapRec* rec = new MipMapRec(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 907a5468b6..e5160ae029 100644 --- a/src/core/SkBitmapCache.h +++ b/src/core/SkBitmapCache.h @@ -19,27 +19,15 @@ uint64_t SkMakeResourceCacheSharedIDForBitmap(uint32_t bitmapGenID); void SkNotifyBitmapGenIDIsStale(uint32_t bitmapGenID); struct SkBitmapCacheDesc { - 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) + uint32_t fImageID; + int32_t fWidth; + int32_t fHeight; + SkIRect fBounds; - 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&, int width, int height); static SkBitmapCacheDesc Make(const SkBitmap&); - static SkBitmapCacheDesc Make(const SkImage*, int scaledWidth, int scaledHeight); + static SkBitmapCacheDesc Make(const SkImage*, int width, int height); 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 { @@ -54,19 +42,22 @@ 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 Find(const SkBitmapCacheDesc&, SkBitmap* result, - SkResourceCache* localCache = nullptr); + static bool FindWH(const SkBitmapCacheDesc&, SkBitmap* result, + SkResourceCache* localCache = nullptr); /* * result must be marked isImmutable() */ - static bool Add(const SkBitmapCacheDesc&, const SkBitmap& result, - SkResourceCache* localCache = nullptr); + 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); }; 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 7ef0a99cb6..8bec804243 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::Find(desc, &fResultBitmap)) { + if (!SkBitmapCache::FindWH(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::Add(desc, fResultBitmap)) { + if (SkBitmapCache::AddWH(desc, fResultBitmap)) { provider.notifyAddedToCache(); } } diff --git a/src/core/SkImageCacherator.cpp b/src/core/SkImageCacherator.cpp index 06d2279946..f0bc9973a6 100644 --- a/src/core/SkImageCacherator.cpp +++ b/src/core/SkImageCacherator.cpp @@ -180,8 +180,7 @@ bool SkImageCacherator::directGeneratePixels(const SkImageInfo& info, void* pixe bool SkImageCacherator::lockAsBitmapOnlyIfAlreadyCached(SkBitmap* bitmap, CachedFormat format) { return kNeedNewImageUniqueID != fUniqueIDs[format] && - SkBitmapCache::Find(SkBitmapCacheDesc::Make(fUniqueIDs[format], - fInfo.width(), fInfo.height()), bitmap) && + SkBitmapCache::Find(fUniqueIDs[format], bitmap) && check_output_bitmap(*bitmap, fUniqueIDs[format]); } @@ -200,8 +199,7 @@ bool SkImageCacherator::tryLockAsBitmap(SkBitmap* bitmap, const SkImage* client, } bitmap->pixelRef()->setImmutableWithID(fUniqueIDs[format]); if (SkImage::kAllow_CachingHint == chint) { - SkBitmapCache::Add(SkBitmapCacheDesc::Make(fUniqueIDs[format], - fInfo.width(), fInfo.height()), *bitmap); + SkBitmapCache::Add(fUniqueIDs[format], *bitmap); if (client) { as_IB(client)->notifyAddedToCache(); } @@ -261,8 +259,7 @@ bool SkImageCacherator::lockAsBitmap(GrContext* context, SkBitmap* bitmap, const bitmap->pixelRef()->setImmutableWithID(fUniqueIDs[format]); if (SkImage::kAllow_CachingHint == chint) { - SkBitmapCache::Add(SkBitmapCacheDesc::Make(fUniqueIDs[format], - fInfo.width(), fInfo.height()), *bitmap); + SkBitmapCache::Add(fUniqueIDs[format], *bitmap); if (client) { as_IB(client)->notifyAddedToCache(); } diff --git a/src/core/SkSpecialImage.cpp b/src/core/SkSpecialImage.cpp index 339341a8e8..c0b50634fc 100644 --- a/src/core/SkSpecialImage.cpp +++ b/src/core/SkSpecialImage.cpp @@ -401,8 +401,7 @@ public: } bool onGetROPixels(SkBitmap* dst) const override { - const auto desc = SkBitmapCacheDesc::Make(this->uniqueID(), this->width(), this->height()); - if (SkBitmapCache::Find(desc, dst)) { + if (SkBitmapCache::Find(this->uniqueID(), dst)) { SkASSERT(dst->getGenerationID() == this->uniqueID()); SkASSERT(dst->isImmutable()); SkASSERT(dst->getPixels()); @@ -428,7 +427,7 @@ public: } dst->pixelRef()->setImmutableWithID(this->uniqueID()); - SkBitmapCache::Add(desc, *dst); + SkBitmapCache::Add(this->uniqueID(), *dst); fAddedRasterVersionToCache.store(true); return true; } diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp index b4ec0649f2..66fc813d45 100644 --- a/src/image/SkImage_Gpu.cpp +++ b/src/image/SkImage_Gpu.cpp @@ -63,8 +63,7 @@ SkImageInfo SkImage_Gpu::onImageInfo() const { } bool SkImage_Gpu::getROPixels(SkBitmap* dst, SkColorSpace* dstColorSpace, CachingHint chint) const { - const auto desc = SkBitmapCacheDesc::Make(this); - if (SkBitmapCache::Find(desc, dst)) { + if (SkBitmapCache::Find(this->uniqueID(), dst)) { SkASSERT(dst->getGenerationID() == this->uniqueID()); SkASSERT(dst->isImmutable()); SkASSERT(dst->getPixels()); @@ -87,7 +86,7 @@ bool SkImage_Gpu::getROPixels(SkBitmap* dst, SkColorSpace* dstColorSpace, Cachin dst->pixelRef()->setImmutableWithID(this->uniqueID()); if (kAllow_CachingHint == chint) { - SkBitmapCache::Add(desc, *dst); + SkBitmapCache::Add(this->uniqueID(), *dst); fAddedRasterVersionToCache.store(true); } return true; diff --git a/tests/ImageTest.cpp b/tests/ImageTest.cpp index ba49000d9a..ab2215b17b 100644 --- a/tests/ImageTest.cpp +++ b/tests/ImageTest.cpp @@ -411,7 +411,6 @@ 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)); @@ -419,13 +418,13 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(c, reporter, ctxInfo) { { SkBitmap cachedBitmap; - REPORTER_ASSERT(reporter, !SkBitmapCache::Find(desc, &cachedBitmap)); + REPORTER_ASSERT(reporter, !SkBitmapCache::Find(uniqueID, &cachedBitmap)); } surface->getCanvas()->drawImage(image, 0, 0); { SkBitmap cachedBitmap; - if (SkBitmapCache::Find(desc, &cachedBitmap)) { + if (SkBitmapCache::Find(uniqueID, &cachedBitmap)) { REPORTER_ASSERT(reporter, cachedBitmap.getGenerationID() == uniqueID); REPORTER_ASSERT(reporter, cachedBitmap.isImmutable()); REPORTER_ASSERT(reporter, cachedBitmap.getPixels()); @@ -439,7 +438,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(c, reporter, ctxInfo) { image.reset(nullptr); { SkBitmap cachedBitmap; - REPORTER_ASSERT(reporter, !SkBitmapCache::Find(desc, &cachedBitmap)); + REPORTER_ASSERT(reporter, !SkBitmapCache::Find(uniqueID, &cachedBitmap)); } } diff --git a/tests/SkResourceCacheTest.cpp b/tests/SkResourceCacheTest.cpp index b260d89503..542d80efff 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 auto desc = SkBitmapCacheDesc::Make(src[i]); - const SkMipMap* mipmap = SkMipMapCache::FindAndRef(desc, colorMode, cache); + const SkMipMap* mipmap = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src[i]), + 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(desc, colorMode, cache); + mipmap = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src[i]), 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); - const auto desc = SkBitmapCacheDesc::Make(image.get()); + auto imageId = image->uniqueID(); // delete the image image.reset(nullptr); // all resources should have been purged SkBitmap result; - REPORTER_ASSERT(reporter, !SkBitmapCache::Find(desc, &result)); + REPORTER_ASSERT(reporter, !SkBitmapCache::Find(imageId, &result)); } } |