Goal: ensure we always balance lock/unlock pixels calls.

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.
Committed: https://code.google.com/p/skia/source/detail?r=6798

[Leon, 2012-12-13 19:25:53]

lgtm

https://codereview.appspot.com/6937046/diff/1/src/core/SkBitmapProcShader.cpp
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkBitmapProcShader.cpp...
src/core/SkBitmapProcShader.cpp:203: *ctx = &fState;
Any concern that the caller might modify fState or fState.fOrigBitmap?

https://codereview.appspot.com/6937046/diff/1/src/core/SkColorFilter.cpp
File src/core/SkColorFilter.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkColorFilter.cpp#newc...
src/core/SkColorFilter.cpp:103: this->INHERITED::endContext();
Would it make sense to reverse the order here so that fShader's start/endContext
happens in between SkFilterShader's? (This is how Sk3DShader behaves.)

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp
File src/core/SkComposeShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp#ne...
src/core/SkComposeShader.cpp:95: this->INHERITED::endContext();
Should this happen last to be nested?

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp#ne...
src/core/SkComposeShader.cpp:108: this->INHERITED::endContext();
Nesting?

https://codereview.appspot.com/6937046/diff/1/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkShader.cpp#newcode63
src/core/SkShader.cpp:63: SkDEBUGCODE(fInSetContext = true;)
Previously if two threads were trying to draw a shader simultaneously, the
second one would assert in beginSession, regardless of the success of
setContext. Would it make sense to set this to true at the start of this
function, immediately after asserting that it is not true? Or is that probably a
case we don't need to worry about?

[reed1, 2012-12-13 19:33:22]

https://codereview.appspot.com/6937046/diff/1/src/core/SkBitmapProcShader.cpp
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkBitmapProcShader.cpp...
src/core/SkBitmapProcShader.cpp:203: *ctx = &fState;
On 2012/12/13 19:25:54, scroggo-work wrote:
> Any concern that the caller might modify fState or fState.fOrigBitmap?

The caller just sees a void*, it never knows about fState or its contents.

https://codereview.appspot.com/6937046/diff/1/src/core/SkColorFilter.cpp
File src/core/SkColorFilter.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkColorFilter.cpp#newc...
src/core/SkColorFilter.cpp:103: this->INHERITED::endContext();
On 2012/12/13 19:25:54, scroggo-work wrote:
> Would it make sense to reverse the order here so that fShader's
start/endContext
> happens in between SkFilterShader's? (This is how Sk3DShader behaves.)

Done.

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp
File src/core/SkComposeShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp#ne...
src/core/SkComposeShader.cpp:95: this->INHERITED::endContext();
On 2012/12/13 19:25:54, scroggo-work wrote:
> Should this happen last to be nested?

Done.

Though, we never call endContext on both, 'cause in that case both would have
succeeded, and we would return true instead.

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp#ne...
src/core/SkComposeShader.cpp:108: this->INHERITED::endContext();
On 2012/12/13 19:25:54, scroggo-work wrote:
> Nesting?

Done.

https://codereview.appspot.com/6937046/diff/1/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkShader.cpp#newcode63
src/core/SkShader.cpp:63: SkDEBUGCODE(fInSetContext = true;)
On 2012/12/13 19:25:54, scroggo-work wrote:
> Previously if two threads were trying to draw a shader simultaneously, the
> second one would assert in beginSession, regardless of the success of
> setContext. Would it make sense to set this to true at the start of this
> function, immediately after asserting that it is not true? Or is that probably
a
> case we don't need to worry about?

Shaders are 99% not thread-safe already.

[Leon, 2012-12-13 19:40:31]

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp
File src/core/SkComposeShader.cpp (right):

https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp#ne...
src/core/SkComposeShader.cpp:95: this->INHERITED::endContext();
On 2012/12/13 19:33:22, reed1 wrote:
> On 2012/12/13 19:25:54, scroggo-work wrote:
> > Should this happen last to be nested?
> 
> Done.
> 
> Though, we never call endContext on both, 'cause in that case both would have
> succeeded, and we would return true instead.

Oh, haha, I meant call this->INHERITED::endContext() after calling fShaderA (or
fShaderB)->endContext()

[reed1, 2012-12-13 19:50:02]

On 2012/12/13 19:40:31, scroggo-work wrote:
> https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp
> File src/core/SkComposeShader.cpp (right):
> 
>
https://codereview.appspot.com/6937046/diff/1/src/core/SkComposeShader.cpp#ne...
> src/core/SkComposeShader.cpp:95: this->INHERITED::endContext();
> On 2012/12/13 19:33:22, reed1 wrote:
> > On 2012/12/13 19:25:54, scroggo-work wrote:
> > > Should this happen last to be nested?
> > 
> > Done.
> > 
> > Though, we never call endContext on both, 'cause in that case both would
have
> > succeeded, and we would return true instead.
> 
> Oh, haha, I meant call this->INHERITED::endContext() after calling fShaderA
(or
> fShaderB)->endContext()

Done.

[Leon, 2012-12-13 19:51:41]

lgtm