Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
207683004:
Extract most of the mutable state of SkShader into a separate Context object. (Closed)
|
Created:
6 years, 9 months ago by Dominik Grewe Modified:
6 years, 8 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionExtract most of the mutable state of SkShader into a separate Context object.
SkShader currently stores some state during draw calls via setContext(...).
Move that mutable state into a separate SkShader::Context class that is
constructed on demand for the duration of the draw.
Calls to setContext() are replaced with createContext() which returns a context
corresponding to the shader object or NULL if the parameters to createContext
are invalid.
TEST=out/Debug/dm
BUG=skia:1976
Committed: http://code.google.com/p/skia/source/detail?r=14216
Committed: http://code.google.com/p/skia/source/detail?r=14323
Patch Set 1 #Patch Set 2 : clean up #
Total comments: 94
Patch Set 3 : scroggo's comments #
Total comments: 12
Patch Set 4 : freeLast reduces fStorageUsed; TriColorShaderContext::setup; totalInverse param to validContext #
Total comments: 6
Patch Set 5 : totalInverse param optional; SkShader::computeTotalInverse; SkBitmapProcShader::validInternal #
Total comments: 8
Patch Set 6 : Remove SkBitmapProcState copy constructor and use pointer instead; nits. #
Total comments: 11
Patch Set 7 : SkGradientShader #
Total comments: 20
Patch Set 8 : fix SkColorShader unflattening; nits #Patch Set 9 : rebase & cleanup #
Total comments: 35
Patch Set 10 : nits #
Total comments: 2
Patch Set 11 : clean up #
Total comments: 6
Patch Set 12 : Remove comment about local matrix. #Patch Set 13 : shared caches for gradient shader #Patch Set 14 : split SkOnceFlag #Patch Set 15 : rebase; SkPictureShader #
Total comments: 16
Patch Set 16 : const fixes #Patch Set 17 : add SkGradientShaderBase::getFlags #Patch Set 18 : SkPictureShader fixes #Patch Set 19 : fCachedLocalMatrix #
Total comments: 6
Patch Set 20 : rebase & Leon's comments #
Total comments: 8
Patch Set 21 : nits #
Total comments: 1
Patch Set 22 : rebase; fix memory leak in SkGradientShaderBase::getGradientTableBitmap; if -> ifdef #Patch Set 23 : call virtual SkShader::Context destructor #Patch Set 24 : remove SkBitmapProcState::endContext #Patch Set 25 : rebase #Messages
Total messages: 105 (0 generated)
This is how far I got this week. I've started with your patch but used a slightly different setup: SkShader is still called SkShader and the mutable state is stored in SkShader::Context (rather then SkShaderImpl). This is more in line with SkDrawLooper and I didn't see a reason for using different names here. createContext() always calls validContext() and returns NULL if validContext() returns false. SkSmallAllocator has a new free() method which effectively frees space reserved via reserveT without calling the destructor. But we can easily go back to a model where validContext is called externally and createContext should only be called if the validation succeeds. I've updated all SkShader subclasses except SkGradientShaderBase (which has lots of subclasses of its own). The tests currently compile and succeed when excluding the test cases using gradient shaders (see TEST= above). I'm not sure what's the best way forward but at least we have two possible alternatives now. Maybe we can have a chat next week. https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2589: if (!blitter->resetShaderContext(*fBitmap, p, *fMatrix)) { Afaict, we want to create a new shader context here, but we don't have access to either the shader context or the allocator. The solution here seems to work but maybe there's a better way?
cc Tom & Sami
> This is how far I got this week. I've started with your patch Some of my comments may be addressing things you got from my patch. If so, sorry for the churn... > but used a > slightly different setup: SkShader is still called SkShader and the mutable > state is stored in SkShader::Context (rather then SkShaderImpl). This is more in > line with SkDrawLooper and I didn't see a reason for using different names here. Using the name Context seems fine to me. > createContext() always calls validContext() and returns NULL if validContext() > returns false. SkSmallAllocator has a new free() method which effectively frees > space reserved via reserveT without calling the destructor. But we can easily go > back to a model where validContext is called externally and createContext should > only be called if the validation succeeds. The nice thing about calling validContext externally is that we potentially skip allocating space if validContext returned false. It does complicate things in other ways though, and may not be worth it for the following reasons: - The SkSmallAllocator will make many "allocations" no-ops. - I'm not sure how often createContext fails. > I've updated all SkShader subclasses except SkGradientShaderBase (which has lots > of subclasses of its own). The tests currently compile and succeed when > excluding the test cases using gradient shaders (see TEST= above). > I'm not sure what's the best way forward but at least we have two possible > alternatives now. Maybe we can have a chat next week. I don't think my version is interestingly different (and it isn't as far along). Feel free to schedule a meeting if you want to discuss. My calendar should be up to date (though keep in mind I typically won't be in until around 9am my time - EDT). https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... File include/core/SkEmptyShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... include/core/SkEmptyShader.h:24: virtual size_t contextSize() const SK_OVERRIDE { return sizeof(SkShader::Context); } It seems misleading to me that this returns a non-zero size, since createContext will *always* return NULL. https://codereview.chromium.org/207683004/diff/20001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/core/SkShader.h#... include/core/SkShader.h:186: kFixedStepInX_MatrixClass, // fast perspective, need to call fixedStepInX() each scanline nit: line too long https://codereview.chromium.org/207683004/diff/20001/include/core/SkShader.h#... include/core/SkShader.h:203: * FIXME: Make sure device is still needed. You can remove this line. device is still used by SkTransparentShader. https://codereview.chromium.org/207683004/diff/20001/include/effects/SkTransp... File include/effects/SkTransparentShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/effects/SkTransp... include/effects/SkTransparentShader.h:21: const SkBitmap& device, const SkPaint& paint, device should be on the same line, assuming it fits. If you go beyond the length of one line, the next line should either start lined up with the first parameter, or use an 8 space indent. See https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... (at the bottom). Some other declarations for createContext need to be fixed similarly. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only Here's what I might do: 1. Instead of making createContext call this->validContext, make it call this->INHERITED::validContext. 2. Move this work directly into createContext (operating on an SkBitmapProcState on the stack as you do now). 3. Update BitmapProcShaderContext's constructor to take an SkBitmapProcState as a parameter. Then we won't be repeating work - we'll just have to copy the SkBitmapProcState. We might even mimic steps 1 and 2 for other classes. The downside to that approach is that if class A : public SkShader does its checks for validContext inside createContext, and we want class B : public A (or anyone external) to do those same checks, we'll need to dupe code (bringing the problems of duping code). https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:194: SkBitmapProcShader::BitmapProcShaderContext::~BitmapProcShaderContext() { Any reason not to just remove this entirely? https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:209: int x, int y, SkPMColor dstC[], int count) { Parameters that fit should be on the same line. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:262: int x, int y, uint16_t dstC[], int count) { Same https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... File src/core/SkBitmapProcState.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... src/core/SkBitmapProcState.cpp:401: // TODO(dominikg): Can we just use an SkASSERT(fBitmap)? I think humper@ is the right person to ask that question. Did you try running it with the assert? https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... src/core/SkBitmapProcState.cpp:484: // TODO(dominikg): SkASSERT(fMatrixProc) instead? chooseMatrixProc never returns NULL. If chooseMatrixProc never returns NULL, then I think it's okay to change to SkASSERT (though perhaps it should be a separate change?). https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:583: size_t size = sizeof(SkShader::Context); I think this should be sizeof(Sk3DShaderContext). reed@ had suggested we pass the size to createContext. I was initially against it, but maybe it would have helped us find a bug like this? https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:712: SkShader::Context* fProxyContext; nit: The variable names should align. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1008: static_cast<Sk3DShader::Sk3DShaderContext*>(shaderContext)); Maybe add a comment here that we know shaderContext *must* be an instance of Sk3DShaderContext? (It's too bad we can't assert that.) It's not obvious unless you delve into the code above. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1033: bool newContext = false; Why do you need the variable newContext? Why not just say: if (!fShader->validContext(device, paint, matrix)) { return false; } . . . return true; https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1040: SkASSERT(fShaderContext); This makes me nervous. It means we have to enforce that no shader returns true for validContext and NULL for createContext using the same parameters. Maybe that's the right behavior though, and any shaders that are broken should fail here and result in an assert. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_A8.cpp File src/core/SkBlitter_A8.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_A8.cp... src/core/SkBlitter_A8.cpp:291: int opaque = fShaderContext->getFlags() & SkShader::kOpaqueAlpha_Flag; shaderContext? https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_A8.cp... src/core/SkBlitter_A8.cpp:337: fShaderContext->shadeSpan(x, y, span, width); I wonder why we chose not to cache fShader here. Would it make sense to cache fShaderContext like we do in blitAntiH? https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_ARGB3... File src/core/SkBlitter_ARGB32.cpp (left): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_ARGB3... src/core/SkBlitter_ARGB32.cpp:340: SkShader* shader = fShader; Odd that the cached variable shader wasn't used. I think it would make sense to cache fShaderContext. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_ARGB3... src/core/SkBlitter_ARGB32.cpp:414: SkShader* shader = fShader; Same. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_ARGB3... src/core/SkBlitter_ARGB32.cpp:518: SkShader* shader = fShader; Same https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_ARGB3... src/core/SkBlitter_ARGB32.cpp:547: SkShader* shader = fShader; Same https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_RGB16... File src/core/SkBlitter_RGB16.cpp (left): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_RGB16... src/core/SkBlitter_RGB16.cpp:691: SkShader* shader = fShader; More instances of caching a value and not using the cache. It might be worth investigating how that happened. It seems we might want to cache fShaderContext. https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... File src/core/SkComposeShader.cpp (left): https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... src/core/SkComposeShader.cpp:121: SkShader* shaderA = fShaderA; I think we grabbed these pointers outside the loop for caching purposes. Should we do the same for fShaderContextA/B? https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... src/core/SkComposeShader.cpp:101: // TODO: move this check to SkShader::createContext and the rest to createContextImpl(). The general naming pattern in Skia is to name the virtual version (the one you name Impl) onCreateContext. https://codereview.chromium.org/207683004/diff/20001/src/core/SkCoreBlitters.h File src/core/SkCoreBlitters.h (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkCoreBlitters.... src/core/SkCoreBlitters.h:52: bool fShaderContextIsValid; Why is this needed (it appears that it is never used, though maybe I overlooked it). https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2383: const SkTriColorShader& fTriColorShader; Any reason SkTriColorShader does not simply static cast fShader? https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2411: int index0, int index1, int index2, const SkMatrix& matrix) { Maybe rename this parameter to make it clear what it's used for. totalInverse? https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2449: const SkTriColorShader& shader, const SkBitmap& device, nit: Parameters that fit on the same line should go on the same line. https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2589: if (!blitter->resetShaderContext(*fBitmap, p, *fMatrix)) { On 2014/03/21 12:52:50, Dominik Grewe wrote: > Afaict, we want to create a new shader context here, but we don't have access to > either the shader context or the allocator. The solution here seems to work but > maybe there's a better way? This seems like a fine way to do it to me. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:42: const SkPaint& paint, nit: spacing https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:51: if (m->invert(NULL)) { This can be replaced with: return m->invert(NULL); https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:59: : fShader(shader) { nit: Easier to follow if you move the brace to its own line after an initializer list, to distinguish the body of the constructor from the list. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:66: if (fShader.hasLocalMatrix()) { Worth it to refer to the local variable so we don't need to read from memory? https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:67: total.setConcat(matrix, fShader.getLocalMatrix()); Ditto https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:255: : INHERITED(shader, device, paint, matrix) { nit: brace on its own line. https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocator.h File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocato... src/core/SkSmallAllocator.h:142: // Find the corresponding Rec. Do we think it's necessary to allow freeing anything besides the last thing allocated? I'm guessing the common use case will be to call reserveT, learn that the object could not be created, then call free. I'd argue for a function called freeLast(), which would immediately free the memory (if necessary), and reduce fNumObjects by one. That will avoid this linear search, which admittedly, is of a short list, but it's still code we may not need. It also keeps our list of fRecs more consistent (all need to have their fKillProc called). In SkChunkAlloc, we have an analogous function called unalloc(), which simply fails if you request to unalloc anything but the last object allocated. I like freeLast() better personally, because it needs no parameter. https://codereview.chromium.org/207683004/diff/20001/src/effects/SkPerlinNois... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/effects/SkPerlinNois... src/effects/SkPerlinNoiseShader.cpp:463: SkPerlinNoiseShader::PerlinNoiseShaderContext::~PerlinNoiseShaderContext() {} Do you need to declare this destructor at all? https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... File src/effects/SkTransparentShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:29: bool SkTransparentShader::validContext(const SkBitmap& device, Why did you override this if all you do is call inherited? https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:36: const SkTransparentShader& shader, const SkBitmap& device, nit: 8 space indent. https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:40: , fAlpha(paint.getAlpha()) {} fAlpha wasn't created by you, but as long as we're modifying this code - isn't it redundant? Can't you call getPaintAlpha() when you need it? https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:64: int x, int y, SkPMColor span[], int count) { nit: The parameters that fit on the same line should remain on the initial line. https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:123: int x, int y, uint16_t span[], int count) { same.
Thanks! Updated, ptal. https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... File include/core/SkEmptyShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... include/core/SkEmptyShader.h:24: virtual size_t contextSize() const SK_OVERRIDE { return sizeof(SkShader::Context); } On 2014/03/24 21:24:46, scroggo wrote: > It seems misleading to me that this returns a non-zero size, since createContext > will *always* return NULL. I agree it's misleading but the problem is that we allocate space for the context before calling createContext. And when allocating space, SkSmallAllocator checks that the given size is >= sizeof(T) with T = SkShader::Context. I'll add a comment explaining this. https://codereview.chromium.org/207683004/diff/20001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/core/SkShader.h#... include/core/SkShader.h:186: kFixedStepInX_MatrixClass, // fast perspective, need to call fixedStepInX() each scanline On 2014/03/24 21:24:46, scroggo wrote: > nit: line too long Done. https://codereview.chromium.org/207683004/diff/20001/include/core/SkShader.h#... include/core/SkShader.h:203: * FIXME: Make sure device is still needed. On 2014/03/24 21:24:46, scroggo wrote: > You can remove this line. device is still used by SkTransparentShader. Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/24 21:24:46, scroggo wrote: > Here's what I might do: > > 1. Instead of making createContext call this->validContext, make it call > this->INHERITED::validContext. > 2. Move this work directly into createContext (operating on an SkBitmapProcState > on the stack as you do now). > 3. Update BitmapProcShaderContext's constructor to take an SkBitmapProcState as > a parameter. > > Then we won't be repeating work - we'll just have to copy the SkBitmapProcState. > > We might even mimic steps 1 and 2 for other classes. The downside to that > approach is that if class A : public SkShader does its checks for validContext > inside createContext, and we want class B : public A (or anyone external) to do > those same checks, we'll need to dupe code (bringing the problems of duping > code). Yeah, that could easily lead to problems, couldn't it? Or are we sure that we won't have subclasses of SkBitmapProcShader? https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:194: SkBitmapProcShader::BitmapProcShaderContext::~BitmapProcShaderContext() { On 2014/03/24 21:24:46, scroggo wrote: > Any reason not to just remove this entirely? Just for completeness. I've moved the empty body to the class definition. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:209: int x, int y, SkPMColor dstC[], int count) { On 2014/03/24 21:24:46, scroggo wrote: > Parameters that fit should be on the same line. If I read the coding style correctly it seems that I can put them on the next line if indented by 8 spaces, but I'm happy to put the ones that fit on the same line if that's more conventional. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... File src/core/SkBitmapProcState.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... src/core/SkBitmapProcState.cpp:484: // TODO(dominikg): SkASSERT(fMatrixProc) instead? chooseMatrixProc never returns NULL. On 2014/03/24 21:24:46, scroggo wrote: > If chooseMatrixProc never returns NULL, then I think it's okay to change to > SkASSERT (though perhaps it should be a separate change?). I was trying to reduce the cases where this function could fail so that we can create a short validation method, but maybe we won't need that in the end. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:583: size_t size = sizeof(SkShader::Context); On 2014/03/24 21:24:46, scroggo wrote: > I think this should be sizeof(Sk3DShaderContext). Yes. > reed@ had suggested we pass the size to createContext. I was initially against > it, but maybe it would have helped us find a bug like this? Maybe, though we'd check for exactly what we return here, so if we have a bug here we're likely to have the same bug in createContext :) https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:712: SkShader::Context* fProxyContext; On 2014/03/24 21:24:46, scroggo wrote: > nit: The variable names should align. Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1008: static_cast<Sk3DShader::Sk3DShaderContext*>(shaderContext)); On 2014/03/24 21:24:46, scroggo wrote: > Maybe add a comment here that we know shaderContext *must* be an instance of > Sk3DShaderContext? (It's too bad we can't assert that.) It's not obvious unless > you delve into the code above. Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1033: bool newContext = false; On 2014/03/24 21:24:46, scroggo wrote: > Why do you need the variable newContext? > > Why not just say: > > if (!fShader->validContext(device, paint, matrix)) { > return false; > } > . > . > . > return true; Good point. I rewrote this function a few times and I should have removed it. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1040: SkASSERT(fShaderContext); On 2014/03/24 21:24:46, scroggo wrote: > This makes me nervous. It means we have to enforce that no shader returns true > for validContext and NULL for createContext using the same parameters. Maybe > that's the right behavior though, and any shaders that are broken should fail > here and result in an assert. I agree. I was hoping we'd never have to call validContext outside of the shaders, but unless we give the blitter its own SkSmallAllocator (wdyt?) I don't see a way around it. I assumed that createContext returns NULL iff validContext returns false, but that wouldn't be the case any more if we changed SkBitmapProcShader the way you suggested. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_RGB16... File src/core/SkBlitter_RGB16.cpp (left): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter_RGB16... src/core/SkBlitter_RGB16.cpp:691: SkShader* shader = fShader; On 2014/03/24 21:24:46, scroggo wrote: > More instances of caching a value and not using the cache. It might be worth > investigating how that happened. It seems we might want to cache fShaderContext. I didn't realize we can't always rely on the compiler do to it for us. Will update it wherever necessary. https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... File src/core/SkComposeShader.cpp (left): https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... src/core/SkComposeShader.cpp:121: SkShader* shaderA = fShaderA; On 2014/03/24 21:24:46, scroggo wrote: > I think we grabbed these pointers outside the loop for caching purposes. Should > we do the same for fShaderContextA/B? Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkComposeShader... src/core/SkComposeShader.cpp:101: // TODO: move this check to SkShader::createContext and the rest to createContextImpl(). On 2014/03/24 21:24:46, scroggo wrote: > The general naming pattern in Skia is to name the virtual version (the one you > name Impl) onCreateContext. Oh yes, I forgot. But if we break the contract between validContext and createContext we may not want to do this anyway. https://codereview.chromium.org/207683004/diff/20001/src/core/SkCoreBlitters.h File src/core/SkCoreBlitters.h (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkCoreBlitters.... src/core/SkCoreBlitters.h:52: bool fShaderContextIsValid; On 2014/03/24 21:24:46, scroggo wrote: > Why is this needed (it appears that it is never used, though maybe I overlooked > it). You're right. Thanks for spotting. It was used in an earlier version and I forgot to remove it. https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2383: const SkTriColorShader& fTriColorShader; On 2014/03/24 21:24:46, scroggo wrote: > Any reason SkTriColorShader does not simply static cast fShader? Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2411: int index0, int index1, int index2, const SkMatrix& matrix) { On 2014/03/24 21:24:46, scroggo wrote: > Maybe rename this parameter to make it clear what it's used for. totalInverse? It's not the totalInverse though, is it? We use it to compute the inverse but I don't know what it represents by itself. It's the same matrix we pass into createContext(). https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2449: const SkTriColorShader& shader, const SkBitmap& device, On 2014/03/24 21:24:46, scroggo wrote: > nit: Parameters that fit on the same line should go on the same line. If I move the first one to the same line and line up all the others below it, it takes up more space than this way. I think the way it is is cleaner and the style guide seems to support it iiuc. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:42: const SkPaint& paint, On 2014/03/24 21:24:46, scroggo wrote: > nit: spacing Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:51: if (m->invert(NULL)) { On 2014/03/24 21:24:46, scroggo wrote: > This can be replaced with: > > return m->invert(NULL); Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:59: : fShader(shader) { On 2014/03/24 21:24:46, scroggo wrote: > nit: Easier to follow if you move the brace to its own line after an initializer > list, to distinguish the body of the constructor from the list. Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:66: if (fShader.hasLocalMatrix()) { On 2014/03/24 21:24:46, scroggo wrote: > Worth it to refer to the local variable so we don't need to read from memory? Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:67: total.setConcat(matrix, fShader.getLocalMatrix()); On 2014/03/24 21:24:46, scroggo wrote: > Ditto Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:255: : INHERITED(shader, device, paint, matrix) { On 2014/03/24 21:24:46, scroggo wrote: > nit: brace on its own line. Done. https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocator.h File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocato... src/core/SkSmallAllocator.h:142: // Find the corresponding Rec. On 2014/03/24 21:24:46, scroggo wrote: > Do we think it's necessary to allow freeing anything besides the last thing > allocated? I'm guessing the common use case will be to call reserveT, learn that > the object could not be created, then call free. > > I'd argue for a function called freeLast(), which would immediately free the > memory (if necessary), and reduce fNumObjects by one. That will avoid this > linear search, which admittedly, is of a short list, but it's still code we may > not need. It also keeps our list of fRecs more consistent (all need to have > their fKillProc called). > > In SkChunkAlloc, we have an analogous function called unalloc(), which simply > fails if you request to unalloc anything but the last object allocated. I like > freeLast() better personally, because it needs no parameter. Sounds good to me. I've implemented it. If we also want to make the reserved space available for future allocations we'd have to keep track of allocation sizes. Do we want to do that? https://codereview.chromium.org/207683004/diff/20001/src/effects/SkPerlinNois... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/effects/SkPerlinNois... src/effects/SkPerlinNoiseShader.cpp:463: SkPerlinNoiseShader::PerlinNoiseShaderContext::~PerlinNoiseShaderContext() {} On 2014/03/24 21:24:46, scroggo wrote: > Do you need to declare this destructor at all? Moved it to the class definition. https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... File src/effects/SkTransparentShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:29: bool SkTransparentShader::validContext(const SkBitmap& device, On 2014/03/24 21:24:46, scroggo wrote: > Why did you override this if all you do is call inherited? Removed it. https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:36: const SkTransparentShader& shader, const SkBitmap& device, On 2014/03/24 21:24:46, scroggo wrote: > nit: 8 space indent. Done. https://codereview.chromium.org/207683004/diff/20001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:40: , fAlpha(paint.getAlpha()) {} On 2014/03/24 21:24:46, scroggo wrote: > fAlpha wasn't created by you, but as long as we're modifying this code - isn't > it redundant? Can't you call getPaintAlpha() when you need it? Done.
https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... File include/core/SkEmptyShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... include/core/SkEmptyShader.h:24: virtual size_t contextSize() const SK_OVERRIDE { return sizeof(SkShader::Context); } On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > It seems misleading to me that this returns a non-zero size, since > createContext > > will *always* return NULL. > > I agree it's misleading but the problem is that we allocate space for the > context before calling createContext. And when allocating space, > SkSmallAllocator checks that the given size is >= sizeof(T) with T = > SkShader::Context. > I'll add a comment explaining this. Ah, that makes sense. I'll see if I can find it, but I think you have another class that returns 0 for contextSize(). https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > Here's what I might do: > > > > 1. Instead of making createContext call this->validContext, make it call > > this->INHERITED::validContext. > > 2. Move this work directly into createContext (operating on an > SkBitmapProcState > > on the stack as you do now). > > 3. Update BitmapProcShaderContext's constructor to take an SkBitmapProcState > as > > a parameter. > > > > Then we won't be repeating work - we'll just have to copy the > SkBitmapProcState. > > > > We might even mimic steps 1 and 2 for other classes. The downside to that > > approach is that if class A : public SkShader does its checks for validContext > > inside createContext, and we want class B : public A (or anyone external) to > do > > those same checks, we'll need to dupe code (bringing the problems of duping > > code). > > Yeah, that could easily lead to problems, couldn't it? Or are we sure that we > won't have subclasses of SkBitmapProcShader? I don't think we'll subclass SkBitmapProcShader, and it is private (inside src/core), so our clients shouldn't either. What if we created a new inline function that took an SkBitmapProcShader as a parameter, modified it and returned the results of chooseProcs? It could be called by both validContext and createContext. (I guess we're ending up with creating an object that has invalid state, but we're kind of already doing that before your change anyway...) https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:209: int x, int y, SkPMColor dstC[], int count) { On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > Parameters that fit should be on the same line. > > If I read the coding style correctly it seems that I can put them on the next > line if indented by 8 spaces, but I'm happy to put the ones that fit on the same > line if that's more conventional. I think it's more conventional, but you're right, the style guide does seem to allow your version. I'll try to stop being so picky ;) https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:583: size_t size = sizeof(SkShader::Context); On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > I think this should be sizeof(Sk3DShaderContext). > > Yes. > > > reed@ had suggested we pass the size to createContext. I was initially against > > it, but maybe it would have helped us find a bug like this? > > Maybe, though we'd check for exactly what we return here, so if we have a bug > here we're likely to have the same bug in createContext :) I guess we could assert that the size of the returned object in createContext is smaller than this->contextSize(). It's a bit ugly and requires extra code in each class though :( https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1040: SkASSERT(fShaderContext); On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > This makes me nervous. It means we have to enforce that no shader returns true > > for validContext and NULL for createContext using the same parameters. Maybe > > that's the right behavior though, and any shaders that are broken should fail > > here and result in an assert. > > I agree. I was hoping we'd never have to call validContext outside of the > shaders, but unless we give the blitter its own SkSmallAllocator (wdyt?) I don't > see a way around it. Who else is calling validContext? (I was thinking someone might call it as an assert, but I don't remember if anyone else calls it.) Why do you want to give the blitter its own allocator? It would be kind of ironic because the blitter is created by an SkSmallAllocator. > > I assumed that createContext returns NULL iff validContext returns false, but > that wouldn't be the case any more if we changed SkBitmapProcShader the way you > suggested. https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2411: int index0, int index1, int index2, const SkMatrix& matrix) { On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > Maybe rename this parameter to make it clear what it's used for. totalInverse? > > It's not the totalInverse though, is it? We use it to compute the inverse but I > don't know what it represents by itself. It's the same matrix we pass into > createContext(). Oh, I misread the code below... https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2429: SkASSERT(matrix.invert(&inverse)); I seemed to have overlooked this part. I take it this is mimicking the work that is done in setContext/createContext? A few comments: - This appears to ignore the local matrix. - Code inside SkASSERT is removed from Release builds. To do what you want (assert that the code returns true in debug, and just run the code ignoring the result in release), you can SkAssertResult. - It's not obvious why you're asserting that it must succeed - presumably because validContext would have returned NULL previously so we would not have reached here. If that's the case, it could use a comment. - Why don't we pass the Context to this function, or Context::getTotalInverse? (It would mean adding a getter to the blitter, maybe that's okay?) Then we know we have the value we want. As suggested in SkBitmapProcShader, perhaps validContext should have an out parameter that returns the total inverse? https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocator.h File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocato... src/core/SkSmallAllocator.h:142: // Find the corresponding Rec. On 2014/03/26 17:22:22, Dominik Grewe wrote: > On 2014/03/24 21:24:46, scroggo wrote: > > Do we think it's necessary to allow freeing anything besides the last thing > > allocated? I'm guessing the common use case will be to call reserveT, learn > that > > the object could not be created, then call free. > > > > I'd argue for a function called freeLast(), which would immediately free the > > memory (if necessary), and reduce fNumObjects by one. That will avoid this > > linear search, which admittedly, is of a short list, but it's still code we > may > > not need. It also keeps our list of fRecs more consistent (all need to have > > their fKillProc called). > > > > In SkChunkAlloc, we have an analogous function called unalloc(), which simply > > fails if you request to unalloc anything but the last object allocated. I like > > freeLast() better personally, because it needs no parameter. > > Sounds good to me. I've implemented it. If we also want to make the reserved > space available for future allocations we'd have to keep track of allocation > sizes. Do we want to do that? That seems reasonable. It might be nice to know how often it's necessary to call freeLast(). https://codereview.chromium.org/207683004/diff/60001/include/core/SkEmptyShad... File include/core/SkEmptyShader.h (right): https://codereview.chromium.org/207683004/diff/60001/include/core/SkEmptyShad... include/core/SkEmptyShader.h:25: // Even though we never create a shader for this class we have to return a value of at least I might say // Even though createContext returns NULL... https://codereview.chromium.org/207683004/diff/60001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:111: // TODO(dominikg): Can we avoid re-computing the inverse? One way would be for validContext to have an out parameter that returns the inverse. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:113: SkASSERT(matrix.invert(&inverse)); As in the SkTriColorShader, this should be SkAssertResult and it should take into account the localMatrix. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1042: fShaderContext = fShader->createContext(device, paint, matrix, (void*)fShaderContext); Ooh, this could be a problem. If the new shader's context is larger than the old one, it won't fit in the storage. https://codereview.chromium.org/207683004/diff/60001/src/effects/SkTransparen... File src/effects/SkTransparentShader.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:43: if (getPaintAlpha() == 255) style nit: When calling an objects own member function, we precede it with 'this->'. Same for the others.
https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... File include/core/SkEmptyShader.h (right): https://codereview.chromium.org/207683004/diff/20001/include/core/SkEmptyShad... include/core/SkEmptyShader.h:24: virtual size_t contextSize() const SK_OVERRIDE { return sizeof(SkShader::Context); } On 2014/03/26 23:13:09, scroggo wrote: > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > On 2014/03/24 21:24:46, scroggo wrote: > > > It seems misleading to me that this returns a non-zero size, since > > createContext > > > will *always* return NULL. > > > > I agree it's misleading but the problem is that we allocate space for the > > context before calling createContext. And when allocating space, > > SkSmallAllocator checks that the given size is >= sizeof(T) with T = > > SkShader::Context. > > I'll add a comment explaining this. > > Ah, that makes sense. > > I'll see if I can find it, but I think you have another class that returns 0 for > contextSize(). Only for SkGradientShader which isn't implement yet. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/26 23:13:09, scroggo wrote: > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > On 2014/03/24 21:24:46, scroggo wrote: > > > Here's what I might do: > > > > > > 1. Instead of making createContext call this->validContext, make it call > > > this->INHERITED::validContext. > > > 2. Move this work directly into createContext (operating on an > > SkBitmapProcState > > > on the stack as you do now). > > > 3. Update BitmapProcShaderContext's constructor to take an SkBitmapProcState > > as > > > a parameter. > > > > > > Then we won't be repeating work - we'll just have to copy the > > SkBitmapProcState. > > > > > > We might even mimic steps 1 and 2 for other classes. The downside to that > > > approach is that if class A : public SkShader does its checks for > validContext > > > inside createContext, and we want class B : public A (or anyone external) to > > do > > > those same checks, we'll need to dupe code (bringing the problems of duping > > > code). > > > > Yeah, that could easily lead to problems, couldn't it? Or are we sure that we > > won't have subclasses of SkBitmapProcShader? > > I don't think we'll subclass SkBitmapProcShader, and it is private (inside > src/core), so our clients shouldn't either. > > What if we created a new inline function that took an SkBitmapProcShader as a > parameter, modified it and returned the results of chooseProcs? It could be > called by both validContext and createContext. (I guess we're ending up with > creating an object that has invalid state, but we're kind of already doing that > before your change anyway...) Do you mean SkBitmapProcState rather than Shader? We'd still call chooseProcs twice then, right? I was hoping we can avoid that or call a more light-weight method in validContext that returns as soon as we know whether we succeed or not (without actually setting up the SkBitmapProcState). https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... File src/core/SkBitmapProcState.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSta... src/core/SkBitmapProcState.cpp:401: // TODO(dominikg): Can we just use an SkASSERT(fBitmap)? On 2014/03/24 21:24:46, scroggo wrote: > I think humper@ is the right person to ask that question. > > Did you try running it with the assert? Works fine on the tests. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1040: SkASSERT(fShaderContext); On 2014/03/26 23:13:09, scroggo wrote: > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > On 2014/03/24 21:24:46, scroggo wrote: > > > This makes me nervous. It means we have to enforce that no shader returns > true > > > for validContext and NULL for createContext using the same parameters. Maybe > > > that's the right behavior though, and any shaders that are broken should > fail > > > here and result in an assert. > > > > I agree. I was hoping we'd never have to call validContext outside of the > > shaders, but unless we give the blitter its own SkSmallAllocator (wdyt?) I > don't > > see a way around it. > > Who else is calling validContext? (I was thinking someone might call it as an > assert, but I don't remember if anyone else calls it.) This is the only caller outside SkShader afaict. > > Why do you want to give the blitter its own allocator? It would be kind of > ironic because the blitter is created by an SkSmallAllocator. It should work in principle, right? :) Though it seems like we're creating the shader context before creating the blitter, so maybe we can't do it here... The idea was this: If the blitter has its own allocator, we could reset it here (might need a new method for that) instead of calling the destructor without actually owning the storage. It's a weird contract at the moment, where SkBlitter may change the object itself but has to ensure that there's always some object so the owner of the storage can destroy it. https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2429: SkASSERT(matrix.invert(&inverse)); On 2014/03/26 23:13:09, scroggo wrote: > I seemed to have overlooked this part. I take it this is mimicking the work that > is done in setContext/createContext? A few comments: > > - This appears to ignore the local matrix. > - Code inside SkASSERT is removed from Release builds. To do what you want > (assert that the code returns true in debug, and just run the code ignoring the > result in release), you can SkAssertResult. > - It's not obvious why you're asserting that it must succeed - presumably > because validContext would have returned NULL previously so we would not have > reached here. If that's the case, it could use a comment. > - Why don't we pass the Context to this function, or Context::getTotalInverse? > (It would mean adding a getter to the blitter, maybe that's okay?) Then we know > we have the value we want. > > As suggested in SkBitmapProcShader, perhaps validContext should have an out > parameter that returns the total inverse? Actually, if we add a SkBlitter::getShaderContext(), we can move this method to TriColorShaderContext where we have access to totalInverse. It also means that SkTriColorShader is immutable. https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocator.h File src/core/SkSmallAllocator.h (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkSmallAllocato... src/core/SkSmallAllocator.h:142: // Find the corresponding Rec. On 2014/03/26 23:13:09, scroggo wrote: > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > On 2014/03/24 21:24:46, scroggo wrote: > > > Do we think it's necessary to allow freeing anything besides the last thing > > > allocated? I'm guessing the common use case will be to call reserveT, learn > > that > > > the object could not be created, then call free. > > > > > > I'd argue for a function called freeLast(), which would immediately free the > > > memory (if necessary), and reduce fNumObjects by one. That will avoid this > > > linear search, which admittedly, is of a short list, but it's still code we > > may > > > not need. It also keeps our list of fRecs more consistent (all need to have > > > their fKillProc called). > > > > > > In SkChunkAlloc, we have an analogous function called unalloc(), which > simply > > > fails if you request to unalloc anything but the last object allocated. I > like > > > freeLast() better personally, because it needs no parameter. > > > > Sounds good to me. I've implemented it. If we also want to make the reserved > > space available for future allocations we'd have to keep track of allocation > > sizes. Do we want to do that? > > That seems reasonable. It might be nice to know how often it's necessary to call > freeLast(). During the tests (excluding the ones using gradient shader) it gets called 6 times. https://codereview.chromium.org/207683004/diff/60001/include/core/SkEmptyShad... File include/core/SkEmptyShader.h (right): https://codereview.chromium.org/207683004/diff/60001/include/core/SkEmptyShad... include/core/SkEmptyShader.h:25: // Even though we never create a shader for this class we have to return a value of at least On 2014/03/26 23:13:09, scroggo wrote: > I might say > > // Even though createContext returns NULL... Agreed, that's better. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:111: // TODO(dominikg): Can we avoid re-computing the inverse? On 2014/03/26 23:13:09, scroggo wrote: > One way would be for validContext to have an out parameter that returns the > inverse. I gave that a try. PTAL. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1042: fShaderContext = fShader->createContext(device, paint, matrix, (void*)fShaderContext); On 2014/03/26 23:13:09, scroggo wrote: > Ooh, this could be a problem. If the new shader's context is larger than the old > one, it won't fit in the storage. I think it's fine though because the shader itself hasn't changed (apart from localMatrix but that shouldn't matter). We only use different parameters to set up the context. I mention that in a comment in the class definition but I'll add a comment here too. https://codereview.chromium.org/207683004/diff/60001/src/effects/SkTransparen... File src/effects/SkTransparentShader.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/effects/SkTransparen... src/effects/SkTransparentShader.cpp:43: if (getPaintAlpha() == 255) On 2014/03/26 23:13:09, scroggo wrote: > style nit: When calling an objects own member function, we precede it with > 'this->'. Same for the others. Done.
https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/27 14:27:20, Dominik Grewe wrote: > On 2014/03/26 23:13:09, scroggo wrote: > > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > > On 2014/03/24 21:24:46, scroggo wrote: > > > > Here's what I might do: > > > > > > > > 1. Instead of making createContext call this->validContext, make it call > > > > this->INHERITED::validContext. > > > > 2. Move this work directly into createContext (operating on an > > > SkBitmapProcState > > > > on the stack as you do now). > > > > 3. Update BitmapProcShaderContext's constructor to take an > SkBitmapProcState > > > as > > > > a parameter. > > > > > > > > Then we won't be repeating work - we'll just have to copy the > > > SkBitmapProcState. > > > > > > > > We might even mimic steps 1 and 2 for other classes. The downside to that > > > > approach is that if class A : public SkShader does its checks for > > validContext > > > > inside createContext, and we want class B : public A (or anyone external) > to > > > do > > > > those same checks, we'll need to dupe code (bringing the problems of > duping > > > > code). > > > > > > Yeah, that could easily lead to problems, couldn't it? Or are we sure that > we > > > won't have subclasses of SkBitmapProcShader? > > > > I don't think we'll subclass SkBitmapProcShader, and it is private (inside > > src/core), so our clients shouldn't either. > > > > What if we created a new inline function that took an SkBitmapProcShader as a > > parameter, modified it and returned the results of chooseProcs? It could be > > called by both validContext and createContext. (I guess we're ending up with > > creating an object that has invalid state, but we're kind of already doing > that > > before your change anyway...) > > Do you mean SkBitmapProcState rather than Shader? We'd still call chooseProcs > twice then, right? I was hoping we can avoid that or call a more light-weight > method in validContext that returns as soon as we know whether we succeed or not > (without actually setting up the SkBitmapProcState). You are correct; I did mean SkBitmapProcState. I don't know of a way to make chooseProcs lighter weight (but perhaps it could be done). Here's what I imagine: private: bool SkBitmapProcShader::validInternal(device, paint, matrix, SkBitmapProcState* state) { SkMatrix totalInverse; if (!this->INHERITED::validContext(device, paint, matrix, &totalInverse)) { return false; } state->fTileModeX = fTileModeX; state->fTileModeY = fTileModeY; state.fOrigBitmap = fRawBitmap; return state.chooseProcs(totalInverse, paint); } bool SkBitmapProcShader::validContext(device, matrix, paint, SkMatrix*) { SkBitmapProcState state; return this->validInternal(device, matrix, paint, &state); } SkShader::Context* SkBitmapProcShader::createContext(device, paint, matrix, storage) { SkBitmapProcState state; if (!this->validInternal(device, matrix, paint, &state)) { return false; } return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, device, paint, matrix, state)); } In this model, chooseProcs is only called once (since I believe the typical caller will skip validContext and skip to createContext), but we still have a useful validContext call (in case we want to assert, for example, or in resetShaderContext - are there other reasons we call it?). https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1040: SkASSERT(fShaderContext); On 2014/03/27 14:27:20, Dominik Grewe wrote: > On 2014/03/26 23:13:09, scroggo wrote: > > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > > On 2014/03/24 21:24:46, scroggo wrote: > > > > This makes me nervous. It means we have to enforce that no shader returns > > true > > > > for validContext and NULL for createContext using the same parameters. > Maybe > > > > that's the right behavior though, and any shaders that are broken should > > fail > > > > here and result in an assert. > > > > > > I agree. I was hoping we'd never have to call validContext outside of the > > > shaders, but unless we give the blitter its own SkSmallAllocator (wdyt?) I > > don't > > > see a way around it. > > > > Who else is calling validContext? (I was thinking someone might call it as an > > assert, but I don't remember if anyone else calls it.) > > This is the only caller outside SkShader afaict. > > > > > Why do you want to give the blitter its own allocator? It would be kind of > > ironic because the blitter is created by an SkSmallAllocator. > > It should work in principle, right? :) Though it seems like we're creating the > shader context before creating the blitter, so maybe we can't do it here... > > The idea was this: > If the blitter has its own allocator, we could reset it here (might need a new > method for that) instead of calling the destructor without actually owning the > storage. It's a weird contract at the moment, where SkBlitter may change the > object itself but has to ensure that there's always some object so the owner of > the storage can destroy it. > I actually think it makes sense that blitter has to call the destructor of the object it replaces. We're assuming that the object is owned, and it was going to have its destructor called. So the blitter just called it early, and put a new one there (whose destructor will be called when the original one would have originally). Back to the blitter having its own SkSmallAllocator, I'm not against it, now that I think about it. In that case, we could almost convert the SkSmallAllocator to only have the ability to create one object (so long as we don't mind going back to heap allocating the Sk3DBlitter in SkBlitter::Choose, which should be fine). https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2429: SkASSERT(matrix.invert(&inverse)); On 2014/03/27 14:27:20, Dominik Grewe wrote: > On 2014/03/26 23:13:09, scroggo wrote: > > I seemed to have overlooked this part. I take it this is mimicking the work > that > > is done in setContext/createContext? A few comments: > > > > - This appears to ignore the local matrix. > > - Code inside SkASSERT is removed from Release builds. To do what you want > > (assert that the code returns true in debug, and just run the code ignoring > the > > result in release), you can SkAssertResult. > > - It's not obvious why you're asserting that it must succeed - presumably > > because validContext would have returned NULL previously so we would not have > > reached here. If that's the case, it could use a comment. > > - Why don't we pass the Context to this function, or Context::getTotalInverse? > > (It would mean adding a getter to the blitter, maybe that's okay?) Then we > know > > we have the value we want. > > > > As suggested in SkBitmapProcShader, perhaps validContext should have an out > > parameter that returns the total inverse? > > Actually, if we add a SkBlitter::getShaderContext(), we can move this method to > TriColorShaderContext where we have access to totalInverse. It also means that > SkTriColorShader is immutable. That seems even better to me. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1042: fShaderContext = fShader->createContext(device, paint, matrix, (void*)fShaderContext); On 2014/03/27 14:27:20, Dominik Grewe wrote: > On 2014/03/26 23:13:09, scroggo wrote: > > Ooh, this could be a problem. If the new shader's context is larger than the > old > > one, it won't fit in the storage. > > I think it's fine though because the shader itself hasn't changed (apart from > localMatrix but that shouldn't matter). We only use different parameters to set > up the context. I mention that in a comment in the class definition but I'll add > a comment here too. I can imagine a shader which creates a different sized object (and returns a different number for contextSize) for a different device/paint/matrix. I can't imagine any use for that, but it's something we should consider. Maybe modify the comment a little to indicate that a maniacal shader could thwart us with mysterious bugs here? https://codereview.chromium.org/207683004/diff/80001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/80001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:602: return fProxy->validContext(device, paint, matrix, &totalInverseProxy); I was imagining that this could be an optional parameter, and then you could just use NULL here. I guess the tradeoff is in your version we have to create this matrix you don't use, versus an if check with the optional parameter. Not sure which is better. (As I look further in your diff and see all the places where we had to add the unused matrix, I'm leaning towards the optional parameter defaulting to NULL.) https://codereview.chromium.org/207683004/diff/80001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/80001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2560: // Important that we abort early, as below we may manipulate the shader context Comment nit: We're replacing the shader context, but that shouldn't be a reason to abort early. We are (currently) modifying the shader (by calling setLocalMatrix). Once we move local matrix off the shader, we won't be modifying the shader, so the only reason to abort early (afaict) is speed up the null blitter case. https://codereview.chromium.org/207683004/diff/80001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/80001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:62: SkAssertResult(fShader.validContext(device, paint, matrix, &fTotalInverse)); Okay, now I understand your concern about calling chooseProcs twice. The last patch calls invert twice, whereas this one calls validContext inside the Context constructor (in both debug and release), resulting in two calls to chooseProcs, in addition to two calls to invert. It seems like you could do just the invert twice (preferably by using a helper function called by both valid- and create-) while still asserting that validContext returns true here.
https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/27 18:05:33, scroggo wrote: > On 2014/03/27 14:27:20, Dominik Grewe wrote: > > On 2014/03/26 23:13:09, scroggo wrote: > > > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > > > On 2014/03/24 21:24:46, scroggo wrote: > > > > > Here's what I might do: > > > > > > > > > > 1. Instead of making createContext call this->validContext, make it call > > > > > this->INHERITED::validContext. > > > > > 2. Move this work directly into createContext (operating on an > > > > SkBitmapProcState > > > > > on the stack as you do now). > > > > > 3. Update BitmapProcShaderContext's constructor to take an > > SkBitmapProcState > > > > as > > > > > a parameter. > > > > > > > > > > Then we won't be repeating work - we'll just have to copy the > > > > SkBitmapProcState. > > > > > > > > > > We might even mimic steps 1 and 2 for other classes. The downside to > that > > > > > approach is that if class A : public SkShader does its checks for > > > validContext > > > > > inside createContext, and we want class B : public A (or anyone > external) > > to > > > > do > > > > > those same checks, we'll need to dupe code (bringing the problems of > > duping > > > > > code). > > > > > > > > Yeah, that could easily lead to problems, couldn't it? Or are we sure that > > we > > > > won't have subclasses of SkBitmapProcShader? > > > > > > I don't think we'll subclass SkBitmapProcShader, and it is private (inside > > > src/core), so our clients shouldn't either. > > > > > > What if we created a new inline function that took an SkBitmapProcShader as > a > > > parameter, modified it and returned the results of chooseProcs? It could be > > > called by both validContext and createContext. (I guess we're ending up with > > > creating an object that has invalid state, but we're kind of already doing > > that > > > before your change anyway...) > > > > Do you mean SkBitmapProcState rather than Shader? We'd still call chooseProcs > > twice then, right? I was hoping we can avoid that or call a more light-weight > > method in validContext that returns as soon as we know whether we succeed or > not > > (without actually setting up the SkBitmapProcState). > > You are correct; I did mean SkBitmapProcState. I don't know of a way to make > chooseProcs lighter weight (but perhaps it could be done). Here's what I > imagine: > > private: > bool SkBitmapProcShader::validInternal(device, paint, matrix, > SkBitmapProcState* state) { > SkMatrix totalInverse; > if (!this->INHERITED::validContext(device, paint, matrix, &totalInverse)) { > return false; > } > > state->fTileModeX = fTileModeX; > state->fTileModeY = fTileModeY; > state.fOrigBitmap = fRawBitmap; > > return state.chooseProcs(totalInverse, paint); > } > > bool SkBitmapProcShader::validContext(device, matrix, paint, SkMatrix*) { > SkBitmapProcState state; > return this->validInternal(device, matrix, paint, &state); > } > > SkShader::Context* SkBitmapProcShader::createContext(device, paint, matrix, > storage) { > SkBitmapProcState state; > if (!this->validInternal(device, matrix, paint, &state)) { > return false; > } > return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, device, > paint, matrix, > state)); > } > > In this model, chooseProcs is only called once (since I believe the typical > caller > will skip validContext and skip to createContext), but we still have a useful > validContext call (in case we want to assert, for example, or in > resetShaderContext > - are there other reasons we call it?). Yes, that looks reasonable. The only caveat is that it requires a decent copy constructor for SkBitmapProcState which currently doesn't exist. Simply copying each element doesn't work. One problem is, for example, that when copying fOrigBitmap and fScaledBitmap we don't copy the pixel refs (I believe). fBitmapFilter also looks problematic. I don't really understand that code very well, so if you have any insights please let me know. Otherwise I'll have a closer look next week. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1040: SkASSERT(fShaderContext); On 2014/03/27 18:05:33, scroggo wrote: > On 2014/03/27 14:27:20, Dominik Grewe wrote: > > On 2014/03/26 23:13:09, scroggo wrote: > > > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > > > On 2014/03/24 21:24:46, scroggo wrote: > > > > > This makes me nervous. It means we have to enforce that no shader > returns > > > true > > > > > for validContext and NULL for createContext using the same parameters. > > Maybe > > > > > that's the right behavior though, and any shaders that are broken should > > > fail > > > > > here and result in an assert. > > > > > > > > I agree. I was hoping we'd never have to call validContext outside of the > > > > shaders, but unless we give the blitter its own SkSmallAllocator (wdyt?) I > > > don't > > > > see a way around it. > > > > > > Who else is calling validContext? (I was thinking someone might call it as > an > > > assert, but I don't remember if anyone else calls it.) > > > > This is the only caller outside SkShader afaict. > > > > > > > > Why do you want to give the blitter its own allocator? It would be kind of > > > ironic because the blitter is created by an SkSmallAllocator. > > > > It should work in principle, right? :) Though it seems like we're creating the > > shader context before creating the blitter, so maybe we can't do it here... > > > > The idea was this: > > If the blitter has its own allocator, we could reset it here (might need a new > > method for that) instead of calling the destructor without actually owning the > > storage. It's a weird contract at the moment, where SkBlitter may change the > > object itself but has to ensure that there's always some object so the owner > of > > the storage can destroy it. > > > > I actually think it makes sense that blitter has to call the destructor of the > object it replaces. We're assuming that the object is owned, and it was going to > have its destructor called. So the blitter just called it early, and put a new > one there (whose destructor will be called when the original one would have > originally). > > Back to the blitter having its own SkSmallAllocator, I'm not against it, now > that I think about it. In that case, we could almost convert the > SkSmallAllocator to only have the ability to create one object (so long as we > don't mind going back to heap allocating the Sk3DBlitter in SkBlitter::Choose, > which should be fine). The only problem is that the first SkShader::Context is created before the blitter is created. Maybe that can be changed but if you're happy with the solution here it may not be worth it. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1042: fShaderContext = fShader->createContext(device, paint, matrix, (void*)fShaderContext); On 2014/03/27 18:05:33, scroggo wrote: > On 2014/03/27 14:27:20, Dominik Grewe wrote: > > On 2014/03/26 23:13:09, scroggo wrote: > > > Ooh, this could be a problem. If the new shader's context is larger than the > > old > > > one, it won't fit in the storage. > > > > I think it's fine though because the shader itself hasn't changed (apart from > > localMatrix but that shouldn't matter). We only use different parameters to > set > > up the context. I mention that in a comment in the class definition but I'll > add > > a comment here too. > > I can imagine a shader which creates a different sized object (and returns a > different number for contextSize) for a different device/paint/matrix. I can't > imagine any use for that, but it's something we should consider. > > Maybe modify the comment a little to indicate that a maniacal shader could > thwart us with mysterious bugs here? SkShader::contextSize() returns a size independent of device, paint or matrix. Could we require for it to return the maximum size of a context? Then we should be good. https://codereview.chromium.org/207683004/diff/80001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/80001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:602: return fProxy->validContext(device, paint, matrix, &totalInverseProxy); On 2014/03/27 18:05:33, scroggo wrote: > I was imagining that this could be an optional parameter, and then you could > just use NULL here. I guess the tradeoff is in your version we have to create > this matrix you don't use, versus an if check with the optional parameter. Not > sure which is better. (As I look further in your diff and see all the places > where we had to add the unused matrix, I'm leaning towards the optional > parameter defaulting to NULL.) I've made it optional. https://codereview.chromium.org/207683004/diff/80001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/80001/src/core/SkDraw.cpp#newc... src/core/SkDraw.cpp:2560: // Important that we abort early, as below we may manipulate the shader context On 2014/03/27 18:05:33, scroggo wrote: > Comment nit: We're replacing the shader context, but that shouldn't be a reason > to abort early. We are (currently) modifying the shader (by calling > setLocalMatrix). Once we move local matrix off the shader, we won't be modifying > the shader, so the only reason to abort early (afaict) is speed up the null > blitter case. Updated. https://codereview.chromium.org/207683004/diff/80001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/80001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:62: SkAssertResult(fShader.validContext(device, paint, matrix, &fTotalInverse)); On 2014/03/27 18:05:33, scroggo wrote: > Okay, now I understand your concern about calling chooseProcs twice. The last > patch calls invert twice, whereas this one calls validContext inside the Context > constructor (in both debug and release), resulting in two calls to chooseProcs, > in addition to two calls to invert. > > It seems like you could do just the invert twice (preferably by using a helper > function called by both valid- and create-) while still asserting that > validContext returns true here. Done.
https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/28 18:16:31, Dominik Grewe wrote: > On 2014/03/27 18:05:33, scroggo wrote: > > On 2014/03/27 14:27:20, Dominik Grewe wrote: > > > On 2014/03/26 23:13:09, scroggo wrote: > > > > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > > > > On 2014/03/24 21:24:46, scroggo wrote: > > > > > > Here's what I might do: > > > > > > > > > > > > 1. Instead of making createContext call this->validContext, make it > call > > > > > > this->INHERITED::validContext. > > > > > > 2. Move this work directly into createContext (operating on an > > > > > SkBitmapProcState > > > > > > on the stack as you do now). > > > > > > 3. Update BitmapProcShaderContext's constructor to take an > > > SkBitmapProcState > > > > > as > > > > > > a parameter. > > > > > > > > > > > > Then we won't be repeating work - we'll just have to copy the > > > > > SkBitmapProcState. > > > > > > > > > > > > We might even mimic steps 1 and 2 for other classes. The downside to > > that > > > > > > approach is that if class A : public SkShader does its checks for > > > > validContext > > > > > > inside createContext, and we want class B : public A (or anyone > > external) > > > to > > > > > do > > > > > > those same checks, we'll need to dupe code (bringing the problems of > > > duping > > > > > > code). > > > > > > > > > > Yeah, that could easily lead to problems, couldn't it? Or are we sure > that > > > we > > > > > won't have subclasses of SkBitmapProcShader? > > > > > > > > I don't think we'll subclass SkBitmapProcShader, and it is private (inside > > > > src/core), so our clients shouldn't either. > > > > > > > > What if we created a new inline function that took an SkBitmapProcShader > as > > a > > > > parameter, modified it and returned the results of chooseProcs? It could > be > > > > called by both validContext and createContext. (I guess we're ending up > with > > > > creating an object that has invalid state, but we're kind of already doing > > > that > > > > before your change anyway...) > > > > > > Do you mean SkBitmapProcState rather than Shader? We'd still call > chooseProcs > > > twice then, right? I was hoping we can avoid that or call a more > light-weight > > > method in validContext that returns as soon as we know whether we succeed or > > not > > > (without actually setting up the SkBitmapProcState). > > > > You are correct; I did mean SkBitmapProcState. I don't know of a way to make > > chooseProcs lighter weight (but perhaps it could be done). Here's what I > > imagine: > > > > private: > > bool SkBitmapProcShader::validInternal(device, paint, matrix, > > SkBitmapProcState* state) { > > SkMatrix totalInverse; > > if (!this->INHERITED::validContext(device, paint, matrix, &totalInverse)) { > > return false; > > } > > > > state->fTileModeX = fTileModeX; > > state->fTileModeY = fTileModeY; > > state.fOrigBitmap = fRawBitmap; > > > > return state.chooseProcs(totalInverse, paint); > > } > > > > bool SkBitmapProcShader::validContext(device, matrix, paint, SkMatrix*) { > > SkBitmapProcState state; > > return this->validInternal(device, matrix, paint, &state); > > } > > > > SkShader::Context* SkBitmapProcShader::createContext(device, paint, matrix, > > storage) { > > SkBitmapProcState state; > > if (!this->validInternal(device, matrix, paint, &state)) { > > return false; > > } > > return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, > device, > > paint, > matrix, > > state)); > > } > > > > In this model, chooseProcs is only called once (since I believe the typical > > caller > > will skip validContext and skip to createContext), but we still have a useful > > validContext call (in case we want to assert, for example, or in > > resetShaderContext > > - are there other reasons we call it?). > > Yes, that looks reasonable. The only caveat is that it requires a decent copy > constructor for SkBitmapProcState which currently doesn't exist. Simply copying > each element doesn't work. One problem is, for example, that when copying > fOrigBitmap and fScaledBitmap we don't copy the pixel refs (I believe). > fBitmapFilter also looks problematic. > > I don't really understand that code very well, so if you have any insights > please let me know. Otherwise I'll have a closer look next week. I also don't have much time to look at this until next week. As for copying fOrigBitmap and fScaledBitmap, I'm assuming we don't want to do a deep copy (the copy constructor for SkBitmap will ref the pixel ref). You're right though, I'm sure the copy constructor will be complicated. On the other hand, we could pass a (SkBitmapProcState) pointer to the (BitmapProcShader) constructor, and have no need to write the copy constructor. We can skip the extra call to new by putting it in the storage space. https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/60001/src/core/SkBlitter.cpp#n... src/core/SkBlitter.cpp:1042: fShaderContext = fShader->createContext(device, paint, matrix, (void*)fShaderContext); On 2014/03/28 18:16:31, Dominik Grewe wrote: > On 2014/03/27 18:05:33, scroggo wrote: > > On 2014/03/27 14:27:20, Dominik Grewe wrote: > > > On 2014/03/26 23:13:09, scroggo wrote: > > > > Ooh, this could be a problem. If the new shader's context is larger than > the > > > old > > > > one, it won't fit in the storage. > > > > > > I think it's fine though because the shader itself hasn't changed (apart > from > > > localMatrix but that shouldn't matter). We only use different parameters to > > set > > > up the context. I mention that in a comment in the class definition but I'll > > add > > > a comment here too. > > > > I can imagine a shader which creates a different sized object (and returns a > > different number for contextSize) for a different device/paint/matrix. I can't > > imagine any use for that, but it's something we should consider. > > > > Maybe modify the comment a little to indicate that a maniacal shader could > > thwart us with mysterious bugs here? > > SkShader::contextSize() returns a size independent of device, paint or matrix. > Could we require for it to return the maximum size of a context? Then we should > be good. Oh right of course! Part of the beauty of making it an immutable class. You are right; there is no danger here :) https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:110: if (totalInverse == NULL) { nit: Prefer putting NULL on the left of this check. https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:123: if (!state->chooseProcs(*totalInverse, paint)) { return state->chooseProcs(*totalInverse, paint); https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:145: return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, device, paint, matrix, state)); nit: 100 chars. https://codereview.chromium.org/207683004/diff/100001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/100001/src/core/SkShader.cpp#n... src/core/SkShader.cpp:66: SkAssertResult(fShader.computeTotalInverse(matrix, &fTotalInverse)); This looks much better. A comment would be nice, explaining why we know we can assert this returns true.
Thanks Leon. I'll look at SkGradientShader next. https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/20001/src/core/SkBitmapProcSha... src/core/SkBitmapProcShader.cpp:119: // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only On 2014/03/28 20:50:07, scroggo wrote: > On 2014/03/28 18:16:31, Dominik Grewe wrote: > > On 2014/03/27 18:05:33, scroggo wrote: > > > On 2014/03/27 14:27:20, Dominik Grewe wrote: > > > > On 2014/03/26 23:13:09, scroggo wrote: > > > > > On 2014/03/26 17:22:22, Dominik Grewe wrote: > > > > > > On 2014/03/24 21:24:46, scroggo wrote: > > > > > > > Here's what I might do: > > > > > > > > > > > > > > 1. Instead of making createContext call this->validContext, make it > > call > > > > > > > this->INHERITED::validContext. > > > > > > > 2. Move this work directly into createContext (operating on an > > > > > > SkBitmapProcState > > > > > > > on the stack as you do now). > > > > > > > 3. Update BitmapProcShaderContext's constructor to take an > > > > SkBitmapProcState > > > > > > as > > > > > > > a parameter. > > > > > > > > > > > > > > Then we won't be repeating work - we'll just have to copy the > > > > > > SkBitmapProcState. > > > > > > > > > > > > > > We might even mimic steps 1 and 2 for other classes. The downside to > > > that > > > > > > > approach is that if class A : public SkShader does its checks for > > > > > validContext > > > > > > > inside createContext, and we want class B : public A (or anyone > > > external) > > > > to > > > > > > do > > > > > > > those same checks, we'll need to dupe code (bringing the problems of > > > > duping > > > > > > > code). > > > > > > > > > > > > Yeah, that could easily lead to problems, couldn't it? Or are we sure > > that > > > > we > > > > > > won't have subclasses of SkBitmapProcShader? > > > > > > > > > > I don't think we'll subclass SkBitmapProcShader, and it is private > (inside > > > > > src/core), so our clients shouldn't either. > > > > > > > > > > What if we created a new inline function that took an SkBitmapProcShader > > as > > > a > > > > > parameter, modified it and returned the results of chooseProcs? It could > > be > > > > > called by both validContext and createContext. (I guess we're ending up > > with > > > > > creating an object that has invalid state, but we're kind of already > doing > > > > that > > > > > before your change anyway...) > > > > > > > > Do you mean SkBitmapProcState rather than Shader? We'd still call > > chooseProcs > > > > twice then, right? I was hoping we can avoid that or call a more > > light-weight > > > > method in validContext that returns as soon as we know whether we succeed > or > > > not > > > > (without actually setting up the SkBitmapProcState). > > > > > > You are correct; I did mean SkBitmapProcState. I don't know of a way to make > > > chooseProcs lighter weight (but perhaps it could be done). Here's what I > > > imagine: > > > > > > private: > > > bool SkBitmapProcShader::validInternal(device, paint, matrix, > > > SkBitmapProcState* state) { > > > SkMatrix totalInverse; > > > if (!this->INHERITED::validContext(device, paint, matrix, &totalInverse)) > { > > > return false; > > > } > > > > > > state->fTileModeX = fTileModeX; > > > state->fTileModeY = fTileModeY; > > > state.fOrigBitmap = fRawBitmap; > > > > > > return state.chooseProcs(totalInverse, paint); > > > } > > > > > > bool SkBitmapProcShader::validContext(device, matrix, paint, SkMatrix*) { > > > SkBitmapProcState state; > > > return this->validInternal(device, matrix, paint, &state); > > > } > > > > > > SkShader::Context* SkBitmapProcShader::createContext(device, paint, matrix, > > > storage) { > > > SkBitmapProcState state; > > > if (!this->validInternal(device, matrix, paint, &state)) { > > > return false; > > > } > > > return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, > > device, > > > paint, > > matrix, > > > state)); > > > } > > > > > > In this model, chooseProcs is only called once (since I believe the typical > > > caller > > > will skip validContext and skip to createContext), but we still have a > useful > > > validContext call (in case we want to assert, for example, or in > > > resetShaderContext > > > - are there other reasons we call it?). > > > > Yes, that looks reasonable. The only caveat is that it requires a decent copy > > constructor for SkBitmapProcState which currently doesn't exist. Simply > copying > > each element doesn't work. One problem is, for example, that when copying > > fOrigBitmap and fScaledBitmap we don't copy the pixel refs (I believe). > > fBitmapFilter also looks problematic. > > > > I don't really understand that code very well, so if you have any insights > > please let me know. Otherwise I'll have a closer look next week. > > I also don't have much time to look at this until next week. As for copying > fOrigBitmap and fScaledBitmap, I'm assuming we don't want to do a deep copy (the > copy constructor for SkBitmap will ref the pixel ref). You're right though, I'm > sure the copy constructor will be complicated. > > On the other hand, we could pass a (SkBitmapProcState) pointer to the > (BitmapProcShader) constructor, and have no need to write the copy constructor. > We can skip the extra call to new by putting it in the storage space. Good idea. I'm now placing the SkBitmapProcState on the storage behind the context. The context just holds a pointer to it. https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:110: if (totalInverse == NULL) { On 2014/03/28 20:50:08, scroggo wrote: > nit: Prefer putting NULL on the left of this check. Done. https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:123: if (!state->chooseProcs(*totalInverse, paint)) { On 2014/03/28 20:50:08, scroggo wrote: > return state->chooseProcs(*totalInverse, paint); Done. https://codereview.chromium.org/207683004/diff/100001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:145: return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, device, paint, matrix, state)); On 2014/03/28 20:50:08, scroggo wrote: > nit: 100 chars. Done. https://codereview.chromium.org/207683004/diff/100001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/100001/src/core/SkShader.cpp#n... src/core/SkShader.cpp:66: SkAssertResult(fShader.computeTotalInverse(matrix, &fTotalInverse)); On 2014/03/28 20:50:08, scroggo wrote: > This looks much better. A comment would be nice, explaining why we know we can > assert this returns true. Done.
https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:202: fState->~SkBitmapProcState(); A comment explaining why we treat it this way would be nice. https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:44: // The context takes ownership of the state. This could use some elaboration - it will call the state's destructor, but it does not free the memory. Should this constructor perhaps take a function pointer for destruction so we don't force this ownership behavior? https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:83: typedef SkSmallAllocator<3, sizeof(SkBitmapProcShader) + Won't this need to be 4 to account for the SkBitmapProcState?
https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:83: typedef SkSmallAllocator<3, sizeof(SkBitmapProcShader) + On 2014/04/01 18:02:41, scroggo wrote: > Won't this need to be 4 to account for the SkBitmapProcState? My mistake; you put the SkBitmapProcState in with the BitmapProcShaderContext. On the other hand, if you did separate it (in which case this would need to be 4), you could eliminate the contract of BitmapProcShaderContext: that it must call SkBitmapProcState's destructor (but not free its memory).
I've updated SkGradientShader. The class holds a cache, which I moved to the context, so we may miss out on some of the benefits. I'm not sure how much though. Will probably have to do some benchmarking. All tests, gms, and benches are now working apart from the tinybitmap gm which seg faults (not sure why yet). I didn't have time to clean everything up yet, but I thought I'd rather let you have a look now than wait for tomorrow. https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:202: fState->~SkBitmapProcState(); On 2014/04/01 18:02:41, scroggo wrote: > A comment explaining why we treat it this way would be nice. Done. https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:44: // The context takes ownership of the state. On 2014/04/01 18:02:41, scroggo wrote: > This could use some elaboration - it will call the state's destructor, but it > does not free the memory. > > Should this constructor perhaps take a function pointer for destruction so we > don't force this ownership behavior? I've extended the comment for now. If we passed a function pointer for destruction, would NULL mean, the caller will destroy it? https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:83: typedef SkSmallAllocator<3, sizeof(SkBitmapProcShader) + On 2014/04/02 16:03:14, scroggo wrote: > On 2014/04/01 18:02:41, scroggo wrote: > > Won't this need to be 4 to account for the SkBitmapProcState? > > My mistake; you put the SkBitmapProcState in with the BitmapProcShaderContext. > On the other hand, if you did separate it (in which case this would need to be > 4), you could eliminate the contract of BitmapProcShaderContext: that it must > call SkBitmapProcState's destructor (but not free its memory). We don't have access to the SkSmallAllocator when we create SkBitmapProcState (in createContext()), that's why I put it together with the context.
On 2014/04/02 18:00:48, Dominik Grewe wrote: > I've updated SkGradientShader. The class holds a cache, which I moved to the > context, so we may miss out on some of the benefits. I'm not sure how much > though. Will probably have to do some benchmarking. Agreed. Unfortunately I can't think of an external place to store the cache :( Some kind of singleton map of (alpha, shader) to cache? > > All tests, gms, and benches are now working apart from the tinybitmap gm which > seg faults (not sure why yet). That likely means an error in SkBitmapProcShader/State, since it calls CreateBitmapShader. https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:44: // The context takes ownership of the state. On 2014/04/02 18:00:48, Dominik Grewe wrote: > On 2014/04/01 18:02:41, scroggo wrote: > > This could use some elaboration - it will call the state's destructor, but it > > does not free the memory. > > > > Should this constructor perhaps take a function pointer for destruction so we > > don't force this ownership behavior? > > I've extended the comment for now. If we passed a function pointer for > destruction, would NULL mean, the caller will destroy it? I think so. We might have some examples of such a calling pattern elsewhere... https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:83: typedef SkSmallAllocator<3, sizeof(SkBitmapProcShader) + On 2014/04/02 18:00:48, Dominik Grewe wrote: > On 2014/04/02 16:03:14, scroggo wrote: > > On 2014/04/01 18:02:41, scroggo wrote: > > > Won't this need to be 4 to account for the SkBitmapProcState? > > > > My mistake; you put the SkBitmapProcState in with the BitmapProcShaderContext. > > On the other hand, if you did separate it (in which case this would need to be > > 4), you could eliminate the contract of BitmapProcShaderContext: that it must > > call SkBitmapProcState's destructor (but not free its memory). > > We don't have access to the SkSmallAllocator when we create SkBitmapProcState > (in createContext()), that's why I put it together with the context. Ah, of course. Nvm. https://codereview.chromium.org/207683004/diff/140001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/140001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:2359: * This class is NOT immutable due to the setup() method being called repeatedly after Now that setup is on the context, isn't this comment no longer needed? https://codereview.chromium.org/207683004/diff/140001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:2597: static_cast<SkComposeShader::ComposeShaderContext*>(shaderContext); This code makes a lot of assumptions about the code above it. It'd be nice if we could add some asserts to verify that all these static casts are valid. For example, we could add in some debug code to assert that p.getShader() == the compose shader you created up above. It might be nice to also assert that the compose shader's shaderA is the triColorShader (although admittedly, there is not an API on SkComposeShader to get shaderA - perhaps add a debug only getter?). https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:226: size_t SkGradientShaderBase::contextSize() const { Is it possible to create an SkGradientShaderBase? Is there any reason to override this? https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:233: : INHERITED(shader, device, paint, matrix), fCache(this->getPaintAlpha()) nit: fCache should go on its own line. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:237: SkAssertResult(fDstToIndex.setConcat(shader.fPtsToUnit, inverse)); I don't think you can assert this result unless you've already called validContext. (And if I recall correctly, SkBlitter does not call it.) https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:295: // Invalidate the cache. I don't think this comment is appropriate, given that this is the constructor (so there was no cache before this). https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... File src/effects/gradients/SkTwoPointConicalGradient.h (right): https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:37: // Mutable part of TwoPtRadial. I find this comment confusing, since it's no longer a part of TwoPtRadial (it is its own thing). https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:38: struct TwoPtRadialContext { This should probably be moved to the cpp file. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:40: float fB, fDB; It appears that fIncX, fIncY, and fDB could be const? https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:41: const TwoPtRadial& fRec; nit: the variable names should be aligned.
On 2014/04/02 20:16:52, scroggo wrote: > On 2014/04/02 18:00:48, Dominik Grewe wrote: > > I've updated SkGradientShader. The class holds a cache, which I moved to the > > context, so we may miss out on some of the benefits. I'm not sure how much > > though. Will probably have to do some benchmarking. > > Agreed. Unfortunately I can't think of an external place to store the cache :( > > Some kind of singleton map of (alpha, shader) to cache? Yeah, I guess that should work. I'll look into it if it turns out the current implementation is too slow. > > > > > All tests, gms, and benches are now working apart from the tinybitmap gm which > > seg faults (not sure why yet). > > That likely means an error in SkBitmapProcShader/State, since it calls > CreateBitmapShader. Turns out it was due to your attempt of making SkColorShader fields const. We need the constructor taking the buffer, so we can call the parent's constructor. Otherwise we don't read the parent's fields.
https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:44: // The context takes ownership of the state. On 2014/04/02 20:16:52, scroggo wrote: > On 2014/04/02 18:00:48, Dominik Grewe wrote: > > On 2014/04/01 18:02:41, scroggo wrote: > > > This could use some elaboration - it will call the state's destructor, but > it > > > does not free the memory. > > > > > > Should this constructor perhaps take a function pointer for destruction so > we > > > don't force this ownership behavior? > > > > I've extended the comment for now. If we passed a function pointer for > > destruction, would NULL mean, the caller will destroy it? > > I think so. We might have some examples of such a calling pattern elsewhere... On the other hand, we shouldn't create any context objects outside of createContext. We could try to enforce that. I first thought of making the classes protected, but that doesn't work because we need to access specific context classes in certain cases (so they have to be public). Another option would be to make the constructor protected and make the corresponding shader class a friend of the context class. Wdyt? https://codereview.chromium.org/207683004/diff/140001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/140001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:2359: * This class is NOT immutable due to the setup() method being called repeatedly after On 2014/04/02 20:16:53, scroggo wrote: > Now that setup is on the context, isn't this comment no longer needed? You're right. Thanks for spotting. https://codereview.chromium.org/207683004/diff/140001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:2597: static_cast<SkComposeShader::ComposeShaderContext*>(shaderContext); On 2014/04/02 20:16:53, scroggo wrote: > This code makes a lot of assumptions about the code above it. It'd be nice if we > could add some asserts to verify that all these static casts are valid. > > For example, we could add in some debug code to assert that p.getShader() == the > compose shader you created up above. > > It might be nice to also assert that the compose shader's shaderA is the > triColorShader (although admittedly, there is not an API on SkComposeShader to > get shaderA - perhaps add a debug only getter?). > Good point. I've added the asserts. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:226: size_t SkGradientShaderBase::contextSize() const { On 2014/04/02 20:16:53, scroggo wrote: > Is it possible to create an SkGradientShaderBase? Is there any reason to > override this? No. Removed. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:233: : INHERITED(shader, device, paint, matrix), fCache(this->getPaintAlpha()) On 2014/04/02 20:16:53, scroggo wrote: > nit: fCache should go on its own line. Done. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:237: SkAssertResult(fDstToIndex.setConcat(shader.fPtsToUnit, inverse)); On 2014/04/02 20:16:53, scroggo wrote: > I don't think you can assert this result unless you've already called > validContext. (And if I recall correctly, SkBlitter does not call it.) The constructor of SkShader::Context asserts that validContext returns true. (And createContext always calls validContext before creating a context.) https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:295: // Invalidate the cache. On 2014/04/02 20:16:53, scroggo wrote: > I don't think this comment is appropriate, given that this is the constructor > (so there was no cache before this). Maybe "invalidate" is the wrong term, but I wanted to say that the cache isn't initialized yet. I'll update the comment and add another on the class definition. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... File src/effects/gradients/SkTwoPointConicalGradient.h (right): https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:37: // Mutable part of TwoPtRadial. On 2014/04/02 20:16:53, scroggo wrote: > I find this comment confusing, since it's no longer a part of TwoPtRadial (it is > its own thing). Removed. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:38: struct TwoPtRadialContext { On 2014/04/02 20:16:53, scroggo wrote: > This should probably be moved to the cpp file. Done. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:40: float fB, fDB; On 2014/04/02 20:16:53, scroggo wrote: > It appears that fIncX, fIncY, and fDB could be const? Done. https://codereview.chromium.org/207683004/diff/140001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.h:41: const TwoPtRadial& fRec; On 2014/04/02 20:16:53, scroggo wrote: > nit: the variable names should be aligned. Done.
Besides some nits, this cl 1gtm. I'm sure reed@ will want to look over it as well. https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/120001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:44: // The context takes ownership of the state. On 2014/04/03 12:12:59, Dominik Grewe wrote: > On 2014/04/02 20:16:52, scroggo wrote: > > On 2014/04/02 18:00:48, Dominik Grewe wrote: > > > On 2014/04/01 18:02:41, scroggo wrote: > > > > This could use some elaboration - it will call the state's destructor, but > > it > > > > does not free the memory. > > > > > > > > Should this constructor perhaps take a function pointer for destruction so > > we > > > > don't force this ownership behavior? > > > > > > I've extended the comment for now. If we passed a function pointer for > > > destruction, would NULL mean, the caller will destroy it? > > > > I think so. We might have some examples of such a calling pattern elsewhere... > > On the other hand, we shouldn't create any context objects outside of > createContext. We could try to enforce that. I first thought of making the > classes protected, but that doesn't work because we need to access specific > context classes in certain cases (so they have to be public). > Another option would be to make the constructor protected and make the > corresponding shader class a friend of the context class. Wdyt? I think that makes a lot of sense. https://codereview.chromium.org/207683004/diff/200001/include/core/SkComposeS... File include/core/SkComposeShader.h (right): https://codereview.chromium.org/207683004/diff/200001/include/core/SkComposeS... include/core/SkComposeShader.h:45: // Takes ownership of contextA and contextB, and calls their destructors. nit: As with the bitmap proc shader/state, I think ownership implies that it owns the memory, but here we only call the destructor, but the memory is freed elsewhere. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:156: : INHERITED(shader, device, paint, matrix), fState(state) nit: fState should go on its own line. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:70: uint8_t fTileModeX, fTileModeY; nit: Spacing. fTileModeX should line up with fRawBitmap. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:84: typedef SkSmallAllocator<3, sizeof(SkBitmapProcShader) + We may want to try different sizes, to make sure we have the optimal size here. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:630: // Takes ownership of proxyContext and calls its destructor. Once again, I think this is not pure ownership. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:817: (shader->getFlags() & SkShader::kOpaqueAlpha_Flag)*/) { Here are the shaders that potentially return kOpaqueAlpha_Flag: SkBitmapProcShader (after setContext) SkColorShader (after setContext) SkTransparentShader - this one is interesting because if it had been called before setContext, its results would have been undefined! It calls into fDevice to determine whether to return the flag, but fDevice is not set to NULL in the constructor, so it could be garbage. We probably never ran into this because SkTransparentShader is only used by our sample code. (This code also only gets called if there is an xfermode, lessening the likelihood it might be called.) SkGradientShader (after setContext) SkTwoPointConicalGradient *clears* the flag (in setContext) All of the shaders only set the flag after setContext (further, they could not know a meaningful value without knowing the paint passed to setContext). So by removing the check for kOpaqueAlpha_Flag, you're not actually changing the behavior any. So I think it's okay to remove the comment and make the check just if (NULL == shader). https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:993: blitter = allocator->createT<SkARGB32_Shader_Blitter>(device, *paint, shaderContext); nit: 100 chars. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:1028: : INHERITED(device), fShader(paint.getShader()), fShaderContext(shaderContext) { nit: fShader and fShaderContext should be on their own lines. https://codereview.chromium.org/207683004/diff/200001/src/core/SkCoreBlitters.h File src/core/SkCoreBlitters.h (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkCoreBlitters... src/core/SkCoreBlitters.h:30: // TODO(dominikg): Should this class have its own SkSmallAllocator for storing the Didn't you determine that this wouldn't work without some more refactoring since we create the context first? https://codereview.chromium.org/207683004/diff/200001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:2528: SkAutoTUnref<SkComposeShader> composeShader; This should be in SkDEBUGCODE, since we only need it for SkASSERT. https://codereview.chromium.org/207683004/diff/200001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkShader.cpp#n... src/core/SkShader.cpp:340: #include "SkEmptyShader.h" nit: This could be inside SK_IGNORE_TO_STRING, now that the other SkEmptyShader functions are gone. https://codereview.chromium.org/207683004/diff/200001/src/effects/SkPerlinNoi... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:9: #include "SkPerlinNoiseShader.h" The perlin noise shader changes look fine to me, but you might want to run them by senorblanco and/or sugoi. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:549: * over and over, we'd like to return exactly the same "bitmap" if possible, Now that the cache is not on the SkGradientShader, we won't be returning the same bitmap (it will be logically the same, but it will be a different bitmap with a different genID). (That is of course, unless we use some sort of external cache.) https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:115: uint16_t* fCache16; // working ptr. If this is NULL, we need to recompute the cache values nit: over 100 chars https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkTwoPointConicalGradient.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.cpp:214: SkShader::Context* SkTwoPointConicalGradient::createContext(const SkBitmap& device, const SkPaint& paint, nit: 100 chars. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.cpp:220: return SkNEW_PLACEMENT_ARGS(storage, TwoPointConicalGradientContext, (*this, device, paint, matrix)); 100 chars. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkTwoPointRadialGradient.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkTwoPointRadialGradient.cpp:244: return SkNEW_PLACEMENT_ARGS(storage, TwoPointRadialGradientContext, (*this, device, paint, matrix)); 100 chars.
Bikeshedding ahoy: to me a Context is an inert data class (essentially a struct) that's passed to each of the processing functions, which should remain on the shader. The way this is currently done, I'd say these are more like Impls. (On the other hand, they are exposed in the shader's public API, which is kind of odd for an Impl too.) What would it look like to have a Context be data holder that's passed to the shader? Would that work? It would require a downcast in each of the processing functions, and accessors to get the data out. That would bring it in line with the way SkImageFilter does things, though. https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... File include/core/SkColorShader.h (right): https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... include/core/SkColorShader.h:51: virtual void shadeSpan(int x, int y, SkPMColor span[], int count) SK_OVERRIDE; It seems a little odd to me that the actual processing is performed on the Context. Would it make sense to leave processing on the shader, and simply have it take a Context? That would require a context downcast in each subclass I suppose.
On 2014/04/03 16:08:57, Stephen White wrote: > Bikeshedding ahoy: to me a Context is an inert data class (essentially a struct) > that's passed to each of the processing functions, which should remain on the > shader. The way this is currently done, I'd say these are more like Impls. (On > the other hand, they are exposed in the shader's public API, which is kind of > odd for an Impl too.) I think calling it an Impl is logical. (In one version of this change, I called the Context Impl, and renamed the Shader to ShaderGenerator.) As for showing up in the API, we could move them to protected with friends for the classes that actually need them. https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... File include/core/SkColorShader.h (right): https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... include/core/SkColorShader.h:51: virtual void shadeSpan(int x, int y, SkPMColor span[], int count) SK_OVERRIDE; On 2014/04/03 16:08:57, Stephen White wrote: > It seems a little odd to me that the actual processing is performed on the > Context. Would it make sense to leave processing on the shader, and simply have > it take a Context? That would require a context downcast in each subclass I > suppose. At one point I considered that approach. I think I moved away from that for a couple of reasons: - It means we have to hang on to pointers to two objects, and use them in tandem (although I think ultimately in this approach we hang on to the shader in order to call resetShaderContext). The context does hang on to its shader, but only as a const reference. - What does it mean if I passed the wrong context to a shader? (This probably wouldn't happen, but the API allows you to provide different context objects.) - It seemed like most of the information I wanted to process was on the context (though some is on the shader - I haven't actually counted all the callers), so it seemed like an extra hassle to have every function read all its info off the passed in object. All that said, I'm not opposed in principle to doing the processing in the shader; I just thought this way was more convenient.
On 2014/04/03 16:32:34, scroggo wrote: > On 2014/04/03 16:08:57, Stephen White wrote: > > Bikeshedding ahoy: to me a Context is an inert data class (essentially a > struct) > > that's passed to each of the processing functions, which should remain on the > > shader. The way this is currently done, I'd say these are more like Impls. (On > > the other hand, they are exposed in the shader's public API, which is kind of > > odd for an Impl too.) > > I think calling it an Impl is logical. (In one version of this change, I called > the > Context Impl, and renamed the Shader to ShaderGenerator.) As for showing up in > the > API, we could move them to protected with friends for the classes that actually > need them. We used the term Context in a very similar way in SkDrawLooper, that's why I also called it this way here. I think we could make the sub-classes of SkShader::Context protected, but SkShader::Context itself has to stay public because the user of SkShader has to hold one. I'm not sure Impl would be a better name, because it's not an implementation of an interface or something like that. It's an object that works together with another object (the shader in this case). > > https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... > File include/core/SkColorShader.h (right): > > https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... > include/core/SkColorShader.h:51: virtual void shadeSpan(int x, int y, SkPMColor > span[], int count) SK_OVERRIDE; > On 2014/04/03 16:08:57, Stephen White wrote: > > It seems a little odd to me that the actual processing is performed on the > > Context. Would it make sense to leave processing on the shader, and simply > have > > it take a Context? That would require a context downcast in each subclass I > > suppose. > > At one point I considered that approach. I think I moved away from that for a > couple of reasons: > > - It means we have to hang on to pointers to two objects, and use them in tandem > (although I think ultimately in this approach we hang on to the shader in order > to call resetShaderContext). The context does hang on to its shader, but only as > a const reference. > > - What does it mean if I passed the wrong context to a shader? (This probably > wouldn't happen, but the API allows you to provide different context objects.) > > - It seemed like most of the information I wanted to process was on the context > (though some is on the shader - I haven't actually counted all the callers), so > it seemed like an extra hassle to have every function read all its info off the > passed in object. > > All that said, I'm not opposed in principle to doing the processing in the > shader; I just thought this way was more convenient. I don't have a strong opinion either way, but as Leon said, it's more convenient this way.
On 2014/04/03 17:41:39, Dominik Grewe wrote: > On 2014/04/03 16:32:34, scroggo wrote: > > On 2014/04/03 16:08:57, Stephen White wrote: > > > Bikeshedding ahoy: to me a Context is an inert data class (essentially a > > struct) > > > that's passed to each of the processing functions, which should remain on > the > > > shader. The way this is currently done, I'd say these are more like Impls. > (On > > > the other hand, they are exposed in the shader's public API, which is kind > of > > > odd for an Impl too.) > > > > I think calling it an Impl is logical. (In one version of this change, I > called > > the > > Context Impl, and renamed the Shader to ShaderGenerator.) As for showing up in > > the > > API, we could move them to protected with friends for the classes that > actually > > need them. > > We used the term Context in a very similar way in SkDrawLooper, that's why I > also called it this way here. > I think we could make the sub-classes of SkShader::Context protected, but > SkShader::Context itself has to stay public because the user of SkShader has to > hold one. > I'm not sure Impl would be a better name, because it's not an implementation of > an interface or something like that. It's an object that works together with > another object (the shader in this case). > > > > > > https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... > > File include/core/SkColorShader.h (right): > > > > > https://codereview.chromium.org/207683004/diff/200001/include/core/SkColorSha... > > include/core/SkColorShader.h:51: virtual void shadeSpan(int x, int y, > SkPMColor > > span[], int count) SK_OVERRIDE; > > On 2014/04/03 16:08:57, Stephen White wrote: > > > It seems a little odd to me that the actual processing is performed on the > > > Context. Would it make sense to leave processing on the shader, and simply > > have > > > it take a Context? That would require a context downcast in each subclass I > > > suppose. > > > > At one point I considered that approach. I think I moved away from that for a > > couple of reasons: > > > > - It means we have to hang on to pointers to two objects, and use them in > tandem > > (although I think ultimately in this approach we hang on to the shader in > order > > to call resetShaderContext). The context does hang on to its shader, but only > as > > a const reference. > > > > - What does it mean if I passed the wrong context to a shader? (This probably > > wouldn't happen, but the API allows you to provide different context objects.) > > > > - It seemed like most of the information I wanted to process was on the > context > > (though some is on the shader - I haven't actually counted all the callers), > so > > it seemed like an extra hassle to have every function read all its info off > the > > passed in object. > > > > All that said, I'm not opposed in principle to doing the processing in the > > shader; I just thought this way was more convenient. > > I don't have a strong opinion either way, but as Leon said, it's more convenient > this way. OK, you've convinced me on the name. :) I'll leave the processing API decision up to the Deciders.
I've done some benchmarking on my phone (seemed more stable than my Linux workstation) using bench_pictures with the buildbot SKPs. Performance has gone down slightly. On average it's <1%, but in can be as high as 8%. The cache performance of the gradient shader cache has gone down slightly, as expected. The hit rate of getCache32 is 99.78% on master and 98.75% with this patch. getCache16 was never called. Not sure if the caching explains all of the performance drop or if other factors (extra computation to validate context params) are also to blame. Might dig a little deeper into the slow cases. Any other things I should test? I've never run a Skia perf trybot. Is it useful for finding regressions?
On 2014/04/03 17:55:53, Dominik Grewe wrote: > I've done some benchmarking on my phone (seemed more stable than my Linux > workstation) using bench_pictures with the buildbot SKPs. Performance has gone > down slightly. On average it's <1%, but in can be as high as 8%. Good to know. It also might be interesting to test our micro benchmarks, although I'm not sure if we have any that test the shaders specifically. In the end, the performance drop may be worth it, if it allows us to speed up recording time and play back multithreaded without cloning a picture. Not sure how to measure all that (especially since there are other obstacles to the multithreaded playback). > > The cache performance of the gradient shader cache has gone down slightly, as > expected. The hit rate of getCache32 is 99.78% on master and 98.75% with this > patch. How many repetitions did you use (the default appears to be one)? When drawing the SKP a second time, I would expect that with caching, the hit rate should be 100% - only counting the second iteration - without this change, and lower the second time with this change. > getCache16 was never called. That will only be used in 565. I don't think we support drawing to 565 in bench_pictures. > > Not sure if the caching explains all of the performance drop or if other factors > (extra computation to validate context params) are also to blame. Might dig a > little deeper into the slow cases. > > Any other things I should test? > I've never run a Skia perf trybot. Is it useful for finding regressions? I have not either, so I'm not sure.
On 2014/04/03 18:20:51, scroggo wrote: > On 2014/04/03 17:55:53, Dominik Grewe wrote: > > I've done some benchmarking on my phone (seemed more stable than my Linux > > workstation) using bench_pictures with the buildbot SKPs. Performance has gone > > down slightly. On average it's <1%, but in can be as high as 8%. > > Good to know. It also might be interesting to test our micro benchmarks, > although > I'm not sure if we have any that test the shaders specifically. > > In the end, the performance drop may be worth it, if it allows us to speed up > recording time and play back multithreaded without cloning a picture. Not sure > how to measure all that (especially since there are other obstacles to the > multithreaded playback). > > > > > The cache performance of the gradient shader cache has gone down slightly, as > > expected. The hit rate of getCache32 is 99.78% on master and 98.75% with this > > patch. > > How many repetitions did you use (the default appears to be one)? When drawing > the SKP a second time, I would expect that with caching, the hit rate should be > 100% - only counting the second iteration - without this change, and lower the > second time with this change. To collect the cache statistics I just used repeat=1. Not sure if it'll effect the number; depends on whether the shaders are being created from scratch each time. With this patch the numbers shouldn't change, because we always create a new context with a new cache, so repetitions shouldn't matter. For the performance numbers I used repeat=10.
On 2014/04/03 19:01:24, Dominik Grewe wrote: > On 2014/04/03 18:20:51, scroggo wrote: > > On 2014/04/03 17:55:53, Dominik Grewe wrote: > > > I've done some benchmarking on my phone (seemed more stable than my Linux > > > workstation) using bench_pictures with the buildbot SKPs. Performance has > gone > > > down slightly. On average it's <1%, but in can be as high as 8%. > > > > Good to know. It also might be interesting to test our micro benchmarks, > > although > > I'm not sure if we have any that test the shaders specifically. > > > > In the end, the performance drop may be worth it, if it allows us to speed up > > recording time and play back multithreaded without cloning a picture. Not sure > > how to measure all that (especially since there are other obstacles to the > > multithreaded playback). > > > > > > > > The cache performance of the gradient shader cache has gone down slightly, > as > > > expected. The hit rate of getCache32 is 99.78% on master and 98.75% with > this > > > patch. > > > > How many repetitions did you use (the default appears to be one)? When drawing > > the SKP a second time, I would expect that with caching, the hit rate should > be > > 100% - only counting the second iteration - without this change, and lower the > > second time with this change. > > To collect the cache statistics I just used repeat=1. Not sure if it'll effect > the number; depends on whether the shaders are being created from scratch each > time. Each iteration of bench_pictures uses the same SkPicture, so the shaders will not be created from scratch each time. I expect you'll get higher numbers with more repeats without this CL (though admittedly 99.78% is already pretty high). > With this patch the numbers shouldn't change, because we always create a > new context with a new cache, so repetitions shouldn't matter. Agreed. > For the performance numbers I used repeat=10.
Thanks for the comments scroggo. @senorblanco: Could you have a look at SkPerlinNoiseShader? https://codereview.chromium.org/207683004/diff/200001/include/core/SkComposeS... File include/core/SkComposeShader.h (right): https://codereview.chromium.org/207683004/diff/200001/include/core/SkComposeS... include/core/SkComposeShader.h:45: // Takes ownership of contextA and contextB, and calls their destructors. On 2014/04/03 15:35:54, scroggo wrote: > nit: As with the bitmap proc shader/state, I think ownership implies that it > owns the memory, but here we only call the destructor, but the memory is freed > elsewhere. Comment updated. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.cpp:156: : INHERITED(shader, device, paint, matrix), fState(state) On 2014/04/03 15:35:54, scroggo wrote: > nit: fState should go on its own line. Done. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... File src/core/SkBitmapProcShader.h (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:70: uint8_t fTileModeX, fTileModeY; On 2014/04/03 15:35:54, scroggo wrote: > nit: Spacing. fTileModeX should line up with fRawBitmap. Done. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBitmapProcSh... src/core/SkBitmapProcShader.h:84: typedef SkSmallAllocator<3, sizeof(SkBitmapProcShader) + On 2014/04/03 15:35:54, scroggo wrote: > We may want to try different sizes, to make sure we have the optimal size here. It seems we can get away with counting the BitmapProcShaderContext size just once. Tested with out/Debug/dm and out/Debug/bench_pictures (on buildbot SKPs). https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp File src/core/SkBlitter.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:630: // Takes ownership of proxyContext and calls its destructor. On 2014/04/03 15:35:54, scroggo wrote: > Once again, I think this is not pure ownership. Done. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:817: (shader->getFlags() & SkShader::kOpaqueAlpha_Flag)*/) { On 2014/04/03 15:35:54, scroggo wrote: > Here are the shaders that potentially return kOpaqueAlpha_Flag: > > SkBitmapProcShader (after setContext) > SkColorShader (after setContext) > SkTransparentShader - this one is interesting because if it had been called > before setContext, its results would have been undefined! It calls into fDevice > to determine whether to return the flag, but fDevice is not set to NULL in the > constructor, so it could be garbage. We probably never ran into this because > SkTransparentShader is only used by our sample code. (This code also only gets > called if there is an xfermode, lessening the likelihood it might be called.) That just shows that getFlags shouldn't be called before setContext, which cannot happen with this patch anyway. > SkGradientShader (after setContext) > SkTwoPointConicalGradient *clears* the flag (in setContext) > > All of the shaders only set the flag after setContext (further, they could not > know a meaningful value without knowing the paint passed to setContext). So by > removing the check for kOpaqueAlpha_Flag, you're not actually changing the > behavior any. So I think it's okay to remove the comment and make the check just > if (NULL == shader). Done. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:993: blitter = allocator->createT<SkARGB32_Shader_Blitter>(device, *paint, shaderContext); On 2014/04/03 15:35:54, scroggo wrote: > nit: 100 chars. Done. https://codereview.chromium.org/207683004/diff/200001/src/core/SkBlitter.cpp#... src/core/SkBlitter.cpp:1028: : INHERITED(device), fShader(paint.getShader()), fShaderContext(shaderContext) { On 2014/04/03 15:35:54, scroggo wrote: > nit: fShader and fShaderContext should be on their own lines. Done. https://codereview.chromium.org/207683004/diff/200001/src/core/SkCoreBlitters.h File src/core/SkCoreBlitters.h (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkCoreBlitters... src/core/SkCoreBlitters.h:30: // TODO(dominikg): Should this class have its own SkSmallAllocator for storing the On 2014/04/03 15:35:54, scroggo wrote: > Didn't you determine that this wouldn't work without some more refactoring since > we create the context first? Yes, you're right. I'll remove this. https://codereview.chromium.org/207683004/diff/200001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:2528: SkAutoTUnref<SkComposeShader> composeShader; On 2014/04/03 15:35:54, scroggo wrote: > This should be in SkDEBUGCODE, since we only need it for SkASSERT. I also use it below though (outside of the assert). I just moved this declaration outside of the if to make sure it's still in scope for the assert. https://codereview.chromium.org/207683004/diff/200001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/core/SkShader.cpp#n... src/core/SkShader.cpp:340: #include "SkEmptyShader.h" On 2014/04/03 15:35:54, scroggo wrote: > nit: This could be inside SK_IGNORE_TO_STRING, now that the other SkEmptyShader > functions are gone. Done. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:549: * over and over, we'd like to return exactly the same "bitmap" if possible, On 2014/04/03 15:35:54, scroggo wrote: > Now that the cache is not on the SkGradientShader, we won't be returning the > same bitmap (it will be logically the same, but it will be a different bitmap > with a different genID). (That is of course, unless we use some sort of external > cache.) I think the comment is referring to the SkBitmapCache defined as a static variable ('gCache') below. Access to it is guarded by a mutex, so we should be good. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:115: uint16_t* fCache16; // working ptr. If this is NULL, we need to recompute the cache values On 2014/04/03 15:35:54, scroggo wrote: > nit: over 100 chars Done. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkTwoPointConicalGradient.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.cpp:214: SkShader::Context* SkTwoPointConicalGradient::createContext(const SkBitmap& device, const SkPaint& paint, On 2014/04/03 15:35:54, scroggo wrote: > nit: 100 chars. Done. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkTwoPointConicalGradient.cpp:220: return SkNEW_PLACEMENT_ARGS(storage, TwoPointConicalGradientContext, (*this, device, paint, matrix)); On 2014/04/03 15:35:54, scroggo wrote: > 100 chars. Done. https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... File src/effects/gradients/SkTwoPointRadialGradient.cpp (right): https://codereview.chromium.org/207683004/diff/200001/src/effects/gradients/S... src/effects/gradients/SkTwoPointRadialGradient.cpp:244: return SkNEW_PLACEMENT_ARGS(storage, TwoPointRadialGradientContext, (*this, device, paint, matrix)); On 2014/04/03 15:35:54, scroggo wrote: > 100 chars. Done.
On 2014/04/04 10:59:41, Dominik Grewe wrote: > Thanks for the comments scroggo. > > @senorblanco: Could you have a look at SkPerlinNoiseShader? Looks good.
I had a closer look at the performance of the 8% slowdown case. Most of it seems to be down to the cache hit rate (~90% vs 99.9% on master). Putting the cache back onto the shader (and thereby breaking const-ness) reduces the slowdown to ~2-3%. Possible ways to improve cache performance: 1) Move the cache back to the shader but make it thread-safe. We'd have to extend the cache to support multiple alpha values at a time and then use locks to synchronize. 2) tomhudson@ was wondering if we could have an unpremultiplied cache on the shader and then store a copy, multiplied by alpha, on the context (if alpha != 255). It seems this would work, but I'm not 100% sure the math adds up. I'm not really able to test it because the dm tests don't seem to be affected by this (I can use random values for the colour values and the tests still pass). Any other ideas? Do you think this needs to be fixed before landing or would it be okay to land this as is and try to improve performance in a follow-up?
I noticed that in src/core/SkBitmapShaderTemplate.h we define a class that looks suspiciously like a shader class. But I'm not really sure if it is indeed a subclass of SkShader... It inherits from HasSpan16_Sampler_BitmapShader which I can't find the definition of. Everything compiles fine without making changes to that class, so I'm not sure it's being used anywhere. Any ideas?
On 2014/04/07 13:36:14, Dominik Grewe wrote: > I noticed that in src/core/SkBitmapShaderTemplate.h we define a class that looks > suspiciously like a shader class. But I'm not really sure if it is indeed a > subclass of SkShader... It inherits from HasSpan16_Sampler_BitmapShader which I > can't find the definition of. > > Everything compiles fine without making changes to that class, so I'm not sure > it's being used anywhere. Any ideas? I say delete that file along with src/core/SkBitmapShader16BilerpTemplate.h. Only one of them has a CL besides "grab latest from Android" and some automatic cleanups, and the one CL that was not an automatic cleanup was a manual cleanup, which deleted HasSpan16_Sampler_BitmapShader - this was in r271. Deleting those files should probably be a separate CL though.
https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h... include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. I know I originally wrote this, and this is what we were thinking at the time, but I've talked it over mtklein, and we no longer think this will move to SkPaint. See https://docs.google.com/a/google.com/document/d/13HHTH6O9QL_gTBNBRZCBzCP8JwsG...
On 2014/04/07 13:44:03, scroggo wrote: > On 2014/04/07 13:36:14, Dominik Grewe wrote: > > I noticed that in src/core/SkBitmapShaderTemplate.h we define a class that > looks > > suspiciously like a shader class. But I'm not really sure if it is indeed a > > subclass of SkShader... It inherits from HasSpan16_Sampler_BitmapShader which > I > > can't find the definition of. > > > > Everything compiles fine without making changes to that class, so I'm not sure > > it's being used anywhere. Any ideas? > > I say delete that file along with src/core/SkBitmapShader16BilerpTemplate.h. > Only > one of them has a CL besides "grab latest from Android" and some automatic > cleanups, > and the one CL that was not an automatic cleanup was a manual cleanup, which > deleted > HasSpan16_Sampler_BitmapShader - this was in r271. > > Deleting those files should probably be a separate CL though. Okay, I'll create a new CL.
https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h... include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. On 2014/04/07 13:44:31, scroggo wrote: > I know I originally wrote this, and this is what we were thinking at the time, > but I've talked it over mtklein, and we no longer think this will move to > SkPaint. See > https://docs.google.com/a/google.com/document/d/13HHTH6O9QL_gTBNBRZCBzCP8JwsG... Okay, cool. I'll delete the comment (and the ones below). Were you planning to make the changes proposed in the doc any time soon? Or would you mind me having a go?
On 2014/04/07 14:04:52, Dominik Grewe wrote: > https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h > File include/core/SkShader.h (right): > > https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h... > include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. > On 2014/04/07 13:44:31, scroggo wrote: > > I know I originally wrote this, and this is what we were thinking at the time, > > but I've talked it over mtklein, and we no longer think this will move to > > SkPaint. See > > > https://docs.google.com/a/google.com/document/d/13HHTH6O9QL_gTBNBRZCBzCP8JwsG... > > Okay, cool. I'll delete the comment (and the ones below). > Were you planning to make the changes proposed in the doc any time soon? Or > would you mind me having a go? By all means, go ahead! Please put mtklein and me on the review.
On 2014/04/07 14:11:53, scroggo wrote: > On 2014/04/07 14:04:52, Dominik Grewe wrote: > > https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h > > File include/core/SkShader.h (right): > > > > > https://codereview.chromium.org/207683004/diff/220001/include/core/SkShader.h... > > include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. > > On 2014/04/07 13:44:31, scroggo wrote: > > > I know I originally wrote this, and this is what we were thinking at the > time, > > > but I've talked it over mtklein, and we no longer think this will move to > > > SkPaint. See > > > > > > https://docs.google.com/a/google.com/document/d/13HHTH6O9QL_gTBNBRZCBzCP8JwsG... > > > > Okay, cool. I'll delete the comment (and the ones below). > > Were you planning to make the changes proposed in the doc any time soon? Or > > would you mind me having a go? > > By all means, go ahead! Please put mtklein and me on the review. Will do. Probably best to get this landed first.
+reed@ for review.
https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h... include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. ... or to the constructor(s) e.g. new gradient(..., matrix) and/or Shader* CloneShader(shader, newMatrix) https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h... include/core/SkShader.h:224: virtual size_t contextSize() const = 0; I wonder if we ever want to see the params (e.g. device, paint, matrix) when answering the question about size... https://codereview.chromium.org/207683004/diff/240001/src/core/SkBlitter.h File src/core/SkBlitter.h (right): https://codereview.chromium.org/207683004/diff/240001/src/core/SkBlitter.h#ne... src/core/SkBlitter.h:67: virtual bool resetShaderContext(const SkBitmap& device, const SkPaint& paint, where are these two called?
https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h... include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. On 2014/04/07 15:35:42, reed1 wrote: > ... or to the constructor(s) > > e.g. > > new gradient(..., matrix) > > and/or > > Shader* CloneShader(shader, newMatrix) > Yes, meant to remove this comment and then handle this in a follow-up patch. https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h... include/core/SkShader.h:224: virtual size_t contextSize() const = 0; On 2014/04/07 15:35:42, reed1 wrote: > I wonder if we ever want to see the params (e.g. device, paint, matrix) when > answering the question about size... We're currently relying on the fact that for a given SkShader instance the context size is the same regardless of the params. This allows us to replace a context with one from the same shader but created with different params in the same pre-allocated storage. If this isn't flexible enough we'd have to come up with something different in those cases. https://codereview.chromium.org/207683004/diff/240001/src/core/SkBlitter.h File src/core/SkBlitter.h (right): https://codereview.chromium.org/207683004/diff/240001/src/core/SkBlitter.h#ne... src/core/SkBlitter.h:67: virtual bool resetShaderContext(const SkBitmap& device, const SkPaint& paint, On 2014/04/07 15:35:42, reed1 wrote: > where are these two called? SkDraw::drawVertices() (src/core/SkDraw.cpp:2572 & 2581)
On 2014/04/07 15:44:52, Dominik Grewe wrote: > https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h > File include/core/SkShader.h (right): > > https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h... > include/core/SkShader.h:41: * FIXME: local matrix is moving to SkPaint. > On 2014/04/07 15:35:42, reed1 wrote: > > ... or to the constructor(s) > > > > e.g. > > > > new gradient(..., matrix) > > > > and/or > > > > Shader* CloneShader(shader, newMatrix) > > > > Yes, meant to remove this comment and then handle this in a follow-up patch. > > https://codereview.chromium.org/207683004/diff/240001/include/core/SkShader.h... > include/core/SkShader.h:224: virtual size_t contextSize() const = 0; > On 2014/04/07 15:35:42, reed1 wrote: > > I wonder if we ever want to see the params (e.g. device, paint, matrix) when > > answering the question about size... > > We're currently relying on the fact that for a given SkShader instance the > context size is the same regardless of the params. This allows us to replace a > context with one from the same shader but created with different params in the > same pre-allocated storage. > If this isn't flexible enough we'd have to come up with something different in > those cases. > > https://codereview.chromium.org/207683004/diff/240001/src/core/SkBlitter.h > File src/core/SkBlitter.h (right): > > https://codereview.chromium.org/207683004/diff/240001/src/core/SkBlitter.h#ne... > src/core/SkBlitter.h:67: virtual bool resetShaderContext(const SkBitmap& device, > const SkPaint& paint, > On 2014/04/07 15:35:42, reed1 wrote: > > where are these two called? > > > SkDraw::drawVertices() (src/core/SkDraw.cpp:2572 & 2581) lgtm* You still need reed@'s approval (at least for the API changes).
change looks as good as it can be for now, congrats. 1. Lets document (well) somewhere all of the still-to-do aspects of the larger change we have in mind... e.g. - not immutable until we have solved the local-matrix question - drawVertices is still weird, since it required a change to the base of SkBlitter 2. What is the performance hit given that we are not caching the gradient-table? - what are the bench/ timings? - what are our options to address this? (SK_ONCE?) - when the GPU is our backend, it is likely that we will *never* need the table!
On 2014/04/09 14:18:09, reed1 wrote: > change looks as good as it can be for now, congrats. > > 1. Lets document (well) somewhere all of the still-to-do aspects of the larger > change we have in mind... e.g. > - not immutable until we have solved the local-matrix question We're already actively discussing that on Leon's doc: https://docs.google.com/a/google.com/document/d/13HHTH6O9QL_gTBNBRZCBzCP8JwsG... > - drawVertices is still weird, since it required a change to the base of > SkBlitter I'll create a bug for cleaning this up. Are there any other issues with SkShader? > > 2. What is the performance hit given that we are not caching the gradient-table? > - what are the bench/ timings? Running bench_pictures on the buildbot SKPs on my N4 I see an average drop in performance of ~1% (maximum drop per page is 8%). > - what are our options to address this? (SK_ONCE?) The problem is that the cache is built for a given alpha, which can change for different contexts. So we can't just share a cache across threads once it's been built. Leon suggested having a cache that can store values for multiple alpha values. Tom was wondering if we can pre-compute the cache without any alphas and then simply multiply the values by the correct alpha value. We'd need for someone to check if that produces the correct output though. > - when the GPU is our backend, it is likely that we will *never* need the > table! That would obviously help :)
On 2014/04/09 15:59:51, Dominik Grewe wrote: > > 2. What is the performance hit given that we are not caching the > gradient-table? > > - what are the bench/ timings? > > Running bench_pictures on the buildbot SKPs on my N4 I see an average drop in > performance of ~1% (maximum drop per page is 8%). > > > - what are our options to address this? (SK_ONCE?) > > The problem is that the cache is built for a given alpha, which can change for > different contexts. So we can't just share a cache across threads once it's been > built. > Leon suggested having a cache that can store values for multiple alpha values. > Tom was wondering if we can pre-compute the cache without any alphas and then > simply multiply the values by the correct alpha value. We'd need for someone to > check if that produces the correct output though. Mike had another suggestion: the shader caches a version with the most recently used alpha, guarded by a mutex. The context will ref the cache, and then if another alpha needs to be cached, the shader drops the old cache, and any context with the old one still has the old cache.
On 2014/04/09 16:24:11, scroggo wrote: > On 2014/04/09 15:59:51, Dominik Grewe wrote: > > > 2. What is the performance hit given that we are not caching the > > gradient-table? > > > - what are the bench/ timings? > > > > Running bench_pictures on the buildbot SKPs on my N4 I see an average drop in > > performance of ~1% (maximum drop per page is 8%). > > > > > - what are our options to address this? (SK_ONCE?) > > > > The problem is that the cache is built for a given alpha, which can change for > > different contexts. So we can't just share a cache across threads once it's > been > > built. > > Leon suggested having a cache that can store values for multiple alpha values. > > Tom was wondering if we can pre-compute the cache without any alphas and then > > simply multiply the values by the correct alpha value. We'd need for someone > to > > check if that produces the correct output though. > > Mike had another suggestion: the shader caches a version with the most recently > used > alpha, guarded by a mutex. The context will ref the cache, and then if another > alpha > needs to be cached, the shader drops the old cache, and any context with the old > one > still has the old cache. That sounds good. Wouldn't require too many changes to the cache itself (only make it ref-counted). Should we do this in a follow-up?
What is the state of the perf-hit on GradientBench?
On 2014/04/09 20:44:09, reed1 wrote: > What is the state of the perf-hit on GradientBench? Running 'bench --match gradient --config 8888' on my N4 I see an average 2.3% slow-down, can be as high as 56% though (e.g. gradient_linear_clamp or gradient_linear_clamp_hicolor)
On 2014/04/10 11:37:38, Dominik Grewe wrote: > On 2014/04/09 20:44:09, reed1 wrote: > > What is the state of the perf-hit on GradientBench? > > Running 'bench --match gradient --config 8888' on my N4 I see an average 2.3% > slow-down, can be as high as 56% though (e.g. gradient_linear_clamp or > gradient_linear_clamp_hicolor) I should say that the average is weighted (i.e. I compute the speedup of the sum of all runs). The geometric mean speedup across all runs is 0.883, so a ~12% slow-down.
On 2014/04/10 12:49:37, Dominik Grewe wrote: > On 2014/04/10 11:37:38, Dominik Grewe wrote: > > On 2014/04/09 20:44:09, reed1 wrote: > > > What is the state of the perf-hit on GradientBench? > > > > Running 'bench --match gradient --config 8888' on my N4 I see an average 2.3% > > slow-down, can be as high as 56% though (e.g. gradient_linear_clamp or > > gradient_linear_clamp_hicolor) > > I should say that the average is weighted (i.e. I compute the speedup of the sum > of all runs). > The geometric mean speedup across all runs is 0.883, so a ~12% slow-down. Thanks for running that. bench/ doesn't try to be representative of how often a given feature is exercised "in the field", it is meant to zero-in on lots of specific features, so we can see any perf changes clearly (great for profiling and iterating on improvements).
On 2014/04/09 16:24:11, scroggo wrote: > On 2014/04/09 15:59:51, Dominik Grewe wrote: > > > 2. What is the performance hit given that we are not caching the > > gradient-table? > > > - what are the bench/ timings? > > > > Running bench_pictures on the buildbot SKPs on my N4 I see an average drop in > > performance of ~1% (maximum drop per page is 8%). > > > > > - what are our options to address this? (SK_ONCE?) > > > > The problem is that the cache is built for a given alpha, which can change for > > different contexts. So we can't just share a cache across threads once it's > been > > built. > > Leon suggested having a cache that can store values for multiple alpha values. > > Tom was wondering if we can pre-compute the cache without any alphas and then > > simply multiply the values by the correct alpha value. We'd need for someone > to > > check if that produces the correct output though. > > Mike had another suggestion: the shader caches a version with the most recently > used > alpha, guarded by a mutex. The context will ref the cache, and then if another > alpha > needs to be cached, the shader drops the old cache, and any context with the old > one > still has the old cache. I tried to implement this but ran into problems with SkOnce. I wanted to use it for the lazy initialization of the cache (see getCache16/32), but I can't figure out how to initialize the SkOnceFlag when it's a class member (I can't use SK_ONCE_INIT). Is there any reason why the initialization doesn't happen in the constructor?
I've uploaded my latest version including the shared caches. Had to modify SkOnce a little bit though to make it work (add constructors to SkOnceFlag and SkSpinlock). Unfortunately it's even slower than before...
On 2014/04/10 16:40:39, Dominik Grewe wrote: > On 2014/04/09 16:24:11, scroggo wrote: > > On 2014/04/09 15:59:51, Dominik Grewe wrote: > > > > 2. What is the performance hit given that we are not caching the > > > gradient-table? > > > > - what are the bench/ timings? > > > > > > Running bench_pictures on the buildbot SKPs on my N4 I see an average drop > in > > > performance of ~1% (maximum drop per page is 8%). > > > > > > > - what are our options to address this? (SK_ONCE?) > > > > > > The problem is that the cache is built for a given alpha, which can change > for > > > different contexts. So we can't just share a cache across threads once it's > > been > > > built. > > > Leon suggested having a cache that can store values for multiple alpha > values. > > > Tom was wondering if we can pre-compute the cache without any alphas and > then > > > simply multiply the values by the correct alpha value. We'd need for someone > > to > > > check if that produces the correct output though. > > > > Mike had another suggestion: the shader caches a version with the most > recently > > used > > alpha, guarded by a mutex. The context will ref the cache, and then if another > > alpha > > needs to be cached, the shader drops the old cache, and any context with the > old > > one > > still has the old cache. > > I tried to implement this but ran into problems with SkOnce. I wanted to use it > for the lazy initialization of the cache (see getCache16/32), but I can't figure > out how to initialize the SkOnceFlag when it's a class member (I can't use > SK_ONCE_INIT). Is there any reason why the initialization doesn't happen in the > constructor? I just added bungeman to the cc list. He's the right person to ask about SkOnce issues. He tells me there's an example in SkFontMgr_Indirect::onCountFamilies for using SkOnce on a non global.
On 2014/04/10 16:51:54, scroggo wrote: > On 2014/04/10 16:40:39, Dominik Grewe wrote: > > On 2014/04/09 16:24:11, scroggo wrote: > > > On 2014/04/09 15:59:51, Dominik Grewe wrote: > > > > > 2. What is the performance hit given that we are not caching the > > > > gradient-table? > > > > > - what are the bench/ timings? > > > > > > > > Running bench_pictures on the buildbot SKPs on my N4 I see an average drop > > in > > > > performance of ~1% (maximum drop per page is 8%). > > > > > > > > > - what are our options to address this? (SK_ONCE?) > > > > > > > > The problem is that the cache is built for a given alpha, which can change > > for > > > > different contexts. So we can't just share a cache across threads once > it's > > > been > > > > built. > > > > Leon suggested having a cache that can store values for multiple alpha > > values. > > > > Tom was wondering if we can pre-compute the cache without any alphas and > > then > > > > simply multiply the values by the correct alpha value. We'd need for > someone > > > to > > > > check if that produces the correct output though. > > > > > > Mike had another suggestion: the shader caches a version with the most > > recently > > > used > > > alpha, guarded by a mutex. The context will ref the cache, and then if > another > > > alpha > > > needs to be cached, the shader drops the old cache, and any context with the > > old > > > one > > > still has the old cache. > > > > I tried to implement this but ran into problems with SkOnce. I wanted to use > it > > for the lazy initialization of the cache (see getCache16/32), but I can't > figure > > out how to initialize the SkOnceFlag when it's a class member (I can't use > > SK_ONCE_INIT). Is there any reason why the initialization doesn't happen in > the > > constructor? > > I just added bungeman to the cc list. He's the right person to ask about SkOnce > issues. He tells me there's an example in SkFontMgr_Indirect::onCountFamilies > for > using SkOnce on a non global. Ah, I see. You can split the SkOnceFlag into a boolean and mutex. Thanks.
> Ah, I see. You can split the SkOnceFlag into a boolean and mutex. Thanks. Yup. Don't even feel guilty about doing it. SkOnceFlag, SK_ONCE_INIT and SK_DECLARE_STATIC_ONCE are all just convenience sugar. Everything goes through void SkOnce(bool* done, Lock* lock, ...);
On 2014/04/10 18:05:39, mtklein wrote: > > Ah, I see. You can split the SkOnceFlag into a boolean and mutex. Thanks. > > Yup. Don't even feel guilty about doing it. SkOnceFlag, SK_ONCE_INIT and > SK_DECLARE_STATIC_ONCE are all just convenience sugar. Everything goes through > void SkOnce(bool* done, Lock* lock, ...); (The reason initialization doesn't happen in the constructor is that we want it to happen at link time for statics, when used by say SK_DECLARE_STATIC_ONCE. Using a constructor will delay that initialization until that annoying bottlenecked runtime slot before main until we get constexpr from C++11. Further, SkOnceFlag uses a custom spinlock instead of a mutex precisely because you can't linktime initialize a mutex on Windows.)
When rebasing, I realized that there's a new subclass of SkShader: SkPictureShader. That shader references another shader (built dynamically at validContext/createContext), and thus the context holds another context. Unfortunately, we can't query the shader's context size because we don't have the shader at that time yet. So for now I'm just dynamically allocating the storage for the shader's context. But maybe you have a better idea. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... File src/core/SkPictureShader.cpp (right): https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:162: // doesn't know the size of the bitmap shader's context. Any ideas?
api lgtm
Found a tiny bug that seemed to have cause the performance regressions! I was doing some profiling to figure out where the perf regressions came from and that's when I found it: I forgot to add SkGradientShaderBase::getFlags() which overrides SkShader::getFlags (always return 0). This meant that we skipped various fast paths because we didn't recognize that the kOpaqueAlpha_Flag was set.
On 2014/04/11 14:52:04, Dominik Grewe wrote: > Found a tiny bug that seemed to have cause the performance regressions! I was > doing some profiling to figure out where the perf regressions came from and > that's when I found it: > > I forgot to add SkGradientShaderBase::getFlags() which overrides > SkShader::getFlags (always return 0). This meant that we skipped various fast > paths because we didn't recognize that the kOpaqueAlpha_Flag was set. That sounds like good news! What are your new perf numbers?
https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... File src/core/SkPictureShader.cpp (right): https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:88: SK_DECLARE_STATIC_MUTEX(cachedShaderMutex); Why is this static? https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:89: SkAutoMutexAcquire ama(cachedShaderMutex); It seems like we're doing an awful lot inside this mutex. What if we created the shader outside the mutex, and only cached it inside the mutex? Here are the pros and cons of my approach, as far as I can tell (perhaps there are more): PRO: - If two threads want to create different shaders, the second thread won't have to wait as long to create its own. CON: - If two threads want to create identical shaders, we could end up making each thread create the same one. Maybe the right choice depends on whether we expect two threads drawing the picture shader simultaneously to be drawing with the same tile size/local matrix or not. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:91: if (!fCachedBitmapShader || tileScale != fCachedTileScale) { Sort of tied into my last comment, if the cached shader has already been created, and thread A needs to make no modifications to it, it still locks the mutex, computes the new (potentially the same) matrix and sets it before returning. Meanwhile thread B could be waiting at the mutex when it also needed to make no changes. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:110: fCachedBitmapShader.get()->ref(); Typically the caller refs the object that gets returned. Is this done here so it can be done inside the mutex? If we do need to go against the typical usage pattern, there should be a comment explaining why. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:162: // doesn't know the size of the bitmap shader's context. On 2014/04/11 12:39:52, Dominik Grewe wrote: > Any ideas? No. I guess we don't know because we don't what type of shader will be returned by CreateBitmapShader? https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:175: if (fBitmapShaderContext) { I don't think this can ever be NULL, since it's created in the constructor (and we assert that it's non NULL), and it's deleted in the destructor. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:182: if (fBitmapShaderContext) { This should never be NULL.
On 2014/04/11 14:56:45, reed1 wrote: > On 2014/04/11 14:52:04, Dominik Grewe wrote: > > Found a tiny bug that seemed to have cause the performance regressions! I was > > doing some profiling to figure out where the perf regressions came from and > > that's when I found it: > > > > I forgot to add SkGradientShaderBase::getFlags() which overrides > > SkShader::getFlags (always return 0). This meant that we skipped various fast > > paths because we didn't recognize that the kOpaqueAlpha_Flag was set. > > That sounds like good news! What are your new perf numbers? This is on my Macbook Air (having issues with my Android build since last sync): bench_pictures: geo mean <1% slowdown (worst case ~40% for desk_yahoogames) GradientBench: geomean <2% slowdown (worst case ~26% for gradient_create_opaque)
https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... File src/core/SkPictureShader.cpp (right): https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:88: SK_DECLARE_STATIC_MUTEX(cachedShaderMutex); On 2014/04/11 15:51:45, scroggo wrote: > Why is this static? Good point. It should be a regular class member. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:89: SkAutoMutexAcquire ama(cachedShaderMutex); On 2014/04/11 15:51:45, scroggo wrote: > It seems like we're doing an awful lot inside this mutex. What if we created the > shader outside the mutex, and only cached it inside the mutex? > > Here are the pros and cons of my approach, as far as I can tell (perhaps there > are more): > PRO: > - If two threads want to create different shaders, the second thread won't > have > to wait as long to create its own. > CON: > - If two threads want to create identical shaders, we could end up making each > thread create the same one. > > Maybe the right choice depends on whether we expect two threads drawing the > picture shader simultaneously to be drawing with the same tile size/local matrix > or not. We always have to lock though before we access the cached shader, even if we don't modify it. Otherwise we may think that we don't need to modify the shader, but before reffing and returning it it's already been changed. So we'd need two locks: one for the early out, another for overwriting the shader. So I'm not sure either which would be faster. Like you said, probably depends on how it's being used. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:91: if (!fCachedBitmapShader || tileScale != fCachedTileScale) { On 2014/04/11 15:51:45, scroggo wrote: > Sort of tied into my last comment, if the cached shader has already been > created, and thread A needs to make no modifications to it, it still locks the > mutex, computes the new (potentially the same) matrix and sets it before > returning. Meanwhile thread B could be waiting at the mutex when it also needed > to make no changes. I didn't even realize the setLocalMatrix below... If we change the matrix, all other references to the cached shader become invalid, right? So we need to create a new shader each time the shaderMatrix changes, don't we? https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:110: fCachedBitmapShader.get()->ref(); On 2014/04/11 15:51:45, scroggo wrote: > Typically the caller refs the object that gets returned. Is this done here so it > can be done inside the mutex? > > If we do need to go against the typical usage pattern, there should be a comment > explaining why. Yes, we have to do it inside the mutex cause otherwise the shader may not exist any more when we try to ref it. Will add a comment. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:162: // doesn't know the size of the bitmap shader's context. On 2014/04/11 15:51:45, scroggo wrote: > On 2014/04/11 12:39:52, Dominik Grewe wrote: > > Any ideas? > > No. I guess we don't know because we don't what type of shader will be returned > by CreateBitmapShader? Yeah, we could hard-code a maximum size, but that's ugly. Just gonna keep the malloc here then. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:175: if (fBitmapShaderContext) { On 2014/04/11 15:51:45, scroggo wrote: > I don't think this can ever be NULL, since it's created in the constructor (and > we assert that it's non NULL), and it's deleted in the destructor. Done. https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:182: if (fBitmapShaderContext) { On 2014/04/11 15:51:45, scroggo wrote: > This should never be NULL. Done.
On 2014/04/11 16:23:41, Dominik Grewe wrote: > This is on my Macbook Air (having issues with my Android build since last sync): > > bench_pictures: geo mean <1% slowdown (worst case ~40% for desk_yahoogames) Yikes! Any idea if this is due to losing the cache? Changes to SkPictureShader lgtm https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... File src/core/SkPictureShader.cpp (right): https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... src/core/SkPictureShader.cpp:91: if (!fCachedBitmapShader || tileScale != fCachedTileScale) { On 2014/04/11 17:08:22, Dominik Grewe wrote: > On 2014/04/11 15:51:45, scroggo wrote: > > Sort of tied into my last comment, if the cached shader has already been > > created, and thread A needs to make no modifications to it, it still locks the > > mutex, computes the new (potentially the same) matrix and sets it before > > returning. Meanwhile thread B could be waiting at the mutex when it also > needed > > to make no changes. > > I didn't even realize the setLocalMatrix below... If we change the matrix, all > other references to the cached shader become invalid, right? So we need to > create a new shader each time the shaderMatrix changes, don't we? Yes. It may make more sense to make that change along with the overall setLocalMatrix changes. That said, we should probably add a TODO.
On 2014/04/11 17:51:35, scroggo wrote: > On 2014/04/11 16:23:41, Dominik Grewe wrote: > > This is on my Macbook Air (having issues with my Android build since last > sync): > > > > bench_pictures: geo mean <1% slowdown (worst case ~40% for desk_yahoogames) > > Yikes! Any idea if this is due to losing the cache? Not sure. I haven't look into it yet. > > Changes to SkPictureShader lgtm > > https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... > File src/core/SkPictureShader.cpp (right): > > https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... > src/core/SkPictureShader.cpp:91: if (!fCachedBitmapShader || tileScale != > fCachedTileScale) { > On 2014/04/11 17:08:22, Dominik Grewe wrote: > > On 2014/04/11 15:51:45, scroggo wrote: > > > Sort of tied into my last comment, if the cached shader has already been > > > created, and thread A needs to make no modifications to it, it still locks > the > > > mutex, computes the new (potentially the same) matrix and sets it before > > > returning. Meanwhile thread B could be waiting at the mutex when it also > > needed > > > to make no changes. > > > > I didn't even realize the setLocalMatrix below... If we change the matrix, all > > other references to the cached shader become invalid, right? So we need to > > create a new shader each time the shaderMatrix changes, don't we? > > Yes. It may make more sense to make that change along with the overall > setLocalMatrix changes. That said, we should probably add a TODO. Once we've moved the local matrix out of the shader we probably don't have to worry about it any more (although I'm not sure yet how we'll deal with that in this case). Until then, we should probably check if the picture shader's local matrix hasn't been changed since we last cached a shader (see latest upload).
On 2014/04/11 18:03:24, Dominik Grewe wrote: > On 2014/04/11 17:51:35, scroggo wrote: > > On 2014/04/11 16:23:41, Dominik Grewe wrote: > > > This is on my Macbook Air (having issues with my Android build since last > > sync): > > > > > > bench_pictures: geo mean <1% slowdown (worst case ~40% for desk_yahoogames) > > > > Yikes! Any idea if this is due to losing the cache? > > Not sure. I haven't look into it yet. > > > > > Changes to SkPictureShader lgtm > > > > > https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... > > File src/core/SkPictureShader.cpp (right): > > > > > https://codereview.chromium.org/207683004/diff/320001/src/core/SkPictureShade... > > src/core/SkPictureShader.cpp:91: if (!fCachedBitmapShader || tileScale != > > fCachedTileScale) { > > On 2014/04/11 17:08:22, Dominik Grewe wrote: > > > On 2014/04/11 15:51:45, scroggo wrote: > > > > Sort of tied into my last comment, if the cached shader has already been > > > > created, and thread A needs to make no modifications to it, it still locks > > the > > > > mutex, computes the new (potentially the same) matrix and sets it before > > > > returning. Meanwhile thread B could be waiting at the mutex when it also > > > needed > > > > to make no changes. > > > > > > I didn't even realize the setLocalMatrix below... If we change the matrix, > all > > > other references to the cached shader become invalid, right? So we need to > > > create a new shader each time the shaderMatrix changes, don't we? > > > > Yes. It may make more sense to make that change along with the overall > > setLocalMatrix changes. That said, we should probably add a TODO. > > Once we've moved the local matrix out of the shader we probably don't have to > worry about it any more (although I'm not sure yet how we'll deal with that in > this case). Until then, we should probably check if the picture shader's local > matrix hasn't been changed since we last cached a shader (see latest upload). Change to picture shader lgtm
On 2014/04/11 17:51:35, scroggo wrote: > On 2014/04/11 16:23:41, Dominik Grewe wrote: > > This is on my Macbook Air (having issues with my Android build since last > sync): > > > > bench_pictures: geo mean <1% slowdown (worst case ~40% for desk_yahoogames) > > Yikes! Any idea if this is due to losing the cache? Had another look today and I think I must have been running bench_pictures with the default params, including --repeat 1, which led to rather noisy results. With --repeat 100 the results are much more stable: mean performance is within <1% of master; max slowdown showed up as 12% on desk_fontwipe but when repeating the benchmark with --repeat 1000 the difference went away (must have been noise before; it's one of the smallest pages). The benchmarks showing the only significant slowdown on GradientBench are gradient_create_alpha (14%) and gradient_create_opaque (26%). I couldn't find an obvious reason why we're slower here. Removing the mutexes didn't help very much. It may just be the additional overhead we sometimes pay when calling validContext and createContext rather than a single setContext.
https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:564: SK_DECLARE_STATIC_MUTEX(cacheMutex); Does this need to be static? https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:566: if (!fCache || fCache->getAlpha() != alpha) { Does "!fCache" do the right thing here? Or do you need to call "!fCache.get()"? https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:569: fCache.get()->ref(); As in the picture shader, it would be nice to add a comment explaining why the function needs to call ref, rather than the caller.
https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:564: SK_DECLARE_STATIC_MUTEX(cacheMutex); On 2014/04/14 21:11:13, scroggo wrote: > Does this need to be static? No. Made it a class member. https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:566: if (!fCache || fCache->getAlpha() != alpha) { On 2014/04/14 21:11:13, scroggo wrote: > Does "!fCache" do the right thing here? Or do you need to call "!fCache.get()"? It seems to be doing the right thing. Stepping through it in GDB you can see that fCache gets converted to a pointer using operator T*() { return fObj; } because it doesn't have it's own "operator!". https://codereview.chromium.org/207683004/diff/380001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:569: fCache.get()->ref(); On 2014/04/14 21:11:13, scroggo wrote: > As in the picture shader, it would be nice to add a comment explaining why the > function needs to call ref, rather than the caller. Done.
Some small nits and a question for Ben or mtklein on SkOnce, but otherwise this LGTM https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:468: if (cache->fCache16Storage == NULL) { // set the storage and our working ptr Since this is called inside SkOnce, won't this always be true? https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:518: if (NULL == cache->fCache32PixelRef) { Again, won't this always be true, since we'll only ever call this function once? https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:125: unsigned fCacheAlpha; // The alpha value we used when we computed the cache. Couldn't this be const? https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:132: SkMutex fCache16Mutex, fCache32Mutex; Why doesn't this use SpinLock from SkOnce.h? Isn't the motivation for SkOnce that it's cheaper than a mutex? (This may just be my misunderstanding of SkOnce - it was clearly written so that you *can* use an SkMutex instead of a SpinLock.)
https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... File src/effects/gradients/SkGradientShader.cpp (right): https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:468: if (cache->fCache16Storage == NULL) { // set the storage and our working ptr On 2014/04/15 13:09:38, scroggo wrote: > Since this is called inside SkOnce, won't this always be true? Good point. I replaced the conditional with an assert. https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShader.cpp:518: if (NULL == cache->fCache32PixelRef) { On 2014/04/15 13:09:38, scroggo wrote: > Again, won't this always be true, since we'll only ever call this function once? Done. https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:125: unsigned fCacheAlpha; // The alpha value we used when we computed the cache. On 2014/04/15 13:09:38, scroggo wrote: > Couldn't this be const? Yes.
Sorry for all the back and forth! I think this is ready to commit. LGTM https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... File src/effects/gradients/SkGradientShaderPriv.h (right): https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... src/effects/gradients/SkGradientShaderPriv.h:132: SkMutex fCache16Mutex, fCache32Mutex; On 2014/04/15 13:09:38, scroggo wrote: > Why doesn't this use SpinLock from SkOnce.h? Isn't the motivation for SkOnce > that it's cheaper than a mutex? > > (This may just be my misunderstanding of SkOnce - it was clearly written so that > you *can* use an SkMutex instead of a SpinLock.) Just talked it over with Ben and Mike, and they said it's fine to use a mutex with SkOnce.
On 2014/04/15 16:55:04, scroggo wrote: > Sorry for all the back and forth! I think this is ready to commit. LGTM > > https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... > File src/effects/gradients/SkGradientShaderPriv.h (right): > > https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... > src/effects/gradients/SkGradientShaderPriv.h:132: SkMutex fCache16Mutex, > fCache32Mutex; > On 2014/04/15 13:09:38, scroggo wrote: > > Why doesn't this use SpinLock from SkOnce.h? Isn't the motivation for SkOnce > > that it's cheaper than a mutex? > > > > (This may just be my misunderstanding of SkOnce - it was clearly written so > that > > you *can* use an SkMutex instead of a SpinLock.) > > Just talked it over with Ben and Mike, and they said it's fine to use a mutex > with SkOnce. Do we have any change in the bench/ perf numbers using the mutex?
On 2014/04/15 19:35:34, reed1 wrote: > On 2014/04/15 16:55:04, scroggo wrote: > > Sorry for all the back and forth! I think this is ready to commit. LGTM > > > > > https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... > > File src/effects/gradients/SkGradientShaderPriv.h (right): > > > > > https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... > > src/effects/gradients/SkGradientShaderPriv.h:132: SkMutex fCache16Mutex, > > fCache32Mutex; > > On 2014/04/15 13:09:38, scroggo wrote: > > > Why doesn't this use SpinLock from SkOnce.h? Isn't the motivation for SkOnce > > > that it's cheaper than a mutex? > > > > > > (This may just be my misunderstanding of SkOnce - it was clearly written so > > that > > > you *can* use an SkMutex instead of a SpinLock.) > > > > Just talked it over with Ben and Mike, and they said it's fine to use a mutex > > with SkOnce. > > Do we have any change in the bench/ perf numbers using the mutex? The performance numbers I gave yesterday were from PS19. Since then, the only relevant change was to turn a static mutex into a class member. On the benchmarks that shouldn't make any difference because we're not running anything in parallel afaict.
On 2014/04/15 20:34:23, Dominik Grewe wrote: > On 2014/04/15 19:35:34, reed1 wrote: > > On 2014/04/15 16:55:04, scroggo wrote: > > > Sorry for all the back and forth! I think this is ready to commit. LGTM > > > > > > > > > https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... > > > File src/effects/gradients/SkGradientShaderPriv.h (right): > > > > > > > > > https://codereview.chromium.org/207683004/diff/430001/src/effects/gradients/S... > > > src/effects/gradients/SkGradientShaderPriv.h:132: SkMutex fCache16Mutex, > > > fCache32Mutex; > > > On 2014/04/15 13:09:38, scroggo wrote: > > > > Why doesn't this use SpinLock from SkOnce.h? Isn't the motivation for > SkOnce > > > > that it's cheaper than a mutex? > > > > > > > > (This may just be my misunderstanding of SkOnce - it was clearly written > so > > > that > > > > you *can* use an SkMutex instead of a SpinLock.) > > > > > > Just talked it over with Ben and Mike, and they said it's fine to use a > mutex > > > with SkOnce. > > > > Do we have any change in the bench/ perf numbers using the mutex? > > The performance numbers I gave yesterday were from PS19. Since then, the only > relevant change was to turn a static mutex into a class member. On the > benchmarks that shouldn't make any difference because we're not running anything > in parallel afaict. 25% on gradient_create_opaque seems unexpected, but I know several things are in play [mutex, separate context]. I'm fine if we land as is, and then I/we can profile afterwards and see what can be done to optimize this new flow.
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/207683004/450001
Message was sent while issue was closed.
Change committed as 14216
Message was sent while issue was closed.
https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... include/core/SkShader.h:219: virtual size_t contextSize() const = 0; Adding these two pure virtuals is causing issues in <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently investigation to see the full extent of the issue.
Message was sent while issue was closed.
On 2014/04/16 16:11:55, bungeman1 wrote: > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h > File include/core/SkShader.h (right): > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... > include/core/SkShader.h:219: virtual size_t contextSize() const = 0; > Adding these two pure virtuals is causing issues in > <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently investigation to > see the full extent of the issue. Oh, I didn't realize that there's code in Chromium that's subclassing SkShader. Could we add the methods that TestDiscardableShader overrides and that we've delete here behind a flag (as empty methods)?
Message was sent while issue was closed.
On 2014/04/16 16:43:16, Dominik Grewe wrote: > On 2014/04/16 16:11:55, bungeman1 wrote: > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h > > File include/core/SkShader.h (right): > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... > > include/core/SkShader.h:219: virtual size_t contextSize() const = 0; > > Adding these two pure virtuals is causing issues in > > <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently investigation > to > > see the full extent of the issue. > > Oh, I didn't realize that there's code in Chromium that's subclassing SkShader. > Could we add the methods that TestDiscardableShader overrides and that we've > delete here behind a flag (as empty methods)? I'm not sure that will be so easy. Chromium's shader will need to implement createContext etc, or it still won't build. Interestingly, it appears that this shader doesn't draw anything in software mode (shadeSpan does nothing). So it could return NULL for createContext and sizeof(SkShader::Context) for contextSize.
Message was sent while issue was closed.
On 2014/04/16 16:55:23, scroggo wrote: > On 2014/04/16 16:43:16, Dominik Grewe wrote: > > On 2014/04/16 16:11:55, bungeman1 wrote: > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h > > > File include/core/SkShader.h (right): > > > > > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... > > > include/core/SkShader.h:219: virtual size_t contextSize() const = 0; > > > Adding these two pure virtuals is causing issues in > > > <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently > investigation > > to > > > see the full extent of the issue. > > > > Oh, I didn't realize that there's code in Chromium that's subclassing > SkShader. > > Could we add the methods that TestDiscardableShader overrides and that we've > > delete here behind a flag (as empty methods)? > > I'm not sure that will be so easy. Chromium's shader will need to implement > createContext > etc, or it still won't build. Interestingly, it appears > that this shader doesn't draw anything in software mode (shadeSpan does > nothing). So it > could return NULL for createContext and sizeof(SkShader::Context) for > contextSize. We need to stage those changes though. Adding createContext and contextSize will only work once this CL has been rolled into Chromium. We could provide empty implementations for those methods in SkShader behind a flag though (only using the actual ones when the flag is not set). After the roll, we can remove the flag in Chromium and give TestDiscardableShader dummy implementations of createContext and contextSize.
Message was sent while issue was closed.
On 2014/04/16 16:43:16, Dominik Grewe wrote: > On 2014/04/16 16:11:55, bungeman1 wrote: > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h > > File include/core/SkShader.h (right): > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... > > include/core/SkShader.h:219: virtual size_t contextSize() const = 0; > > Adding these two pure virtuals is causing issues in > > <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently investigation > to > > see the full extent of the issue. > > Oh, I didn't realize that there's code in Chromium that's subclassing SkShader. > Could we add the methods that TestDiscardableShader overrides and that we've > delete here behind a flag (as empty methods)? So I just went and took a look (built Chromium on Linux), and it looks like the test pixel_ref_utils_unittest.cc (included by src/chrome/chrome_tests_unit.cc) appears to be the only file which requires attention. If it's a really small change I wouldn't mind sticking it in a roll if that's easiest. If it's larger I'd have to take another look.
Message was sent while issue was closed.
On 2014/04/16 17:01:05, Dominik Grewe wrote: > On 2014/04/16 16:55:23, scroggo wrote: > > On 2014/04/16 16:43:16, Dominik Grewe wrote: > > > On 2014/04/16 16:11:55, bungeman1 wrote: > > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h > > > > File include/core/SkShader.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... > > > > include/core/SkShader.h:219: virtual size_t contextSize() const = 0; > > > > Adding these two pure virtuals is causing issues in > > > > <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently > > investigation > > > to > > > > see the full extent of the issue. > > > > > > Oh, I didn't realize that there's code in Chromium that's subclassing > > SkShader. > > > Could we add the methods that TestDiscardableShader overrides and that we've > > > delete here behind a flag (as empty methods)? > > > > I'm not sure that will be so easy. Chromium's shader will need to implement > > createContext > > etc, or it still won't build. Interestingly, it appears > > that this shader doesn't draw anything in software mode (shadeSpan does > > nothing). So it > > could return NULL for createContext and sizeof(SkShader::Context) for > > contextSize. > > We need to stage those changes though. Adding createContext and contextSize will > only work once this CL has been rolled into Chromium. > > We could provide empty implementations for those methods in SkShader behind a > flag though (only using the actual ones when the flag is not set). After the > roll, we can remove the flag in Chromium and give TestDiscardableShader dummy > implementations of createContext and contextSize. Please take a look at https://codereview.chromium.org/240173003 and see if that's what you mean. This looks to build and the test passes on my local Linux build.
Message was sent while issue was closed.
On 2014/04/16 17:37:05, bungeman1 wrote: > On 2014/04/16 17:01:05, Dominik Grewe wrote: > > On 2014/04/16 16:55:23, scroggo wrote: > > > On 2014/04/16 16:43:16, Dominik Grewe wrote: > > > > On 2014/04/16 16:11:55, bungeman1 wrote: > > > > > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h > > > > > File include/core/SkShader.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/207683004/diff/450001/include/core/SkShader.h... > > > > > include/core/SkShader.h:219: virtual size_t contextSize() const = 0; > > > > > Adding these two pure virtuals is causing issues in > > > > > <chromium>/skia/ext/pixel_ref_utils_unittest.cc. I am currently > > > investigation > > > > to > > > > > see the full extent of the issue. > > > > > > > > Oh, I didn't realize that there's code in Chromium that's subclassing > > > SkShader. > > > > Could we add the methods that TestDiscardableShader overrides and that > we've > > > > delete here behind a flag (as empty methods)? > > > > > > I'm not sure that will be so easy. Chromium's shader will need to implement > > > createContext > > > etc, or it still won't build. Interestingly, it appears > > > that this shader doesn't draw anything in software mode (shadeSpan does > > > nothing). So it > > > could return NULL for createContext and sizeof(SkShader::Context) for > > > contextSize. > > > > We need to stage those changes though. Adding createContext and contextSize > will > > only work once this CL has been rolled into Chromium. > > > > We could provide empty implementations for those methods in SkShader behind a > > flag though (only using the actual ones when the flag is not set). After the > > roll, we can remove the flag in Chromium and give TestDiscardableShader dummy > > implementations of createContext and contextSize. > > Please take a look at https://codereview.chromium.org/240173003 and see if > that's what you mean. This looks to build and the test passes on my local Linux > build. Yup, that's doing the trick.
Message was sent while issue was closed.
I am suspicious that this is causing http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... . Are we leaking a bitmap pixel ref?
Message was sent while issue was closed.
On 2014/04/16 21:24:25, bungeman1 wrote: > I am suspicious that this is causing > http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... > . Are we leaking a bitmap pixel ref? I thought it might be because we're no longer calling SkBitmapProcState::endContext, but most of what it does is already in the destructor (which I believe is being called). Will investigate further tomorrow...
Message was sent while issue was closed.
On 2014/04/16 21:24:25, bungeman1 wrote: > I am suspicious that this is causing > http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... > . Are we leaking a bitmap pixel ref? But this is a Chromium test, right? Afaict this CL hasn't even made it into Chromium yet (current Skia revision is 14202). That's what your CL (https://codereview.chromium.org/240173003/) was doing right? Besides, the linux_asan bot has been mostly green since that build.
Message was sent while issue was closed.
On 2014/04/17 10:50:54, Dominik Grewe wrote: > On 2014/04/16 21:24:25, bungeman1 wrote: > > I am suspicious that this is causing > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... > > . Are we leaking a bitmap pixel ref? > > But this is a Chromium test, right? Afaict this CL hasn't even made it into > Chromium yet (current Skia revision is 14202). That's what your CL > (https://codereview.chromium.org/240173003/) was doing right? > Besides, the linux_asan bot has been mostly green since that build. This is a trybot, it failed as part of the roll attempt (in the CL mentioned above). This is a Chromium test, but it looks to be finding an issue in this change which Skia tests did not. Now that I'm back, I'll be taking a look.
Message was sent while issue was closed.
On 2014/04/17 13:44:50, bungeman1 wrote: > On 2014/04/17 10:50:54, Dominik Grewe wrote: > > On 2014/04/16 21:24:25, bungeman1 wrote: > > > I am suspicious that this is causing > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... > > > . Are we leaking a bitmap pixel ref? > > > > But this is a Chromium test, right? Afaict this CL hasn't even made it into > > Chromium yet (current Skia revision is 14202). That's what your CL > > (https://codereview.chromium.org/240173003/) was doing right? > > Besides, the linux_asan bot has been mostly green since that build. > > This is a trybot, it failed as part of the roll attempt (in the CL mentioned > above). This is a Chromium test, but it looks to be finding an issue in this > change which Skia tests did not. Now that I'm back, I'll be taking a look. The failure has happened again on re-trying the linux_asan build with a later revision of Chromium. I am having difficulty reproducing on my local machine (I can get asan, lsan, and the tests running fine, the issue just doesn't appear to be re-producing). If we cannot find a solution soon I'll need to revert this and the follow on change so we can unblock the roll.
Message was sent while issue was closed.
On 2014/04/17 16:17:26, bungeman1 wrote: > On 2014/04/17 13:44:50, bungeman1 wrote: > > On 2014/04/17 10:50:54, Dominik Grewe wrote: > > > On 2014/04/16 21:24:25, bungeman1 wrote: > > > > I am suspicious that this is causing > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... > > > > . Are we leaking a bitmap pixel ref? > > > > > > But this is a Chromium test, right? Afaict this CL hasn't even made it into > > > Chromium yet (current Skia revision is 14202). That's what your CL > > > (https://codereview.chromium.org/240173003/) was doing right? > > > Besides, the linux_asan bot has been mostly green since that build. > > > > This is a trybot, it failed as part of the roll attempt (in the CL mentioned > > above). This is a Chromium test, but it looks to be finding an issue in this > > change which Skia tests did not. Now that I'm back, I'll be taking a look. > > The failure has happened again on re-trying the linux_asan build with a later > revision of Chromium. I am having difficulty reproducing on my local machine (I > can get asan, lsan, and the tests running fine, the issue just doesn't appear to > be re-producing). If we cannot find a solution soon I'll need to revert this and > the follow on change so we can unblock the roll. By adding log messages to the places where SkBitmap::fPixelRef gets created and freed (unreffed with refcnt==1) I can see that with this CL we only free 6 out of the 7 bitmaps we create in RendererPixelTest/1.FastPassColorFilterAlpha. Haven't yet found the problem though.
Message was sent while issue was closed.
On 2014/04/17 16:54:28, Dominik Grewe wrote: > On 2014/04/17 16:17:26, bungeman1 wrote: > > On 2014/04/17 13:44:50, bungeman1 wrote: > > > On 2014/04/17 10:50:54, Dominik Grewe wrote: > > > > On 2014/04/16 21:24:25, bungeman1 wrote: > > > > > I am suspicious that this is causing > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/697... > > > > > . Are we leaking a bitmap pixel ref? > > > > > > > > But this is a Chromium test, right? Afaict this CL hasn't even made it > into > > > > Chromium yet (current Skia revision is 14202). That's what your CL > > > > (https://codereview.chromium.org/240173003/) was doing right? > > > > Besides, the linux_asan bot has been mostly green since that build. > > > > > > This is a trybot, it failed as part of the roll attempt (in the CL mentioned > > > above). This is a Chromium test, but it looks to be finding an issue in this > > > change which Skia tests did not. Now that I'm back, I'll be taking a look. > > > > The failure has happened again on re-trying the linux_asan build with a later > > revision of Chromium. I am having difficulty reproducing on my local machine > (I > > can get asan, lsan, and the tests running fine, the issue just doesn't appear > to > > be re-producing). If we cannot find a solution soon I'll need to revert this > and > > the follow on change so we can unblock the roll. > > By adding log messages to the places where SkBitmap::fPixelRef gets created and > freed (unreffed with refcnt==1) I can see that with this CL we only free 6 out > of the 7 bitmaps we create in RendererPixelTest/1.FastPassColorFilterAlpha. > > Haven't yet found the problem though. If you can't find it either, feel free to revert for now. Friday and Monday are public holidays over here, so I can have another look when I'm back in the office on Tuesday.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/241283003/ by bungeman@google.com. The reason for reverting is: Causing memory leaks in Chromium..
Message was sent while issue was closed.
On 2014/04/17 21:04:02, bungeman1 wrote: > A revert of this CL has been created in > https://codereview.chromium.org/241283003/ by mailto:bungeman@google.com. > > The reason for reverting is: Causing memory leaks in Chromium.. To re-apply this change (and related fixes) you'll want to undo the reverts in r14245, r14246, and r14247. It appears that the software_renderer.cc#cc::SoftwareRenderer::DrawRenderPassQuad#content SkBitmap contains the SkPixelRef which is left with two extra refs at the end of the method (and related tests like this one). Also, the call to drawSprite here seems to contain most of the issue as the SkPixelRef consistently has two more refs when returning from this call than going in. This was determined by making SkRefCnt ref and unref virtual, overriding them in SkPixelRef and printf'ing 'this' and 'getRefCnt' there.
Message was sent while issue was closed.
I've found the memory leak. When explicitly calling the destructor of an SkShader::Context pointer, I did the following: fShaderContext->SkShader::Context::~Context() I didn't know that this is a *non-virtual* call. Changing it to fShaderContext->~Context() makes it virtual. The problem was that we didn't call the destructor for an SkBitmapProcShader context properly when it was referenced by an SkFilterShader context. The SkBitmapProcState of the context thus wasn't correctly destroyed. I've rebased this patch, applied all the other fixes and this one. Shall I re-land from here?
On 2014/04/22 14:52:07, Dominik Grewe wrote: > I've found the memory leak. When explicitly calling the destructor of an > SkShader::Context pointer, I did the following: > fShaderContext->SkShader::Context::~Context() > I didn't know that this is a *non-virtual* call. Changing it to > fShaderContext->~Context() > makes it virtual. > > The problem was that we didn't call the destructor for an SkBitmapProcShader > context properly when it was referenced by an SkFilterShader context. The > SkBitmapProcState of the context thus wasn't correctly destroyed. > > I've rebased this patch, applied all the other fixes and this one. Shall I > re-land from here? What are "all the other fixes"? Can you also include https://codereview.chromium.org/238923010/ ?
On 2014/04/22 15:07:08, scroggo wrote: > On 2014/04/22 14:52:07, Dominik Grewe wrote: > > I've found the memory leak. When explicitly calling the destructor of an > > SkShader::Context pointer, I did the following: > > fShaderContext->SkShader::Context::~Context() > > I didn't know that this is a *non-virtual* call. Changing it to > > fShaderContext->~Context() > > makes it virtual. > > > > The problem was that we didn't call the destructor for an SkBitmapProcShader > > context properly when it was referenced by an SkFilterShader context. The > > SkBitmapProcState of the context thus wasn't correctly destroyed. > > > > I've rebased this patch, applied all the other fixes and this one. Shall I > > re-land from here? > > What are "all the other fixes"? The ones mentioned by bungeman: a previous fix to a memory leak (https://codereview.chromium.org/240303003/) and "#if SK_DEBUG -> #ifdef SK_DEBUG" > > Can you also include https://codereview.chromium.org/238923010/ ? Done.
LGTM. I think we'll need to update <chromium>/skia/ext/pixel_ref_utils_unittest.cc (which has class that overrides SkShader) when we do the DEPS roll. cc'ing Brian who is sheriff this week.
On 2014/04/22 15:28:59, scroggo wrote: > LGTM. I think we'll need to update > <chromium>/skia/ext/pixel_ref_utils_unittest.cc (which has class that overrides > SkShader) when we do the DEPS roll. cc'ing Brian who is sheriff this week. Yes, it needs to be updated as shown here: https://codereview.chromium.org/240173003/diff/40001/skia/ext/pixel_ref_utils...
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/207683004/490002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests, BuildTools http://skia-tree-status.appspot.com/redirect/compile-buildbots/buildstatus?bu...
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/207683004/540001
Message was sent while issue was closed.
Change committed as 14323
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/249643002/ by bsalomon@google.com. The reason for reverting is: This is blocking the DEPS roll into Chromium. Failures can be seen here: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/17....
Message was sent while issue was closed.
On 2014/04/23 16:16:18, bsalomon wrote: > A revert of this CL has been created in > https://codereview.chromium.org/249643002/ by mailto:bsalomon@google.com. > > The reason for reverting is: This is blocking the DEPS roll into Chromium. > Failures can be seen here: > > http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/17.... I found the patch necessary to roll this Chromium and am retrying the Skia->Chromium roll here: https://codereview.chromium.org/249563002/ If I'm able to land it I'll revert the revert. |
