Prevent deferred canvas from recording draw operations that use an SkPaint object that indirectly r…

Prevent deferred canvases from recording draw operations that use an SkPaint object that indirectly references a GPU resource.
The specific case targeted by this patch is SkBitmapProcShaders that use an SkBitmap object that references a GPU texture.

BUG=http://code.google.com/p/chromium/issues/detail?id=139204
Committed: https://code.google.com/p/skia/source/detail?r=4871

[Leon, 2012-07-31 17:55:56]

https://codereview.appspot.com/6447068/diff/1001/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.appspot.com/6447068/diff/1001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:37: AutoImmediateDrawIfNeeded(SkDeferredCanvas&
canvas, const SkBitmap* bitmap, const SkPaint* paint) {
100 cols

https://codereview.appspot.com/6447068/diff/1001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:38: init(canvas, bitmap, paint);
this->init

https://codereview.appspot.com/6447068/diff/1001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:45: void init(SkDeferredCanvas& canvas, const
SkBitmap* bitmap, const SkPaint* paint)
init need not be public.

[junov1, 2012-07-31 17:59:41]

Done.  New patch.

[reed1, 2012-07-31 19:15:43]

Can we do this at the call site, using existing APIs?

if (!shader->asAGradient()) {
    if (shader->asABitmap(&bm)) {
        // return true if the bitmap has a texture
        return NULL != bm.getTexture();
    }
}

[junov1, 2012-07-31 19:28:24]

On 2012/07/31 19:15:43, reed1 wrote:
> Can we do this at the call site, using existing APIs?
> 
> if (!shader->asAGradient()) {
>     if (shader->asABitmap(&bm)) {
>         // return true if the bitmap has a texture
>         return NULL != bm.getTexture();
>     }
> }

I think that would work... but it's kinda fragile. Too many hidden assumptions.

[reed1, 2012-07-31 19:36:01]

On 2012/07/31 19:28:24, junov1 wrote:
> On 2012/07/31 19:15:43, reed1 wrote:
> > Can we do this at the call site, using existing APIs?
> > 
> > if (!shader->asAGradient()) {
> >     if (shader->asABitmap(&bm)) {
> >         // return true if the bitmap has a texture
> >         return NULL != bm.getTexture();
> >     }
> > }
> 
> I think that would work... but it's kinda fragile. Too many hidden
assumptions.

They don't need to be hidden, since the code will exist in DeferredCanvas. Its a
heurisitic to trigger another heuristic (stop deferring). We can document the
intent of each call, as part of documenting why we care.

[junov1, 2012-07-31 19:56:16]

On 2012/07/31 19:36:01, reed1 wrote:
> On 2012/07/31 19:28:24, junov1 wrote:
> > On 2012/07/31 19:15:43, reed1 wrote:
> > > Can we do this at the call site, using existing APIs?
> > > 
> > > if (!shader->asAGradient()) {
> > >     if (shader->asABitmap(&bm)) {
> > >         // return true if the bitmap has a texture
> > >         return NULL != bm.getTexture();
> > >     }
> > > }
> > 
> > I think that would work... but it's kinda fragile. Too many hidden
> assumptions.
> 
> They don't need to be hidden, since the code will exist in DeferredCanvas. Its
a
> heurisitic to trigger another heuristic (stop deferring). We can document the
> intent of each call, as part of documenting why we care.

Sorry, I wasn't clear about what I meant by "fragile".  In the future, someone
may implement a new SkShader subclass or modify an existing one. Adding
documentation to SkDeferredCanvas will do much to prevent someone making changes
in 'core' from breaking any of the following assumptions:
a) That gradient shaders and bitmap shaders are the only ones that implement
asABitmap();
b) That calling asABitmap on any non-gradient shader is a lightweight call;
c) That bitmaps shaders are the only types of shaders that may encapsulate
texture bitmaps.

[junov1, 2012-07-31 19:57:07]

On 2012/07/31 19:56:16, junov1 wrote:
> On 2012/07/31 19:36:01, reed1 wrote:
> > On 2012/07/31 19:28:24, junov1 wrote:
> > > On 2012/07/31 19:15:43, reed1 wrote:
> > > > Can we do this at the call site, using existing APIs?
> > > > 
> > > > if (!shader->asAGradient()) {
> > > >     if (shader->asABitmap(&bm)) {
> > > >         // return true if the bitmap has a texture
> > > >         return NULL != bm.getTexture();
> > > >     }
> > > > }
> > > 
> > > I think that would work... but it's kinda fragile. Too many hidden
> > assumptions.
> > 
> > They don't need to be hidden, since the code will exist in DeferredCanvas.
Its
> a
> > heurisitic to trigger another heuristic (stop deferring). We can document
the
> > intent of each call, as part of documenting why we care.
> 
> Sorry, I wasn't clear about what I meant by "fragile".  In the future, someone
> may implement a new SkShader subclass or modify an existing one. Adding
> documentation to SkDeferredCanvas will do much to prevent someone making
changes
> in 'core' from breaking any of the following assumptions:
> a) That gradient shaders and bitmap shaders are the only ones that implement
> asABitmap();
> b) That calling asABitmap on any non-gradient shader is a lightweight call;
> c) That bitmaps shaders are the only types of shaders that may encapsulate
> texture bitmaps.

I meant "will not do much", obviously

[junov1, 2012-07-31 20:41:31]

New patch.

[Leon, 2012-07-31 20:50:48]

lgtm