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
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 @@
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 @@
/**
* 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 @@
}
/**
- * 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 @@
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 @@
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 @@
}
if (!fState.chooseProcs(this->getTotalInverse(), paint)) {
+ fState.fOrigBitmap.unlockPixels();
return false;
}
@@ -149,6 +138,11 @@
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 @@
// 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 @@
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 @@
}
}
- 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 @@
// 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 @@
: 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 @@
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 @@
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 @@
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 @@
/* 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 @@
SkAutoAlphaRestore restore(const_cast<SkPaint*>(&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 @@
}
}
- // 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 @@
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 @@
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 @@
if (releaseMode) {
xmode->unref();
}
+ shader = compose;
}
}
@@ -2436,10 +2430,17 @@
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 @@
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 @@
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 @@
SkShader::SkShader() {
fLocalMatrix.reset();
- SkDEBUGCODE(fInSession = false;)
+ SkDEBUGCODE(fInSetContext = false;)
}
SkShader::SkShader(SkFlattenableReadBuffer& buffer)
@@ -28,21 +28,11 @@
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 @@
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 @@
}
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 @@
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 @@
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 @@
}
}
-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;
}