Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(46)

Issue 54203006: Break up SkLazyPixelRef functionally into class hierarchy. (Closed)

Created:
7 years, 1 month ago by hal.canary
Modified:
7 years, 1 month ago
Reviewers:
scroggo, reed1
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Break up SkLazyPixelRef functionally into class hierarchy. The reason for this CL is to allow greater decoder flexibility. Chrome currently uses its own decoding functions. These allow for greater flexibility in dealing with images with multiple frames or partial data. The DecodeProc function was not flexible enough to handle these. Instead of asking the decoder to squeeze everything into the DecodeProc, we now ask the downstream library to inherit from SkCachingPixelRef. If WebKit's LazyDecodingPixelRef is re-tooled to inherit from SkCachingPixelRef, then it can make use of Skia's caching ability while still allowing it to deal with multiple frames, scaling, subsetting, and partial data. - The abstract SkCachingPixelRef class handles caching the decoded data in a SkScaledImageCache. This class relies on the virtual functions onDecodeInfo() and onDecode() to do the actual decoding of data. - The SkLazyCachingPixelRef class is derived from SkCachingPixelRef. It provides an implementation of onDecodeInfo() and onDecode() in terms of calls to a SkBitmapFactory::DecodeProc function. It also provides an Install() static method which installs a new SkLazyCachingPixelRef into a SkBitmap. SkLazyCachingPixelRef exists for two reasons: to test SkCachingPixelRef within Skia and as an example for downstream developers to make their own classes that inherit from SkCachingPixelRef. - The CachedDecodingPixelRefTest was updated to test the SkLazyCachingPixelRef class and indirectly the SkCachingPixelRef class. BUG= Committed: http://code.google.com/p/skia/source/detail?r=12149

Patch Set 1 #

Total comments: 35

Patch Set 2 : response to scroggo #

Patch Set 3 : lint nits #

Total comments: 8

Patch Set 4 : more nits #

Patch Set 5 : Use SkImageInfo #

Total comments: 6

Patch Set 6 : changes for reed and scroggo #

Total comments: 3

Patch Set 7 : unit test with lockless SkScaledImageCache #

Patch Set 8 : remove SkCachingPixelRef::fCache #

Total comments: 4

Patch Set 9 : suggestions from reed #

Patch Set 10 : remvoe explicit #

Total comments: 3

Patch Set 11 : getInfo change #

Total comments: 16

Patch Set 12 : changes from scroggo #

Total comments: 4

Patch Set 13 : mike's ideas #

