AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Revert "Include color type and color space in bitmap cache key"

Browse Source 
This reverts commit 274a89e0f5.

Reason for revert: Failing unit tests

Original change's description:
> Include color type and color space in bitmap cache key
> 
> Lazy images will soon support decoding to multiple color types and
> color spaces. To avoid cache collisions, we need to fold the decoded
> color type and color space into the key.
> 
> To avoid storing multiple (different) bitmaps with the same gen ID,
> stop propagating the image's ID to the one allocated in the cache.
> 
> Bug: skia:
> Change-Id: I06714725d4309ec813b75e42cc76eda2cda3d2e0
> Reviewed-on: https://skia-review.googlesource.com/c/160380
> Reviewed-by: Mike Klein <mtklein@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>

TBR=mtklein@google.com,brianosman@google.com,reed@google.com

Change-Id: I6c5834b5b65b85dbb7661f526920d9a140ba5737
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:
Reviewed-on: https://skia-review.googlesource.com/c/160801
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This commit is contained in:
Brian Osman 2018-10-09 16:57:15 +00:00 committed by Skia Commit-Bot
parent f7828d0e44
commit 9ff04c00f3
7 changed files with 27 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
src/core/SkBitmapCache.cpp
View File

@ -51,29 +51,22 @@ static SkIRect get_bounds_from_image(const SkImage* image) {
return SkIRect::MakeWH(image->width(), image->height());
}
static uint64_t get_hash_from_color_space(SkColorSpace* colorSpace) {
return colorSpace ? colorSpace->hash() : 0;
}
SkBitmapCacheDesc SkBitmapCacheDesc::Make(uint32_t imageID, SkColorType colorType,
SkColorSpace* colorSpace, const SkIRect& subset) {
SkBitmapCacheDesc SkBitmapCacheDesc::Make(uint32_t imageID, const SkIRect& subset) {
SkASSERT(imageID);
SkASSERT(subset.width() > 0 && subset.height() > 0);
return { imageID, colorType, get_hash_from_color_space(colorSpace), subset };
return { imageID, subset };
}
SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkBitmap& bm) {
SkASSERT(bm.width() > 0 && bm.height() > 0);
return { bm.getGenerationID(), bm.colorType(), get_hash_from_color_space(bm.colorSpace()),
get_bounds_from_bitmap(bm) };
return { bm.getGenerationID(), get_bounds_from_bitmap(bm) };
}
SkBitmapCacheDesc SkBitmapCacheDesc::Make(const SkImage* image) {
SkASSERT(image->width() > 0 && image->height() > 0);
return { image->uniqueID(), image->colorType(), get_hash_from_color_space(image->colorSpace()),
get_bounds_from_image(image) };
return { image->uniqueID(), get_bounds_from_image(image) };
}
namespace {
@ -117,9 +110,8 @@ public:
{
SkASSERT(!(fDM && fMalloc)); // can't have both
// We need an ID to return with the bitmap/pixelref. We can't necessarily use the key/desc
// ID - lazy images cache the same ID with multiple keys (in different color types).
fPrUniqueID = SkNextID::ImageID();
// We need an ID to return with the bitmap/pixelref - return the same ID as the key/desc
fPrUniqueID = desc.fImageID;
REC_TRACE(" Rec(%d): [%d %d] %d\n",
sk_atomic_inc(&gRecCounter), fInfo.width(), fInfo.height(), fPrUniqueID);
}
@ -258,8 +250,6 @@ SkBitmapCache::RecPtr SkBitmapCache::Alloc(const SkBitmapCacheDesc& desc, const
// Ensure that the info matches the subset (i.e. the subset is the entire image)
SkASSERT(info.width() == desc.fSubset.width());
SkASSERT(info.height() == desc.fSubset.height());
SkASSERT(info.colorType() == desc.fColorType);
SkASSERT(get_hash_from_color_space(info.colorSpace()) == desc.fColorSpaceHash);
const size_t rb = info.minRowBytes();
size_t size = info.computeByteSize(rb);

6
src/core/SkBitmapCache.h
View File

@ -20,21 +20,17 @@ void SkNotifyBitmapGenIDIsStale(uint32_t bitmapGenID);
struct SkBitmapCacheDesc {
uint32_t fImageID; // != 0
SkColorType fColorType;
uint64_t fColorSpaceHash;
SkIRect fSubset; // always set to a valid rect (entire or subset)
void validate() const {
SkASSERT(fImageID);
SkASSERT(fSubset.fLeft >= 0 && fSubset.fTop >= 0);
SkASSERT(fSubset.width() > 0 && fSubset.height() > 0);
SkASSERT(kUnknown_SkColorType != fColorType);
}
static SkBitmapCacheDesc Make(const SkBitmap&);
static SkBitmapCacheDesc Make(const SkImage*);
static SkBitmapCacheDesc Make(uint32_t genID, SkColorType, SkColorSpace*,
const SkIRect& subset);
static SkBitmapCacheDesc Make(uint32_t genID, const SkIRect& subset);
};
class SkBitmapCache {

3
src/core/SkSpecialImage.cpp
View File

@ -396,8 +396,7 @@ public:
}
bool onGetROPixels(SkBitmap* dst) const override {
const auto desc = SkBitmapCacheDesc::Make(this->uniqueID(), kN32_SkColorType,
fColorSpace.get(), this->subset());
const auto desc = SkBitmapCacheDesc::Make(this->uniqueID(), this->subset());
if (SkBitmapCache::Find(desc, dst)) {
SkASSERT(dst->getGenerationID() == this->uniqueID());
SkASSERT(dst->isImmutable());

1
src/image/SkImage_GpuBase.cpp
View File

@ -61,6 +61,7 @@ bool SkImage_GpuBase::getROPixels(SkBitmap* dst, SkColorSpace*, CachingHint chin
// rolled the dstColorSpace into the key).
const auto desc = SkBitmapCacheDesc::Make(this);
if (SkBitmapCache::Find(desc, dst)) {
SkASSERT(dst->getGenerationID() == this->uniqueID());
SkASSERT(dst->isImmutable());
SkASSERT(dst->getPixels());
return true;

28
src/image/SkImage_Lazy.cpp
View File

@ -136,11 +136,10 @@ SkImage_Lazy::~SkImage_Lazy() {
//////////////////////////////////////////////////////////////////////////////////////////////////
static bool check_output_bitmap(const SkBitmap& bitmap, const SkImageInfo& info) {
static bool check_output_bitmap(const SkBitmap& bitmap, uint32_t expectedID) {
SkASSERT(bitmap.getGenerationID() == expectedID);
SkASSERT(bitmap.isImmutable());
SkASSERT(bitmap.getPixels());
SkASSERT(bitmap.colorType() == info.colorType());
SkASSERT(SkColorSpace::Equals(bitmap.colorSpace(), info.colorSpace()));
return true;
}
@ -158,12 +157,10 @@ bool SkImage_Lazy::directGeneratePixels(const SkImageInfo& info, void* pixels, s
//////////////////////////////////////////////////////////////////////////////////////////////////
bool SkImage_Lazy::lockAsBitmapOnlyIfAlreadyCached(SkBitmap* bitmap,
const SkImageInfo& dstInfo) const {
auto desc = SkBitmapCacheDesc::Make(fUniqueID, dstInfo.colorType(), dstInfo.colorSpace(),
SkIRect::MakeSize(fInfo.dimensions()));
bool SkImage_Lazy::lockAsBitmapOnlyIfAlreadyCached(SkBitmap* bitmap) const {
auto desc = SkBitmapCacheDesc::Make(fUniqueID, SkIRect::MakeSize(fInfo.dimensions()));
return SkBitmapCache::Find(desc, bitmap) &&
check_output_bitmap(*bitmap, dstInfo);
check_output_bitmap(*bitmap, fUniqueID);
}
static bool generate_pixels(SkImageGenerator* gen, const SkPixmap& pmap, int originX, int originY) {
@ -204,7 +201,8 @@ static bool generate_pixels(SkImageGenerator* gen, const SkPixmap& pmap, int ori
bool SkImage_Lazy::lockAsBitmap(SkBitmap* bitmap, SkImage::CachingHint chint,
const SkImageInfo& info) const {
if (this->lockAsBitmapOnlyIfAlreadyCached(bitmap, info)) {
// TODO: Verify dstColorSpace here
if (this->lockAsBitmapOnlyIfAlreadyCached(bitmap)) {
return true;
}
@ -212,8 +210,7 @@ bool SkImage_Lazy::lockAsBitmap(SkBitmap* bitmap, SkImage::CachingHint chint,
SkBitmapCache::RecPtr cacheRec;
SkPixmap pmap;
if (SkImage::kAllow_CachingHint == chint) {
auto desc = SkBitmapCacheDesc::Make(fUniqueID, info.colorType(), info.colorSpace(),
SkIRect::MakeSize(info.dimensions()));
auto desc = SkBitmapCacheDesc::Make(fUniqueID, SkIRect::MakeSize(info.dimensions()));
cacheRec = SkBitmapCache::Alloc(desc, info, &pmap);
if (!cacheRec) {
return false;
@ -234,13 +231,16 @@ bool SkImage_Lazy::lockAsBitmap(SkBitmap* bitmap, SkImage::CachingHint chint,
if (cacheRec) {
SkBitmapCache::Add(std::move(cacheRec), bitmap);
SkASSERT(bitmap->getPixels()); // we're locked
SkASSERT(bitmap->isImmutable());
SkASSERT(bitmap->getGenerationID() == fUniqueID);
this->notifyAddedToRasterCache();
} else {
*bitmap = tmpBitmap;
bitmap->setImmutable();
bitmap->pixelRef()->setImmutableWithID(fUniqueID);
}
check_output_bitmap(*bitmap, info);
check_output_bitmap(*bitmap, fUniqueID);
return true;
}
@ -251,7 +251,7 @@ bool SkImage_Lazy::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, siz
SkColorSpace* dstColorSpace = dstInfo.colorSpace();
SkBitmap bm;
if (kDisallow_CachingHint == chint) {
if (this->lockAsBitmapOnlyIfAlreadyCached(&bm, dstInfo)) {
if (this->lockAsBitmapOnlyIfAlreadyCached(&bm)) {
return bm.readPixels(dstInfo, dstPixels, dstRB, srcX, srcY);
} else {
// Try passing the caller's buffer directly down to the generator. If this fails we

4
src/image/SkImage_Lazy.h
View File

@ -59,8 +59,8 @@ public:
bool onIsValid(GrContext*) const override;
// Only return true if the generate has already been cached, in a format that matches the info.
bool lockAsBitmapOnlyIfAlreadyCached(SkBitmap*, const SkImageInfo&) const;
// Only return true if the generate has already been cached.
bool lockAsBitmapOnlyIfAlreadyCached(SkBitmap*) const;
// Call the underlying generator directly
bool directGeneratePixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
int srcX, int srcY) const;

2
tests/ImageTest.cpp
View File

@ -337,6 +337,7 @@ DEF_TEST(image_newfrombitmap, reporter) {
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkImage_Gpu2Cpu, 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));
@ -352,6 +353,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkImage_Gpu2Cpu, reporter, ctxInfo) {
{
SkBitmap cachedBitmap;
if (SkBitmapCache::Find(desc, &cachedBitmap)) {
REPORTER_ASSERT(reporter, cachedBitmap.getGenerationID() == uniqueID);
REPORTER_ASSERT(reporter, cachedBitmap.isImmutable());
REPORTER_ASSERT(reporter, cachedBitmap.getPixels());
} else {