Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
37343002:
Allow SkLazyPixelRef to use SkScaledImageCache (Closed)
|
Created:
7 years, 2 months ago by hal.canary Modified:
7 years, 1 month ago CC:
skia-review_googlegroups.com, reed1, mtklein Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionAllow SkLazyPixelRef to use SkScaledImageCache
- SkScaledImageCache:
- Add new FindAndLock/AddAndLock variants that work well with
SkLazyPixelRefs (take width, height, generation_id).
- Add static versions of these new variants.
- SkLazyPixelRef:
- If NULL passed in as SkImageCache* in the constructor, it will
now default to using the static SkScaledImageCache methods to
cache decoded images.
- If (fImageCache==NULL), the default allocator can be changed
with the setAllocator method. If (fImageCache!=NULL), the
SkImageCache handles allocation.
- CachedDecodingPixelRefTest to test the new functionality.
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=12006
Patch Set 1 #Patch Set 2 : linted #
Total comments: 31
Patch Set 3 : changes from all commenters #
Total comments: 20
Patch Set 4 : more changes #
Total comments: 10
Patch Set 5 : clean up SkScaledImageCache::ID* vs. SkImageCache::ID #Patch Set 6 : nits from mtklein #Patch Set 7 : remove cruft #Patch Set 8 : again #
Total comments: 22
Patch Set 9 : refactoring SkScaledImageCache.cpp #Patch Set 10 : lint #Patch Set 11 : lint #
Total comments: 10
Patch Set 12 : remove allocator, clean up code, nits #Patch Set 13 : re-upload #
Messages
Total messages: 22 (0 generated)
Work in progress: it compiles, but isn't tested yet.
https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:521: static SkScaledImageCache* gCache; Doesn't need to be in this CL, but is this a candidate for sk_once? https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:29: , fCacheId(0) Could you add a comment or an assert that 0 is an appropriate number whether we're using SkImageCache or SkScaledImageCache? It might be nice to even define a constant local to this file so we're not comparing against 0. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:42: cache->ref(); nit: 4 spaces You could also use SkSafeRef, which checks against NULL. Not sure whether that's preferred, but it means you could use it in the initialization. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:55: if (fImageCache == NULL) { nit: NULL == fImageCache https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:99: SkBitmap * bm) { Maybe assert that none of these parameters are NULL? https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.h... src/lazy/SkLazyPixelRef.h:74: fAllocator = allocator; SkBitmap::Allocator inherits from SkRefCnt. Typically we would take ownership via SkRefCnt_SafeAssign(fAllocator, allocator); If for some reason we do not want to follow that pattern here, the comment should explain that ownership of fAllocator is not transferred. Is this meant to be protected? Are we going to have a subclass of SkLazyPixelRef that calls it?
Working on changes suggested by scroggo & reed. Still working on tests https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:521: static SkScaledImageCache* gCache; On 2013/10/23 15:27:49, scroggo wrote: > Doesn't need to be in this CL, but is this a candidate for sk_once? I think so. I meant to ask mtklein. I'm not strong on these sorts of data structures. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:29: , fCacheId(0) On 2013/10/23 15:27:49, scroggo wrote: > Could you add a comment or an assert that 0 is an appropriate number whether > we're using SkImageCache or SkScaledImageCache? > > It might be nice to even define a constant local to this file so we're not > comparing against 0. Done. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:42: cache->ref(); On 2013/10/23 15:27:49, scroggo wrote: > nit: 4 spaces > > You could also use SkSafeRef, which checks against NULL. Not sure whether that's > preferred, but it means you could use it in the initialization. Done. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:55: if (fImageCache == NULL) { On 2013/10/23 15:27:49, scroggo wrote: > nit: NULL == fImageCache Done. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:99: SkBitmap * bm) { On 2013/10/23 15:27:49, scroggo wrote: > Maybe assert that none of these parameters are NULL? Done. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.h... src/lazy/SkLazyPixelRef.h:74: fAllocator = allocator; On 2013/10/23 15:27:49, scroggo wrote: > SkBitmap::Allocator inherits from SkRefCnt. Typically we would take ownership > via > > SkRefCnt_SafeAssign(fAllocator, allocator); > > If for some reason we do not want to follow that pattern here, the comment > should explain that ownership of fAllocator is not transferred. I'll make that change, unless there is any reason that the Allocator shouldn't bother incrementing itself. > Is this meant to be protected? Are we going to have a subclass of SkLazyPixelRef > that calls it? Good catch. I would have (hopefully) caught this after I wrote my unit tests.
https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:64: bool init(int32_t width, Can't hurt to have init take its parameters in the same order as your new methods? https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:74: return true; I'm skeptical of methods that always return true. --> void? https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:231: #ifdef USE_HASH It would make me feel warmer and fuzzier if you found a way to factor out more of this common code rather than duplicating it. As it is I'm left wondering if it's exactly the same and keep moving my head back and forth. https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:243: return reinterpret_cast<ID*>(rec); Most of these are using (ID*) to do the cast. Should we be using reinterpret_cast everywhere? What exactly is the relationship between Rec*, intptr_t, ID* and the other ID*? https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:521: static SkScaledImageCache* gCache; On 2013/10/23 16:11:52, halcanary wrote: > On 2013/10/23 15:27:49, scroggo wrote: > > Doesn't need to be in this CL, but is this a candidate for sk_once? > > I think so. I meant to ask mtklein. I'm not strong on these sorts of data > structures. Yep, perfect place for it. #include "SkOnce.h" static void create_cache(SkScaledImageCache** cache) { *cache = SkNEW_ARGS(SkScaledImageCache, (SK_DEFAULT_IMAGE_CACHE_LIMIT)); } static SkScaledImageCache* get_cache() { static SkScaledImageCache* gCache = NULL; SK_DECLARE_STATIC_ONCE(once); SkOnce(&once, create_cache, &gCache); SkASSERT(NULL != gCache); return gCache; } https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:41: if (cache != NULL) { I'd slightly prefer it if you started calling it fImageCache once you initialized it. It's easier to see that it's the same thing we talk about below when it's got the same name. Given that this NULL value has special meaning, I kind of like the explicit if. It calls it out as special rather than hiding it in fImageCache(SkSafeRef(cache)) or something. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:57: SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId)); This sort of cast makes me nervous. Can we make fCacheId a union to make it clear which types of pointers we allow? I guess, if (fImageCache == NULL), SkScaledImageCache::ID, otherwise SkImageCache::ID? https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:94: static void * decode_into_bitmap(SkImage::Info * info, These *'s look wishy washy! Pick a side! (Choose the correct side, the side by the TYPE.) https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:119: if (NULL == fImageCache) { Usually when the nesting gets this deep I think it means we want more functions that do things less conditionally. E.g. if (NULL == fImageCache) { return lockScaledImageCachePixels(...); } else { ... } I trust myself to understand a single if statement, two nested make me think too hard, and three practically guarantees I'm going to get lost. Trust the inliner. Use small functions. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:121: if (fLazilyCachedInfo.fWidth > 0) { // fInfo already populated Sometimes a little private inline method can be helpful for readability, e.g. private: inline bool imageInfoPopulated() const { return fLazilyCachedInfo.fWidth > 0; } I often find that when I write a comment like this it's a sign I want better names. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:121: if (fLazilyCachedInfo.fWidth > 0) { // fInfo already populated We've got about three levels of code jammed into one block here. I'd like it if this hypothetical lockScaledImageCachePixels() method, corresponding to this block, looked like this: if (!this->imageInfoPopulated()) { this->populateImageInfo(...); } void* pixels = this->findInScaledImageCache(...); if (pixels) { return pixels; } pixels = this->decodeImage(...); if (!pixels) { return NULL; } this->addToScaledImageCache(...); return pixels; https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:216: if (NULL == fImageCache) { This guy is small enough that even I'd probably leave it all in one place. You can't really get lost in here like you can in the jungle of lockPixels.
https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:64: bool init(int32_t width, On 2013/10/23 18:50:14, mtklein wrote: > Can't hurt to have init take its parameters in the same order as your new > methods? The parameters are in the order of the key's fields. https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:521: static SkScaledImageCache* gCache; On 2013/10/23 18:50:14, mtklein wrote: > On 2013/10/23 16:11:52, halcanary wrote: > > On 2013/10/23 15:27:49, scroggo wrote: > > > Doesn't need to be in this CL, but is this a candidate for sk_once? > > > > I think so. I meant to ask mtklein. I'm not strong on these sorts of data > > structures. > > Yep, perfect place for it. > > #include "SkOnce.h" > > static void create_cache(SkScaledImageCache** cache) { > *cache = SkNEW_ARGS(SkScaledImageCache, (SK_DEFAULT_IMAGE_CACHE_LIMIT)); > } > > static SkScaledImageCache* get_cache() { > static SkScaledImageCache* gCache = NULL; > SK_DECLARE_STATIC_ONCE(once); > SkOnce(&once, create_cache, &gCache); > SkASSERT(NULL != gCache); > return gCache; > } Done. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:41: if (cache != NULL) { On 2013/10/23 18:50:14, mtklein wrote: > I'd slightly prefer it if you started calling it fImageCache once you > initialized it. It's easier to see that it's the same thing we talk about below > when it's got the same name. > > Given that this NULL value has special meaning, I kind of like the explicit if. > It calls it out as special rather than hiding it in > fImageCache(SkSafeRef(cache)) or something. Done. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:57: SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId)); On 2013/10/23 18:50:14, mtklein wrote: > This sort of cast makes me nervous. Can we make fCacheId a union to make it > clear which types of pointers we allow? I guess, if (fImageCache == NULL), > SkScaledImageCache::ID, otherwise SkImageCache::ID? Can you show me what that would look like? I don't want to put too many implementation details in the header. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:94: static void * decode_into_bitmap(SkImage::Info * info, On 2013/10/23 18:50:14, mtklein wrote: > These *'s look wishy washy! Pick a side! (Choose the correct side, the side by > the TYPE.) Done.
https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/core/SkScaledImageCac... src/core/SkScaledImageCache.cpp:231: #ifdef USE_HASH On 2013/10/23 18:50:14, mtklein wrote: > It would make me feel warmer and fuzzier if you found a way to factor out more > of this common code rather than duplicating it. As it is I'm left wondering if > it's exactly the same and keep moving my head back and forth. +1. It also means that if we fix a bug/modify behavior in one of them we do it in both. https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:53: SkLazyPixelRef::~SkLazyPixelRef() { Now that we ref fAllocator, it needs to be unref'ed in the destructor. (SkSafeUnref) https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:118: SkASSERT(!fErrorInDecoding); Maybe assert NULL == fImageCache as well? https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:127: SkAutoLockPixels autoLockPixels(bitmap); Maybe this is correct, but it seems odd to me: - lockPixels() ensured that bitmap had pixels - getPixels() returns a pointer to those pixels - unlockPixels() makes it so we're no longer guaranteed that those pixels are valid - return potentially invalid pixels. I guess we really know that the pixels are valid due to FindAndLock, which guaranteed that they're valid until Unlock is called. https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:165: SkASSERT(!fErrorInDecoding); Assert that fImageCache != NULL? https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:113: size_t bitmapSize = original.getSize(); const https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:114: size_t oldByteLimit = SkScaledImageCache::GetByteLimit(); Maybe this should be defaultByteLimit? Also, it could be const. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:120: SkImageEncoder::kWEBP_Type}; nit: This would look cleaner as: SkImageEncoder::kWEBP_Type }; <blank line> https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:134: // Since this is lazy, it shouldn't have fPixels yet! This comment seems to go with the REPORTER_ASSERT on line 141? https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:135: size_t bytes = SkScaledImageCache::GetBytesUsed(); Could you rename bytes to bytesUsed? https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:140: void * ptr = NULL; Better name for ptr? lazyPixels?
I still need to ckean up the ID fiasco. https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:53: SkLazyPixelRef::~SkLazyPixelRef() { On 2013/10/23 23:38:33, scroggo wrote: > Now that we ref fAllocator, it needs to be unref'ed in the destructor. > (SkSafeUnref) Done. https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:118: SkASSERT(!fErrorInDecoding); On 2013/10/23 23:38:33, scroggo wrote: > Maybe assert NULL == fImageCache as well? Done. https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:127: SkAutoLockPixels autoLockPixels(bitmap); On 2013/10/23 23:38:33, scroggo wrote: > Maybe this is correct, but it seems odd to me: > > - lockPixels() ensured that bitmap had pixels > - getPixels() returns a pointer to those pixels > - unlockPixels() makes it so we're no longer guaranteed that those pixels are > valid > - return potentially invalid pixels. > > I guess we really know that the pixels are valid due to FindAndLock, which > guaranteed that they're valid until Unlock is called. This is confusing enough that I will add comments explaining it. // At this point, the autoLockPixels will unlockPixels() // to remove bitmap's lock on the pixels. We will then // destroy bitmap. The *only* guarantee that this pointer // remains valid is the guarantee made by // SkScaledImageCache that it will not destroy the *other* // bitmap (SkScaledImageCache::Rec.fBitmap) that holds a // reference to the concrete PixelRef while this record is // locked. https://codereview.chromium.org/37343002/diff/190001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:165: SkASSERT(!fErrorInDecoding); On 2013/10/23 23:38:33, scroggo wrote: > Assert that fImageCache != NULL? Done. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:113: size_t bitmapSize = original.getSize(); On 2013/10/23 23:38:33, scroggo wrote: > const Done. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:114: size_t oldByteLimit = SkScaledImageCache::GetByteLimit(); On 2013/10/23 23:38:33, scroggo wrote: > Maybe this should be defaultByteLimit? > > Also, it could be const. Done. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:120: SkImageEncoder::kWEBP_Type}; On 2013/10/23 23:38:33, scroggo wrote: > nit: This would look cleaner as: > > SkImageEncoder::kWEBP_Type > }; > <blank line> Done. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:134: // Since this is lazy, it shouldn't have fPixels yet! On 2013/10/23 23:38:33, scroggo wrote: > This comment seems to go with the REPORTER_ASSERT on line 141? Done. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:135: size_t bytes = SkScaledImageCache::GetBytesUsed(); On 2013/10/23 23:38:33, scroggo wrote: > Could you rename bytes to bytesUsed? Done. https://codereview.chromium.org/37343002/diff/190001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:140: void * ptr = NULL; On 2013/10/23 23:38:33, scroggo wrote: > Better name for ptr? lazyPixels? Done.
Let me know if this wasn't enough to get you rolling. Happy to help out if you're unfamiliar with what I'm getting at. https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/60001/src/lazy/SkLazyPixelRef.c... src/lazy/SkLazyPixelRef.cpp:57: SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId)); On 2013/10/23 22:57:41, halcanary wrote: > On 2013/10/23 18:50:14, mtklein wrote: > > This sort of cast makes me nervous. Can we make fCacheId a union to make it > > clear which types of pointers we allow? I guess, if (fImageCache == NULL), > > SkScaledImageCache::ID, otherwise SkImageCache::ID? > > Can you show me what that would look like? I don't want to put too many > implementation details in the header. In the header, replace this: // fCacheId is a (SkScaledImageCache::ID*) or a (SkImageCache::ID). intptr_t fCacheId; with something like this: union { SkImageCache::ID fCacheID; // fImageCache != NULL SkScaledImageCache::ID* fScaledCacheID; // fImageCache == NULL }; (I hope that spacing looks right :P )
https://codereview.chromium.org/37343002/diff/220001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/220001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:519: SkOnce<SkScaledImageCache**>(&create_cache_once, create_cache, &gCache); Just FYI, I think the <SkScaledImageCache**> will be inferred for you. Doesn't hurt to keep it if you like it explicit. https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:59: SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId)); * affinity https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:150: void * ptr = decode_into_bitmap(&fLazilyCachedInfo, fDecodeProc, * https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:243: SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId)); * https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:304: void SkLazyPixelRef::setAllocator(SkBitmap::Allocator * allocator) { * https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/220001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:59: virtual void setAllocator(SkBitmap::Allocator * allocator = NULL); * https://codereview.chromium.org/37343002/diff/220001/tests/CachedDecodingPixe... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/37343002/diff/220001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:72: const SkBitmap & b1, const SkBitmap & b2, & https://codereview.chromium.org/37343002/diff/220001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:108: * This checks to see that a LazyPizelRef works as advertized. Are we halfway through a name search-and-replace here? LazyPixelRef == CachedDecodingPixelRef, right? https://codereview.chromium.org/37343002/diff/220001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:110: static void CachedDecodingPixelRef(skiatest::Reporter* reporter) { You may find it somewhat less verbose to just write #include "TestClassDef.h" DEF_TEST(CachedDecodingPixelRef, reporter) { ... } and then drop the stuff at the bottom (which DEF_TEST writes for you). https://codereview.chromium.org/37343002/diff/220001/tests/CachedDecodingPixe... tests/CachedDecodingPixelRefTest.cpp:142: void * lazyPixels = NULL; *
Would one of you like to formally review the cl as it stands now? Thanks for all of the feedback as I was working on this, it helped a lot.
On 2013/10/24 19:32:41, halcanary wrote: > Would one of you like to formally review the cl as it stands now? > > Thanks for all of the feedback as I was working on this, it helped a lot. A number of your files failed the upload. Try again?
On 2013/10/24 19:54:19, scroggo wrote: > On 2013/10/24 19:32:41, halcanary wrote: > > Would one of you like to formally review the cl as it stands now? > > > > Thanks for all of the feedback as I was working on this, it helped a lot. > > A number of your files failed the upload. Try again? Did I fix it?
https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:214: Rec* rec = fHash->find(key); Can this whole #if #else be a helper function? Rec* findRec(Key key) { #ifdef USE_HASH return fHash->find(key) #else return find_rec_in_list(fHead, key); #endif } https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:23: unnecessary newline. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:125: if (fLazilyCachedInfo.fWidth > 0) { // fInfo already populated inline helper function? https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:146: (void)this->getCachedInfo(); The idea behind getCachedInfo returning an info pointer is that we never refer directly to fLazilyCachedInfo (except inside getCachedInfo and the initialization). One way we could preserve that model would be to write the function as follows: { // Asserts SkBitmap bitmap; SkImage::Info* info = this->getCachedInfo(); if (NULL == info) { return NULL } fScaledCacheId = SkScaledImageCache::FindAndLock(...); if (fScaledCacheId != NULL) { SkAutoLockPixels alp(bitmap); void* pixels = bitmap.getPixels(); SkASSERT(NULL != pixels); // Explanation return pixels; } void* pixels = decode_into_bitmap(info, ...); if (NULL == pixels) { fErrorInDecoding = true; return NULL; } fScaledCacheId = SkScaledImageCache::AddAndLock(...); return pixels; } It's probably easier to follow, but it also means that we'll be calling FindAndLock the first time, when we know it would fail. Just a thought. If we end up initializing the SkPixelRef with the proper Info like we discussed, we'll still have to come up with a solution to that problem, but perhaps it can wait til then. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:240: if (fErrorInDecoding || (0 == fCacheId)) { Maybe a comment here that 0 == fCacheId is the same as saying NULL == fScaledCacheId? https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:84: SkScaledImageCache::ID* fScaledCacheId; I thought you were going to change the declaration of ID from a struct to an intptr_t?
https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:72: SkScalar scaleX, do we *always* pass in 1.0 for these today? If so, and so it matches more closely the external API, lets not have these two parameters, and just hard-code 1.0 into fScale[X,Y]. We already make other assumptions for this entry-point (e.g. that our offset is 0,0). https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:199: while ((rec != NULL) && !(rec->fKey == key)) { !(a == b) ? seems like we should have != operator... MUCH easier to read from afar https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:34: static ID* FindAndLock(uint32_t pixelGenerationID, Probably could use a simple comment... // Called if there is no scaling, and no subsetting (i.e. pixelOffset is 0). https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:37: SkBitmap* scaled); slightly confusing to call this 'scaled'. How about just bitmap or result? https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:61: virtual void setAllocator(SkBitmap::Allocator * allocator = NULL); Why is this API introduced at this time?
please take a look. I refactored some of the code inside SkScaledImageCache.cpp to make it cleaner. https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:72: SkScalar scaleX, On 2013/10/24 21:07:04, reed1 wrote: > do we *always* pass in 1.0 for these today? If so, and so it matches more > closely the external API, lets not have these two parameters, and just hard-code > 1.0 into fScale[X,Y]. We already make other assumptions for this entry-point > (e.g. that our offset is 0,0). I have just cleaned this code up a lot. Now there is a single initialization function. https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:199: while ((rec != NULL) && !(rec->fKey == key)) { On 2013/10/24 21:07:04, reed1 wrote: > !(a == b) ? > > seems like we should have != operator... MUCH easier to read from afar Done. https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:214: Rec* rec = fHash->find(key); On 2013/10/24 20:41:39, scroggo wrote: > Can this whole #if #else be a helper function? > > Rec* findRec(Key key) { > #ifdef USE_HASH > return fHash->find(key) > #else > return find_rec_in_list(fHead, key); > #endif > } I have move some things around to remove this repetition. https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:34: static ID* FindAndLock(uint32_t pixelGenerationID, On 2013/10/24 21:07:04, reed1 wrote: > Probably could use a simple comment... > > // Called if there is no scaling, and no subsetting (i.e. pixelOffset is 0). Done. https://codereview.chromium.org/37343002/diff/540001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:37: SkBitmap* scaled); On 2013/10/24 21:07:04, reed1 wrote: > slightly confusing to call this 'scaled'. How about just bitmap or result? Done. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:23: On 2013/10/24 20:41:39, scroggo wrote: > unnecessary newline. Done. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:125: if (fLazilyCachedInfo.fWidth > 0) { // fInfo already populated On 2013/10/24 20:41:39, scroggo wrote: > inline helper function? changed https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:146: (void)this->getCachedInfo(); On 2013/10/24 20:41:39, scroggo wrote: > The idea behind getCachedInfo returning an info pointer is that we never refer > directly to fLazilyCachedInfo (except inside getCachedInfo and the > initialization). One way we could preserve that model would be to write the > function as follows: > { > // Asserts > SkBitmap bitmap; > > SkImage::Info* info = this->getCachedInfo(); > if (NULL == info) { > return NULL > } > > fScaledCacheId = SkScaledImageCache::FindAndLock(...); > if (fScaledCacheId != NULL) { > SkAutoLockPixels alp(bitmap); > void* pixels = bitmap.getPixels(); > SkASSERT(NULL != pixels); > // Explanation > return pixels; > } > > void* pixels = decode_into_bitmap(info, ...); > if (NULL == pixels) { > fErrorInDecoding = true; > return NULL; > } > fScaledCacheId = SkScaledImageCache::AddAndLock(...); > return pixels; > } > > It's probably easier to follow, but it also means that we'll be calling > FindAndLock the first time, when we know it would fail. Just a thought. If we > end up initializing the SkPixelRef with the proper Info like we discussed, we'll > still have to come up with a solution to that problem, but perhaps it can wait > til then. Done. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:240: if (fErrorInDecoding || (0 == fCacheId)) { On 2013/10/24 20:41:39, scroggo wrote: > Maybe a comment here that 0 == fCacheId is the same as saying NULL == > fScaledCacheId? Cleaned up the code to make this clear. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:61: virtual void setAllocator(SkBitmap::Allocator * allocator = NULL); On 2013/10/24 21:07:04, reed1 wrote: > Why is this API introduced at this time? It needs to be done, since "Make current image cache in skia use a simple allocator interface that maps to the DM allocator in chromium and uses malloc elsewhere." is in the roadmap. It makes sense to do it now, since it fits into the decode_into_bitmap() function. https://codereview.chromium.org/37343002/diff/540001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:84: SkScaledImageCache::ID* fScaledCacheId; On 2013/10/24 20:41:39, scroggo wrote: > I thought you were going to change the declaration of ID from a struct to an > intptr_t? I would have, but this way allows me to skip a dozen useless cast operations, each of which is an opportunity to make a mistake.
Have we thought about just removing the code-path for ImageCache entirely? We can always re-add it (unified perhaps with scaledcache) later if we find the need. That would certainly address some of my nits about having to include scaledbitmapcache.h https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:37: SkBitmap* scaled); rename this last param? (since we're not scaling) https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:61: virtual void setAllocator(SkBitmap::Allocator * allocator = NULL); Why are we introducing this now, and why is it virtual?
https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:196: const SkIRect& bound) { nit: bounds https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:253: scaleY, bounds); This pattern is repeated 4 times. Helper function? https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:37: SkBitmap* scaled); returnedBitmap? https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:151: void* pixels = decode_into_bitmap(info, fDecodeProc, &fRowBytes, Since this is the only modifier of info, why not put the const_cast here?
https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:196: const SkIRect& bound) { On 2013/10/25 18:09:48, scroggo wrote: > nit: bounds Done. https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.cpp:253: scaleY, bounds); On 2013/10/25 18:09:48, scroggo wrote: > This pattern is repeated 4 times. Helper function? Really, it is two times for findAndLock and two times for addAndLock. But I was able to streamline it a bit. https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:37: SkBitmap* scaled); On 2013/10/25 18:09:48, scroggo wrote: > returnedBitmap? Done. https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... src/core/SkScaledImageCache.h:37: SkBitmap* scaled); On 2013/10/25 18:09:47, reed1 wrote: > rename this last param? (since we're not scaling) Done. https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.cpp File src/lazy/SkLazyPixelRef.cpp (right): https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.cpp:151: void* pixels = decode_into_bitmap(info, fDecodeProc, &fRowBytes, On 2013/10/25 18:09:48, scroggo wrote: > Since this is the only modifier of info, why not put the const_cast here? Done. https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.h File src/lazy/SkLazyPixelRef.h (right): https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... src/lazy/SkLazyPixelRef.h:61: virtual void setAllocator(SkBitmap::Allocator * allocator = NULL); On 2013/10/25 18:09:47, reed1 wrote: > Why are we introducing this now, and why is it virtual? Okay, I'll remove it.
On 2013/10/25 20:49:37, halcanary wrote: > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > File src/core/SkScaledImageCache.cpp (right): > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > src/core/SkScaledImageCache.cpp:196: const SkIRect& bound) { > On 2013/10/25 18:09:48, scroggo wrote: > > nit: bounds > > Done. > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > src/core/SkScaledImageCache.cpp:253: scaleY, bounds); > On 2013/10/25 18:09:48, scroggo wrote: > > This pattern is repeated 4 times. Helper function? > > Really, it is two times for findAndLock and two times for addAndLock. But I was > able to streamline it a bit. > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > File src/core/SkScaledImageCache.h (right): > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > src/core/SkScaledImageCache.h:37: SkBitmap* scaled); > On 2013/10/25 18:09:48, scroggo wrote: > > returnedBitmap? > > Done. > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > src/core/SkScaledImageCache.h:37: SkBitmap* scaled); > On 2013/10/25 18:09:47, reed1 wrote: > > rename this last param? (since we're not scaling) > > Done. > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.cpp > File src/lazy/SkLazyPixelRef.cpp (right): > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... > src/lazy/SkLazyPixelRef.cpp:151: void* pixels = decode_into_bitmap(info, > fDecodeProc, &fRowBytes, > On 2013/10/25 18:09:48, scroggo wrote: > > Since this is the only modifier of info, why not put the const_cast here? > > Done. > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.h > File src/lazy/SkLazyPixelRef.h (right): > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... > src/lazy/SkLazyPixelRef.h:61: virtual void setAllocator(SkBitmap::Allocator * > allocator = NULL); > On 2013/10/25 18:09:47, reed1 wrote: > > Why are we introducing this now, and why is it virtual? > > Okay, I'll remove it. There are more upload errors. Can you try again?
On 2013/10/28 16:23:12, scroggo wrote: > On 2013/10/25 20:49:37, halcanary wrote: > > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > > File src/core/SkScaledImageCache.cpp (right): > > > > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > > src/core/SkScaledImageCache.cpp:196: const SkIRect& bound) { > > On 2013/10/25 18:09:48, scroggo wrote: > > > nit: bounds > > > > Done. > > > > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > > src/core/SkScaledImageCache.cpp:253: scaleY, bounds); > > On 2013/10/25 18:09:48, scroggo wrote: > > > This pattern is repeated 4 times. Helper function? > > > > Really, it is two times for findAndLock and two times for addAndLock. But I > was > > able to streamline it a bit. > > > > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > > File src/core/SkScaledImageCache.h (right): > > > > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > > src/core/SkScaledImageCache.h:37: SkBitmap* scaled); > > On 2013/10/25 18:09:48, scroggo wrote: > > > returnedBitmap? > > > > Done. > > > > > https://codereview.chromium.org/37343002/diff/790001/src/core/SkScaledImageCa... > > src/core/SkScaledImageCache.h:37: SkBitmap* scaled); > > On 2013/10/25 18:09:47, reed1 wrote: > > > rename this last param? (since we're not scaling) > > > > Done. > > > > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.cpp > > File src/lazy/SkLazyPixelRef.cpp (right): > > > > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... > > src/lazy/SkLazyPixelRef.cpp:151: void* pixels = decode_into_bitmap(info, > > fDecodeProc, &fRowBytes, > > On 2013/10/25 18:09:48, scroggo wrote: > > > Since this is the only modifier of info, why not put the const_cast here? > > > > Done. > > > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.h > > File src/lazy/SkLazyPixelRef.h (right): > > > > > https://codereview.chromium.org/37343002/diff/790001/src/lazy/SkLazyPixelRef.... > > src/lazy/SkLazyPixelRef.h:61: virtual void setAllocator(SkBitmap::Allocator * > > allocator = NULL); > > On 2013/10/25 18:09:47, reed1 wrote: > > > Why are we introducing this now, and why is it virtual? > > > > Okay, I'll remove it. > > There are more upload errors. Can you try again? lgtm. You'll still need reed's approval for changes to include files.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/halcanary@google.com/37343002/1020001
Message was sent while issue was closed.
Change committed as 12006 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