Patch Set 14 : last change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+500 lines, -79 lines) Patch
M gyp/core.gypi View 1 chunk +5 lines, -0 lines 0 comments Download
M include/core/SkBitmap.h View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M include/core/SkImage.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M src/core/SkBitmap.cpp View 1 2 3 4 2 chunks +6 lines, -0 lines 0 comments Download
M src/image/SkImage.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -0 lines 0 comments Download
A src/lazy/SkCachingPixelRef.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +81 lines, -0 lines 0 comments Download
A src/lazy/SkCachingPixelRef.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +112 lines, -0 lines 0 comments Download
A src/lazy/SkLazyCachingPixelRef.h View 1 2 3 4 5 6 7 8 1 chunk +99 lines, -0 lines 0 comments Download
A src/lazy/SkLazyCachingPixelRef.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +62 lines, -0 lines 0 comments Download
M tests/CachedDecodingPixelRefTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +122 lines, -79 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
hal.canary
7 years, 1 month ago (2013-10-31 21:08:55 UTC) #1
scroggo
https://codereview.chromium.org/54203006/diff/1/src/core/SkScaledImageCache.h File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/54203006/diff/1/src/core/SkScaledImageCache.h#newcode34 src/core/SkScaledImageCache.h:34: static SkScaledImageCache* Instance(); I think typically our functions to ...
7 years, 1 month ago (2013-10-31 21:53:06 UTC) #2
hal.canary
https://codereview.chromium.org/54203006/diff/1/src/core/SkScaledImageCache.h File src/core/SkScaledImageCache.h (right): https://codereview.chromium.org/54203006/diff/1/src/core/SkScaledImageCache.h#newcode34 src/core/SkScaledImageCache.h:34: static SkScaledImageCache* Instance(); On 2013/10/31 21:53:06, scroggo wrote: > ...
7 years, 1 month ago (2013-11-01 03:29:24 UTC) #3
Tom Hudson
The CL comment gives us a great description of what you're doing, but doesn't say ...
7 years, 1 month ago (2013-11-01 10:12:44 UTC) #4
hal.canary
On 2013/11/01 10:12:44, Tom Hudson wrote: > The CL comment gives us a great description ...
7 years, 1 month ago (2013-11-01 13:05:41 UTC) #5
scroggo
From the issue description: > SkLazyCachingPixelRef exists for two reasons: to test > SkCachingPixelRef within ...
7 years, 1 month ago (2013-11-01 15:47:59 UTC) #6
hal.canary
On 2013/11/01 15:47:59, scroggo wrote: > their* fixed > I'm not convinced that onDecodeInfo should ...
7 years, 1 month ago (2013-11-01 16:30:59 UTC) #7
hal.canary
https://codereview.chromium.org/54203006/diff/230001/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/230001/src/lazy/SkCachingPixelRef.h#newcode23 src/lazy/SkCachingPixelRef.h:23: * attempt to re-decoded. On 2013/11/01 15:48:00, scroggo wrote: ...
7 years, 1 month ago (2013-11-01 16:31:09 UTC) #8
hal.canary
Mike, let me know if you find the changes to Skbitmap.[h|cpp] acceptible. You guys are ...
7 years, 1 month ago (2013-11-01 18:08:40 UTC) #9
reed1
This is at least two different CLs, one being some changes to how mutexes are ...
7 years, 1 month ago (2013-11-01 18:22:27 UTC) #10
scroggo
https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkCachingPixelRef.h#newcode68 src/lazy/SkCachingPixelRef.h:68: * (info->fHeight * fRowBytes) bytes. Should follow the format ...
7 years, 1 month ago (2013-11-01 18:27:55 UTC) #11
hal.canary
https://codereview.chromium.org/54203006/diff/370001/src/core/SkScaledImageCache.cpp File src/core/SkScaledImageCache.cpp (right): https://codereview.chromium.org/54203006/diff/370001/src/core/SkScaledImageCache.cpp#newcode403 src/core/SkScaledImageCache.cpp:403: size_t SkScaledImageCache::getBytesUsed() const { On 2013/11/01 18:22:27, reed1 wrote: ...
7 years, 1 month ago (2013-11-01 18:52:59 UTC) #12
reed1
https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelRef.h#newcode39 src/lazy/SkCachingPixelRef.h:39: explicit SkCachingPixelRef(SkScaledImageCache* cache = NULL); Do we have a ...
7 years, 1 month ago (2013-11-01 21:41:10 UTC) #13
scroggo
On 2013/11/01 21:41:10, reed1 wrote: > https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelRef.h > File src/lazy/SkCachingPixelRef.h (right): > > https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelRef.h#newcode39 > ...
7 years, 1 month ago (2013-11-01 22:30:10 UTC) #14
hal.canary
https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelRef.h#newcode39 src/lazy/SkCachingPixelRef.h:39: explicit SkCachingPixelRef(SkScaledImageCache* cache = NULL); On 2013/11/01 21:41:10, reed1 ...
7 years, 1 month ago (2013-11-04 13:38:49 UTC) #15
hal.canary
Okay, Mike, please take a look.
7 years, 1 month ago (2013-11-04 14:41:20 UTC) #16
reed1
https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelRef.h#newcode29 src/lazy/SkCachingPixelRef.h:29: explicit SkCachingPixelRef(); don't need this word if there is ...
7 years, 1 month ago (2013-11-04 14:49:06 UTC) #17
hal.canary
done https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelRef.h#newcode29 src/lazy/SkCachingPixelRef.h:29: explicit SkCachingPixelRef(); On 2013/11/04 14:49:06, reed1 wrote: > ...
7 years, 1 month ago (2013-11-04 15:25:21 UTC) #18
reed1
https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelRef.h#newcode48 src/lazy/SkCachingPixelRef.h:48: const SkImageInfo* getInfo(); hmmm, seems safer as an API ...
7 years, 1 month ago (2013-11-04 15:44:33 UTC) #19
scroggo
https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelRef.h File src/lazy/SkCachingPixelRef.h (right): https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelRef.h#newcode48 src/lazy/SkCachingPixelRef.h:48: const SkImageInfo* getInfo(); On 2013/11/04 15:44:33, reed1 wrote: > ...
7 years, 1 month ago (2013-11-04 16:09:56 UTC) #20
scroggo
https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelRef.cpp File src/lazy/SkCachingPixelRef.cpp (right): https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelRef.cpp#newcode14 src/lazy/SkCachingPixelRef.cpp:14: // Please note: a non-NULL cache is not thread-safe. ...
7 years, 1 month ago (2013-11-04 17:18:02 UTC) #21
hal.canary
https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelRef.cpp File src/lazy/SkCachingPixelRef.cpp (right): https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelRef.cpp#newcode14 src/lazy/SkCachingPixelRef.cpp:14: // Please note: a non-NULL cache is not thread-safe. ...
7 years, 1 month ago (2013-11-04 18:24:19 UTC) #22
reed1
testing could consider writing a couple of subclasses of CachingPixelRef, that return errors either on ...
7 years, 1 month ago (2013-11-04 18:34:46 UTC) #23
hal.canary
I added one more test. https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelRef.cpp File src/lazy/SkCachingPixelRef.cpp (right): https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelRef.cpp#newcode106 src/lazy/SkCachingPixelRef.cpp:106: fErrorInDecoding = true; On ...
7 years, 1 month ago (2013-11-04 19:41:07 UTC) #24
reed1
lgtm
7 years, 1 month ago (2013-11-04 20:08:06 UTC) #25
scroggo
On 2013/11/04 20:08:06, reed1 wrote: > lgtm lgtm at patch set 14
7 years, 1 month ago (2013-11-04 23:09:51 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/halcanary@google.com/54203006/730002
7 years, 1 month ago (2013-11-05 17:00:25 UTC) #27
commit-bot: I haz the power
7 years, 1 month ago (2013-11-06 10:08:34 UTC) #28
Message was sent while issue was closed.
Change committed as 12149

Powered by Google App Engine
This is Rietveld 408576698