From 1adcf8859cc9414591038e440e3f22382c8e4aa0 Mon Sep 17 00:00:00 2001 From: "reed@google.com" Date: Thu, 13 Dec 2012 21:39:56 +0000 Subject: [PATCH] Goal: ensure we always balance lock/unlock pixels calls. A big caller of lockPixels is setContext in the bitmapshader. This change replaces beginSession/endSession with adding endContext(), and adds debugging code to ensure that 1. setContext calls are never nested 2. endContext is always called after each setContext call. Review URL: https://codereview.appspot.com/6937046 git-svn-id: http://skia.googlecode.com/svn/trunk@6798 2bbb7eff-a529-9590-31e7-b0007b416f81 --- include/core/SkComposeShader.h | 9 ++-- include/core/SkShader.h | 29 +++++++----- src/core/SkBitmapProcShader.cpp | 18 +++----- src/core/SkBitmapProcShader.h | 3 +- src/core/SkBlitter.cpp | 52 +++++++++++++--------- src/core/SkColorFilter.cpp | 28 +++++++----- src/core/SkComposeShader.cpp | 38 +++++++++------- src/core/SkDraw.cpp | 20 +++++---- src/core/SkFilterShader.h | 14 +++--- src/core/SkShader.cpp | 24 +++++----- src/effects/gradients/SkGradientShader.cpp | 2 + src/effects/gradients/SkGradientShaderPriv.h | 1 - src/effects/gradients/SkTwoPointRadialGradient.cpp | 13 +++--- 13 files changed, 136 insertions(+), 115 deletions(-) diff --git a/include/core/SkComposeShader.h b/include/core/SkComposeShader.h index a8a8e0b..b0790bf 100644 --- a/include/core/SkComposeShader.h +++ b/include/core/SkComposeShader.h @@ -34,11 +34,10 @@ public: SkComposeShader(SkShader* sA, SkShader* sB, SkXfermode* mode = NULL); virtual ~SkComposeShader(); - // override - virtual bool setContext(const SkBitmap& device, const SkPaint& paint, const SkMatrix& matrix); - virtual void shadeSpan(int x, int y, SkPMColor result[], int count); - virtual void beginSession(); - virtual void endSession(); + virtual bool setContext(const SkBitmap&, const SkPaint&, + const SkMatrix&) SK_OVERRIDE; + virtual void endContext() SK_OVERRIDE; + virtual void shadeSpan(int x, int y, SkPMColor[], int count) SK_OVERRIDE; SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkComposeShader) diff --git a/include/core/SkShader.h b/include/core/SkShader.h index 1286177..7edbe6f 100644 --- a/include/core/SkShader.h +++ b/include/core/SkShader.h @@ -139,12 +139,29 @@ public: /** * Called once before drawing, with the current paint and device matrix. * Return true if your shader supports these parameters, or false if not. - * If false is returned, nothing will be drawn. + * If false is returned, nothing will be drawn. If true is returned, then + * a balancing call to endContext() will be made before the next call to + * setContext. + * + * Subclasses should be sure to call their INHERITED::setContext() if they + * override this method. */ virtual bool setContext(const SkBitmap& device, const SkPaint& paint, const SkMatrix& matrix); /** + * Assuming setContext returned true, endContext() will be called when + * the draw using the shader has completed. It is an error for setContext + * to be called twice w/o an intervening call to endContext(). + * + * Subclasses should be sure to call their INHERITED::endContext() if they + * override this method. + */ + virtual void endContext(); + + SkDEBUGCODE(bool setContextHasBeenCalled() const { return fInSetContext; }) + + /** * Called for each span of the object being drawn. Your subclass should * set the appropriate colors (with premultiplied alpha) that correspond * to the specified device coordinates. @@ -183,14 +200,6 @@ public: } /** - * Called before a session using the shader begins. Some shaders override - * this to defer some of their work (like calling bitmap.lockPixels()). - * Must be balanced by a call to endSession. - */ - virtual void beginSession(); - virtual void endSession(); - - /** Gives method bitmap should be read to implement a shader. Also determines number and interpretation of "extra" parameters returned by asABitmap @@ -355,7 +364,7 @@ private: uint8_t fPaintAlpha; uint8_t fDeviceConfig; uint8_t fTotalInverseClass; - SkDEBUGCODE(SkBool8 fInSession;) + SkDEBUGCODE(SkBool8 fInSetContext;) static SkShader* CreateBitmapShader(const SkBitmap& src, TileMode, TileMode, diff --git a/src/core/SkBitmapProcShader.cpp b/src/core/SkBitmapProcShader.cpp index df2caee..3e41166 100644 --- a/src/core/SkBitmapProcShader.cpp +++ b/src/core/SkBitmapProcShader.cpp @@ -41,18 +41,6 @@ SkBitmapProcShader::SkBitmapProcShader(SkFlattenableReadBuffer& buffer) fFlags = 0; // computed in setContext } -void SkBitmapProcShader::beginSession() { - this->INHERITED::beginSession(); - - fRawBitmap.lockPixels(); -} - -void SkBitmapProcShader::endSession() { - fRawBitmap.unlockPixels(); - - this->INHERITED::endSession(); -} - SkShader::BitmapType SkBitmapProcShader::asABitmap(SkBitmap* texture, SkMatrix* texM, TileMode xy[]) const { @@ -102,6 +90,7 @@ bool SkBitmapProcShader::setContext(const SkBitmap& device, } if (!fState.chooseProcs(this->getTotalInverse(), paint)) { + fState.fOrigBitmap.unlockPixels(); return false; } @@ -149,6 +138,11 @@ bool SkBitmapProcShader::setContext(const SkBitmap& device, return true; } +void SkBitmapProcShader::endContext() { + fState.fOrigBitmap.unlockPixels(); + this->INHERITED::endContext(); +} + #define BUF_MAX 128 #define TEST_BUFFER_OVERRITEx diff --git a/src/core/SkBitmapProcShader.h b/src/core/SkBitmapProcShader.h index 23e0992..cb791d0 100644 --- a/src/core/SkBitmapProcShader.h +++ b/src/core/SkBitmapProcShader.h @@ -20,12 +20,11 @@ public: // overrides from SkShader virtual bool isOpaque() const SK_OVERRIDE; virtual bool setContext(const SkBitmap&, const SkPaint&, const SkMatrix&); + virtual void endContext(); virtual uint32_t getFlags() { return fFlags; } virtual void shadeSpan(int x, int y, SkPMColor dstC[], int count); virtual ShadeProc asAShadeProc(void** ctx) SK_OVERRIDE; virtual void shadeSpan16(int x, int y, uint16_t dstC[], int count); - virtual void beginSession(); - virtual void endSession(); virtual BitmapType asABitmap(SkBitmap*, SkMatrix*, TileMode*) const; static bool CanDo(const SkBitmap&, TileMode tx, TileMode ty); diff --git a/src/core/SkBlitter.cpp b/src/core/SkBlitter.cpp index 85331c2..1234f43 100644 --- a/src/core/SkBlitter.cpp +++ b/src/core/SkBlitter.cpp @@ -572,16 +572,30 @@ public: void setMask(const SkMask* mask) { fMask = mask; } virtual bool setContext(const SkBitmap& device, const SkPaint& paint, - const SkMatrix& matrix) { + const SkMatrix& matrix) SK_OVERRIDE { + if (!this->INHERITED::setContext(device, paint, matrix)) { + return false; + } if (fProxy) { - return fProxy->setContext(device, paint, matrix); + if (!fProxy->setContext(device, paint, matrix)) { + // must keep our set/end context calls balanced + this->INHERITED::endContext(); + return false; + } } else { fPMColor = SkPreMultiplyColor(paint.getColor()); - return this->INHERITED::setContext(device, paint, matrix); } + return true; + } + + virtual void endContext() SK_OVERRIDE { + if (fProxy) { + fProxy->endContext(); + } + this->INHERITED::endContext(); } - virtual void shadeSpan(int x, int y, SkPMColor span[], int count) { + virtual void shadeSpan(int x, int y, SkPMColor span[], int count) SK_OVERRIDE { if (fProxy) { fProxy->shadeSpan(x, y, span, count); } @@ -645,20 +659,6 @@ public: } } - virtual void beginSession() { - this->INHERITED::beginSession(); - if (fProxy) { - fProxy->beginSession(); - } - } - - virtual void endSession() { - if (fProxy) { - fProxy->endSession(); - } - this->INHERITED::endSession(); - } - SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(Sk3DShader) protected: @@ -907,8 +907,17 @@ SkBlitter* SkBlitter::Choose(const SkBitmap& device, // if there is one, the shader will take care of it. } + /* + * We need to have balanced calls to the shader: + * setContext + * endContext + * We make the first call here, in case it fails we can abort the draw. + * The endContext() call is made by the blitter (assuming setContext did + * not fail) in its destructor. + */ if (shader && !shader->setContext(device, *paint, matrix)) { - return SkNEW(SkNullBlitter); + SK_PLACEMENT_NEW(blitter, SkNullBlitter, storage, storageSize); + return blitter; } switch (device.getConfig()) { @@ -978,14 +987,15 @@ SkShaderBlitter::SkShaderBlitter(const SkBitmap& device, const SkPaint& paint) : INHERITED(device) { fShader = paint.getShader(); SkASSERT(fShader); + SkASSERT(fShader->setContextHasBeenCalled()); fShader->ref(); - fShader->beginSession(); fShaderFlags = fShader->getFlags(); } SkShaderBlitter::~SkShaderBlitter() { - fShader->endSession(); + SkASSERT(fShader->setContextHasBeenCalled()); + fShader->endContext(); fShader->unref(); } diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp index 6d9f4ba..156fb8f 100644 --- a/src/core/SkColorFilter.cpp +++ b/src/core/SkColorFilter.cpp @@ -62,16 +62,6 @@ SkFilterShader::~SkFilterShader() { fShader->unref(); } -void SkFilterShader::beginSession() { - this->INHERITED::beginSession(); - fShader->beginSession(); -} - -void SkFilterShader::endSession() { - fShader->endSession(); - this->INHERITED::endSession(); -} - void SkFilterShader::flatten(SkFlattenableWriteBuffer& buffer) const { this->INHERITED::flatten(buffer); buffer.writeFlattenable(fShader); @@ -96,8 +86,22 @@ uint32_t SkFilterShader::getFlags() { bool SkFilterShader::setContext(const SkBitmap& device, const SkPaint& paint, const SkMatrix& matrix) { - return this->INHERITED::setContext(device, paint, matrix) && - fShader->setContext(device, paint, matrix); + // we need to keep the setContext/endContext calls balanced. If we return + // false, our endContext() will not be called. + + if (!this->INHERITED::setContext(device, paint, matrix)) { + return false; + } + if (!fShader->setContext(device, paint, matrix)) { + this->INHERITED::endContext(); + return false; + } + return true; +} + +void SkFilterShader::endContext() { + fShader->endContext(); + this->INHERITED::endContext(); } void SkFilterShader::shadeSpan(int x, int y, SkPMColor result[], int count) { diff --git a/src/core/SkComposeShader.cpp b/src/core/SkComposeShader.cpp index 2fe7a20..92ffaf7 100644 --- a/src/core/SkComposeShader.cpp +++ b/src/core/SkComposeShader.cpp @@ -43,18 +43,6 @@ SkComposeShader::~SkComposeShader() { fShaderA->unref(); } -void SkComposeShader::beginSession() { - this->INHERITED::beginSession(); - fShaderA->beginSession(); - fShaderB->beginSession(); -} - -void SkComposeShader::endSession() { - fShaderA->endSession(); - fShaderB->endSession(); - this->INHERITED::endSession(); -} - class SkAutoAlphaRestore { public: SkAutoAlphaRestore(SkPaint* paint, uint8_t newAlpha) { @@ -81,7 +69,10 @@ void SkComposeShader::flatten(SkFlattenableWriteBuffer& buffer) const { /* We call setContext on our two worker shaders. However, we always let them see opaque alpha, and if the paint really is translucent, then we apply that after the fact. -*/ + + We need to keep the calls to setContext/endContext balanced, since if we + return false, our endContext() will not be called. + */ bool SkComposeShader::setContext(const SkBitmap& device, const SkPaint& paint, const SkMatrix& matrix) { @@ -98,8 +89,25 @@ bool SkComposeShader::setContext(const SkBitmap& device, SkAutoAlphaRestore restore(const_cast(&paint), 0xFF); - return fShaderA->setContext(device, paint, tmpM) && - fShaderB->setContext(device, paint, tmpM); + bool setContextA = fShaderA->setContext(device, paint, tmpM); + bool setContextB = fShaderB->setContext(device, paint, tmpM); + if (!setContextA || !setContextB) { + if (setContextB) { + fShaderB->endContext(); + } + else if (setContextA) { + fShaderA->endContext(); + } + this->INHERITED::endContext(); + return false; + } + return true; +} + +void SkComposeShader::endContext() { + fShaderB->endContext(); + fShaderA->endContext(); + this->INHERITED::endContext(); } // larger is better (fewer times we have to loop), but we shouldn't diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp index 3035afe..f71187e 100644 --- a/src/core/SkDraw.cpp +++ b/src/core/SkDraw.cpp @@ -1228,13 +1228,6 @@ void SkDraw::drawBitmap(const SkBitmap& bitmap, const SkMatrix& prematrix, } } - // only lock the pixels if we passed the clip and bounder tests - SkAutoLockPixels alp(bitmap); - // after the lock, check if we are valid - if (!bitmap.readyToDraw()) { - return; - } - if (bitmap.getConfig() != SkBitmap::kA8_Config && just_translate(matrix, bitmap)) { int ix = SkScalarRound(matrix.getTranslateX()); @@ -2279,7 +2272,7 @@ public: bool setup(const SkPoint pts[], const SkColor colors[], int, int, int); - virtual void shadeSpan(int x, int y, SkPMColor dstC[], int count); + virtual void shadeSpan(int x, int y, SkPMColor dstC[], int count) SK_OVERRIDE; SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkTriColorShader) @@ -2406,7 +2399,7 @@ void SkDraw::drawVertices(SkCanvas::VertexMode vmode, int count, if (NULL != colors) { if (NULL == textures) { // just colors (no texture) - p.setShader(&triShader); + shader = p.setShader(&triShader); } else { // colors * texture SkASSERT(shader); @@ -2421,6 +2414,7 @@ void SkDraw::drawVertices(SkCanvas::VertexMode vmode, int count, if (releaseMode) { xmode->unref(); } + shader = compose; } } @@ -2436,10 +2430,17 @@ void SkDraw::drawVertices(SkCanvas::VertexMode vmode, int count, savedLocalM = shader->getLocalMatrix(); } + // do we need this? triShader should have been installed in p, either + // directly or indirectly (using compose shader), so its setContext + // should have already been called. if (NULL != colors) { + SkASSERT(triShader.setContextHasBeenCalled()); +#if 0 + if (!triShader.setContext(*fBitmap, p, *fMatrix)) { colors = NULL; } +#endif } while (vertProc(&state)) { @@ -2448,6 +2449,7 @@ void SkDraw::drawVertices(SkCanvas::VertexMode vmode, int count, tempM.postConcat(savedLocalM); shader->setLocalMatrix(tempM); // need to recal setContext since we changed the local matrix + shader->endContext(); if (!shader->setContext(*fBitmap, p, *fMatrix)) { continue; } diff --git a/src/core/SkFilterShader.h b/src/core/SkFilterShader.h index f637584..ded3125 100644 --- a/src/core/SkFilterShader.h +++ b/src/core/SkFilterShader.h @@ -17,14 +17,12 @@ public: SkFilterShader(SkShader* shader, SkColorFilter* filter); virtual ~SkFilterShader(); - // override - virtual uint32_t getFlags(); - virtual bool setContext(const SkBitmap& device, const SkPaint& paint, - const SkMatrix& matrix); - virtual void shadeSpan(int x, int y, SkPMColor result[], int count); - virtual void shadeSpan16(int x, int y, uint16_t result[], int count); - virtual void beginSession(); - virtual void endSession(); + virtual uint32_t getFlags() SK_OVERRIDE; + virtual bool setContext(const SkBitmap&, const SkPaint&, + const SkMatrix&) SK_OVERRIDE; + virtual void endContext() SK_OVERRIDE; + virtual void shadeSpan(int x, int y, SkPMColor[], int count) SK_OVERRIDE; + virtual void shadeSpan16(int x, int y, uint16_t[], int count) SK_OVERRIDE; SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkFilterShader) diff --git a/src/core/SkShader.cpp b/src/core/SkShader.cpp index 2b20e3d..706c1b7 100644 --- a/src/core/SkShader.cpp +++ b/src/core/SkShader.cpp @@ -17,7 +17,7 @@ SK_DEFINE_INST_COUNT(SkShader) SkShader::SkShader() { fLocalMatrix.reset(); - SkDEBUGCODE(fInSession = false;) + SkDEBUGCODE(fInSetContext = false;) } SkShader::SkShader(SkFlattenableReadBuffer& buffer) @@ -28,21 +28,11 @@ SkShader::SkShader(SkFlattenableReadBuffer& buffer) fLocalMatrix.reset(); } - SkDEBUGCODE(fInSession = false;) + SkDEBUGCODE(fInSetContext = false;) } SkShader::~SkShader() { - SkASSERT(!fInSession); -} - -void SkShader::beginSession() { - SkASSERT(!fInSession); - SkDEBUGCODE(fInSession = true;) -} - -void SkShader::endSession() { - SkASSERT(fInSession); - SkDEBUGCODE(fInSession = false;) + SkASSERT(!fInSetContext); } void SkShader::flatten(SkFlattenableWriteBuffer& buffer) const { @@ -57,6 +47,8 @@ void SkShader::flatten(SkFlattenableWriteBuffer& buffer) const { bool SkShader::setContext(const SkBitmap& device, const SkPaint& paint, const SkMatrix& matrix) { + SkASSERT(!this->setContextHasBeenCalled()); + const SkMatrix* m = &matrix; SkMatrix total; @@ -68,11 +60,17 @@ bool SkShader::setContext(const SkBitmap& device, } if (m->invert(&fTotalInverse)) { fTotalInverseClass = (uint8_t)ComputeMatrixClass(fTotalInverse); + SkDEBUGCODE(fInSetContext = true;) return true; } return false; } +void SkShader::endContext() { + SkASSERT(fInSetContext); + SkDEBUGCODE(fInSetContext = false;) +} + SkShader::ShadeProc SkShader::asAShadeProc(void** ctx) { return NULL; } diff --git a/src/effects/gradients/SkGradientShader.cpp b/src/effects/gradients/SkGradientShader.cpp index 15c7510..5b2a60e 100644 --- a/src/effects/gradients/SkGradientShader.cpp +++ b/src/effects/gradients/SkGradientShader.cpp @@ -213,6 +213,8 @@ bool SkGradientShaderBase::setContext(const SkBitmap& device, const SkMatrix& inverse = this->getTotalInverse(); if (!fDstToIndex.setConcat(fPtsToUnit, inverse)) { + // need to keep our set/end context calls balanced. + this->INHERITED::endContext(); return false; } diff --git a/src/effects/gradients/SkGradientShaderPriv.h b/src/effects/gradients/SkGradientShaderPriv.h index 792cf66..829d153 100644 --- a/src/effects/gradients/SkGradientShaderPriv.h +++ b/src/effects/gradients/SkGradientShaderPriv.h @@ -91,7 +91,6 @@ public: int colorCount, SkShader::TileMode mode, SkUnitMapper* mapper); virtual ~SkGradientShaderBase(); - // overrides virtual bool setContext(const SkBitmap&, const SkPaint&, const SkMatrix&) SK_OVERRIDE; virtual uint32_t getFlags() SK_OVERRIDE { return fFlags; } virtual bool isOpaque() const SK_OVERRIDE; diff --git a/src/effects/gradients/SkTwoPointRadialGradient.cpp b/src/effects/gradients/SkTwoPointRadialGradient.cpp index 71b3126..9aa923b 100644 --- a/src/effects/gradients/SkTwoPointRadialGradient.cpp +++ b/src/effects/gradients/SkTwoPointRadialGradient.cpp @@ -292,16 +292,15 @@ void SkTwoPointRadialGradient::shadeSpan(int x, int y, SkPMColor* dstCParam, } } -bool SkTwoPointRadialGradient::setContext( - const SkBitmap& device, - const SkPaint& paint, - const SkMatrix& matrix){ - if (!this->INHERITED::setContext(device, paint, matrix)) { +bool SkTwoPointRadialGradient::setContext( const SkBitmap& device, + const SkPaint& paint, + const SkMatrix& matrix){ + // For now, we might have divided by zero, so detect that + if (0 == fDiffRadius) { return false; } - // For now, we might have divided by zero, so detect that - if (0 == fDiffRadius) { + if (!this->INHERITED::setContext(device, paint, matrix)) { return false; } -- 2.7.4