Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22956004:
skia: Added SkDiscardableMemory interface. (Closed)
|
Created:
7 years, 4 months ago by ernstm Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionskia: Added SkDiscardableMemory interface.
Chrome's implementation of SkDiscardableMemory is provided here: https://codereview.chromium.org/23206002. SkDiscardableMemory is intended to be used for image caching.
R=reed@google.com, scroggo@google.com, hclam@chromium.org, jamesr@chromium.org
BUG=229120
Patch Set 1 #
Total comments: 4
Patch Set 2 : Replace factory class by factory function. #
Total comments: 2
Patch Set 3 : Made factory function a method. Added dummy imlementation. #Patch Set 4 : Added missing cpp file. #
Total comments: 1
Patch Set 5 : Capitalized name of static factory method. #Patch Set 6 : Rebase #
Messages
Total messages: 24 (0 generated)
This is an inital version of a SkDiscardableMemory interface for Skia. It resembles the WebDiscardableMemory interface in Blink. The Chrome part of this CL is https://codereview.chromium.org/23206002 Todo: SkGraphics::SetSkDiscardableMemoryFactor needs to store the pointer to the factory in the right place in Skia.
+vollick who may have suggestions/ideas
https://codereview.chromium.org/22956004/diff/1/include/core/SkGraphics.h File include/core/SkGraphics.h (right): https://codereview.chromium.org/22956004/diff/1/include/core/SkGraphics.h#new... include/core/SkGraphics.h:69: static void SetSkDiscardableMemoryFactory( Once SetSkDiscardableMemoryFactory does something, will the SkGraphics take ownership of the SkDiscardableMemoryFactory? Looks like the implementation uses a singleton. Can you add a comment that states that SkGraphics does not manage the factory's lifecycle? https://codereview.chromium.org/22956004/diff/1/src/core/SkGraphics.cpp File src/core/SkGraphics.cpp (right): https://codereview.chromium.org/22956004/diff/1/src/core/SkGraphics.cpp#newco... src/core/SkGraphics.cpp:150: // TODO: Store factory pointer at the right place in Skia. A global, like with the font cache uses? (getSharedGlobals, in SkGlyphCache.cpp)
Not sure this needs to be up at the public SkGraphics.h level. Isn't this a porting impl issue for skia, and not really part of its public api. As an example, see how we map in SkMutex, SkFontHost. Not the cleanest, but the goal was to 1. allow for flexibility based on the embedder 2. try to keep it private to skia's impl
On 2013/08/15 14:49:07, reed1 wrote: > Not sure this needs to be up at the public SkGraphics.h level. Isn't this a > porting impl issue for skia, and not really part of its public api. > > As an example, see how we map in SkMutex, SkFontHost. Not the cleanest, but the > goal was to > 1. allow for flexibility based on the embedder > 2. try to keep it private to skia's impl Should I replace the Factory by a function SK_API SkDiscardableMemory* sk_create_discardable_memory(size_t bytes); that Chrome implements?
On 2013/08/15 18:33:17, Manfred Ernst wrote: > On 2013/08/15 14:49:07, reed1 wrote: > > Not sure this needs to be up at the public SkGraphics.h level. Isn't this a > > porting impl issue for skia, and not really part of its public api. > > > > As an example, see how we map in SkMutex, SkFontHost. Not the cleanest, but > the > > goal was to > > 1. allow for flexibility based on the embedder > > 2. try to keep it private to skia's impl > > Should I replace the Factory by a function > SK_API SkDiscardableMemory* sk_create_discardable_memory(size_t bytes); > that Chrome implements? I think that would be a simpler way to go, at least for the moment.
Replaced SkDiscardableMemoryFactory by a factory function sk_create_discardable_memory, that is implemented by Chrome. Removed public SkGraphics API function. https://codereview.chromium.org/22956004/diff/1/include/core/SkGraphics.h File include/core/SkGraphics.h (right): https://codereview.chromium.org/22956004/diff/1/include/core/SkGraphics.h#new... include/core/SkGraphics.h:69: static void SetSkDiscardableMemoryFactory( On 2013/08/15 14:06:06, scroggo wrote: > Once SetSkDiscardableMemoryFactory does something, will the SkGraphics take > ownership of the SkDiscardableMemoryFactory? > > Looks like the implementation uses a singleton. Can you add a comment that > states that SkGraphics does not manage the factory's lifecycle? Replaced SkDiscardableMemoryFactory by a factory function. https://codereview.chromium.org/22956004/diff/1/src/core/SkGraphics.cpp File src/core/SkGraphics.cpp (right): https://codereview.chromium.org/22956004/diff/1/src/core/SkGraphics.cpp#newco... src/core/SkGraphics.cpp:150: // TODO: Store factory pointer at the right place in Skia. On 2013/08/15 14:06:06, scroggo wrote: > A global, like with the font cache uses? (getSharedGlobals, in SkGlyphCache.cpp) Using the embedding pattern that was used for SkThread.
Lets checkin a dummy .cpp file, so we can test calling this in contexts where we don't have it. e.g. SkThread_none.cpp Just have the impl return NULL I presume. https://codereview.chromium.org/22956004/diff/10001/src/core/SkDiscardableMem... File src/core/SkDiscardableMemory.h (right): https://codereview.chromium.org/22956004/diff/10001/src/core/SkDiscardableMem... src/core/SkDiscardableMemory.h:52: SK_API SkDiscardableMemory* sk_create_discardable_memory(size_t bytes); minor nit: Often in skia we will put this sort of factory on the class itself. e.g. class SkDiscardableMemory { ... static SkDiscardableMemory* Create(size_t); }; Just an option. Either way is fine functionally.
Added dummy implementation and made factory function a method. https://codereview.chromium.org/22956004/diff/10001/src/core/SkDiscardableMem... File src/core/SkDiscardableMemory.h (right): https://codereview.chromium.org/22956004/diff/10001/src/core/SkDiscardableMem... src/core/SkDiscardableMemory.h:52: SK_API SkDiscardableMemory* sk_create_discardable_memory(size_t bytes); On 2013/08/15 20:42:04, reed1 wrote: > minor nit: Often in skia we will put this sort of factory on the class itself. > > e.g. > > class SkDiscardableMemory { > ... > static SkDiscardableMemory* Create(size_t); > }; > > Just an option. Either way is fine functionally. Done.
are we missing the .cpp mentioned in the gyp?
On 2013/08/15 21:14:51, reed1 wrote: > are we missing the .cpp mentioned in the gyp? yep. It's there now.
lgtm w/ Capitalization request https://codereview.chromium.org/22956004/diff/20001/src/core/SkDiscardableMem... File src/core/SkDiscardableMemory.h (right): https://codereview.chromium.org/22956004/diff/20001/src/core/SkDiscardableMem... src/core/SkDiscardableMemory.h:24: static SkDiscardableMemory* create(size_t bytes); nit: static methods in Skia are Capitalized. e.g. Create(size_t)
Capitalized name of static factory method -> Create.
lgtm
On 2013/08/16 00:07:56, scroggo wrote: > lgtm lgtm2, modulo my question on https://codereview.chromium.org/23206002/; if it turns out we want to be smarter when the discardable is purged, a boolean may not cut it as a return type for lock.
On 2013/08/16 11:39:20, vollick wrote: > On 2013/08/16 00:07:56, scroggo wrote: > > lgtm > > lgtm2, modulo my question on https://codereview.chromium.org/23206002/; if it > turns out we want to be smarter when the discardable is purged, a boolean may > not cut it as a return type for lock. I have no doubts that we may need to iterate on this API. That is fine.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/ernstm@chromium.org/22956004/12002
Failed to apply patch for gyp/ports.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file gyp/ports.gyp
Hunk #1 FAILED at 26.
1 out of 1 hunk FAILED -- saving rejects to file gyp/ports.gyp.rej
Patch: gyp/ports.gyp
Index: gyp/ports.gyp
diff --git a/gyp/ports.gyp b/gyp/ports.gyp
index
fff368df6a5b209b8a6b5839c03bc467672f5de7..eff45a1200f5a0f861aeca5de10fdb7dd4f56059
100644
--- a/gyp/ports.gyp
+++ b/gyp/ports.gyp
@@ -26,6 +26,7 @@
'../src/ports/SkDebug_nacl.cpp',
'../src/ports/SkDebug_stdio.cpp',
'../src/ports/SkDebug_win.cpp',
+ '../src/ports/SkDiscardableMemory_none.cpp',
'../src/ports/SkFontHost_win.cpp',
'../src/ports/SkFontHost_win_dw.cpp',
'../src/ports/SkGlobalInitialization_default.cpp',
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/ernstm@chromium.org/22956004/35001
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildBench, BuildGm, BuildMost, BuildSkiaLib, BuildTests, BuildTools http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...
On 2013/08/16 22:29:31, I haz the power (commit-bot) wrote: > Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for > step(s) BuildBench, BuildGm, BuildMost, BuildSkiaLib, BuildTests, BuildTools > http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re... It looks like the build bot is not picking up my changes to gyp/ports.gyp, or am I missing something else?
On 2013/08/16 23:04:30, Manfred Ernst wrote: > On 2013/08/16 22:29:31, I haz the power (commit-bot) wrote: > > Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for > > step(s) BuildBench, BuildGm, BuildMost, BuildSkiaLib, BuildTests, BuildTools > > > http://108.170.217.252:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re... > > It looks like the build bot is not picking up my changes to gyp/ports.gyp, or am > I missing something else? Actually, no, it's not picking up your changes to add SkDiscardableMemory_none.cpp. I saw this last week with https://codereview.chromium.org/22854005/. I think it has something to do with the fact that SkDiscardableMemory_none.cpp is seen as a modification of an existing file, rather than a new file (notice the A+ next to the file, instead of an A). I'm not sure why that results in not submitting the file at all though.
Just talked to Ravi, and it turns out this is a known bug in the commit queue, which doesn't work very well with git (and he is looking into fixing). A git cl dcommit should work though. I'll try to submit when the tree is open.
On 2013/08/19 14:52:02, scroggo wrote: > Just talked to Ravi, and it turns out this is a known bug in the commit queue, > which doesn't work very well with git (and he is looking into fixing). A git cl > dcommit should work though. I'll try to submit when the tree is open. Submitted in https://code.google.com/p/skia/source/detail?r=10797 via git cl dcommit workaround. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
