apply imagefilter to all draw calls

apply imagefilter to all draw calls
Committed: https://code.google.com/p/skia/source/detail?r=3476

[reed1, 2012-03-22 14:14:01]

this is the CL in mid-testing.
posting now just to compare w/ Steven's change to drawSprite...

[Stephen White, 2012-03-22 15:58:10]

https://codereview.appspot.com/5856048/diff/1001/trunk/src/core/SkCanvas.cpp
File trunk/src/core/SkCanvas.cpp (right):

https://codereview.appspot.com/5856048/diff/1001/trunk/src/core/SkCanvas.cpp#...
trunk/src/core/SkCanvas.cpp:1629: LOOPER_BEGIN_DRAWDEVICE(*paint,
SkDrawFilter::kBitmap_Type)
It seems the image filters are not applied in drawSprite() in the CPU case with
this change.  I think we'll need my changes to SkCanvas::drawSprite() as well.

We could also roll back to LOOPER_BEGIN here, but then I see a 40% slowdown in
the GPU case over the combined patch.

[reed1, 2012-03-22 16:28:00]

#4 leaves some #if 0 code in, just to remind me of what I think I no longer
need.

[reed1, 2012-03-22 16:28:57]

In this version, gpu::drawDevice is ignoring the imagefilter, but drawSprite is
handling it. Stephen, is that something your other CL addresses?

[Stephen White, 2012-03-22 17:34:10]

Although this does fix the problems with the previous patch, I actually prefer
the previous version (combined with mine), since:

1)  It doesn't make drawSprite() "special" at the device level; all drawing
commands are agnostic of image filters, and all filter processing is centralized
in one place (SkDevice::filterImage()).
2)  It leaves open the possibility of exposing SkDevice::filterImage() at the
SkCanvas level in the future, which could be very useful when you want to do
something other than immediately draw the resulting bitmap.

https://codereview.appspot.com/5856048/diff/2003/trunk/src/core/SkPaint.cpp
File trunk/src/core/SkPaint.cpp (right):

https://codereview.appspot.com/5856048/diff/2003/trunk/src/core/SkPaint.cpp#n...
trunk/src/core/SkPaint.cpp:2224: canvas.drawSprite(src, -dstR.fLeft, -dstR.fTop,
&paint);
So this one assumes that drawSprite() always applies filters?  Hmm, no that
won't work with my patch, since I moved it to SkDevice::filterImage().

[reed1, 2012-03-22 20:44:30]

restores SkDevice::filterImage(), though it is not called in the fast-case
(relies on Device to notice the filter in drawSprite and drawDevice).

[reed1, 2012-03-22 21:09:06]

Stephen,

1. from our chat, I presume you know how to make SkGpuDevice::filterImage more
efficient (i.e. returning the filtered texture directly, rather than calling
drawSprite).

2. SkGpuDevice::drawDevice now needs to do what drawSprite is doing, (or
refactor something so they can share code).

It seems to me that #1 we can defer if we wish til later, but #2 should be done
soon. Can you send a diff that will effect #2? I could try copy/pasting from
drawSprite, but I figure you might have some refactoring already in mind...

[Stephen White, 2012-03-22 21:41:28]

On 2012/03/22 21:09:06, reed1 wrote:
> Stephen,
> 
> 1. from our chat, I presume you know how to make SkGpuDevice::filterImage more
> efficient (i.e. returning the filtered texture directly, rather than calling
> drawSprite).

Yeah, that's the SkGpuDevice change in https://codereview.appspot.com/5877044/

> 2. SkGpuDevice::drawDevice now needs to do what drawSprite is doing, (or
> refactor something so they can share code).

Well, once filterImage() is fast, we can just call that from both drawSprite()
and drawDevice().  I still prefer the earlier version of this patch where
SkCanvas does that for us in all cases, since it seems more natural.

If you don't want to do that, apply the SkGpuDevice.cpp change from my patch,
then call filterImage() from drawSprite() and drawDevice(), and use the returned
SkBitmap in place of the passed-in argument.

[Stephen White, 2012-03-22 23:19:49]

On 2012/03/22 21:41:28, Stephen White wrote:
> On 2012/03/22 21:09:06, reed1 wrote:
> > Stephen,
> > 
> > 1. from our chat, I presume you know how to make SkGpuDevice::filterImage
more
> > efficient (i.e. returning the filtered texture directly, rather than calling
> > drawSprite).
> 
> Yeah, that's the SkGpuDevice change in https://codereview.appspot.com/5877044/
> 
> > 2. SkGpuDevice::drawDevice now needs to do what drawSprite is doing, (or
> > refactor something so they can share code).
> 
> Well, once filterImage() is fast, we can just call that from both drawSprite()
> and drawDevice().  I still prefer the earlier version of this patch where
> SkCanvas does that for us in all cases, since it seems more natural.
> 
> If you don't want to do that, apply the SkGpuDevice.cpp change from my patch,
> then call filterImage() from drawSprite() and drawDevice(), and use the
returned
> SkBitmap in place of the passed-in argument.

OK, I've uploaded versions of both of these approaches to
https://codereview.appspot.com/5877044/.  I'm partial to option2, since it's
shorter, but at least this way we have something to talk about.

[reed1, 2012-03-23 14:02:48]

Decided to go with stephen's option #1, as it allows other filters to be done
inline, rather than forcing a separate temp texture during draw (e.g.
colorfilters, colormatrix, offset, etc.). Tom's refactoring work around
shader-stages should make these optimizations more obvious.

Applying Stephen's patch worked like a charm : new gm draws accelerated across
the board now!

[Stephen White, 2012-03-23 14:16:04]

I'm ok with this.  Leaving for Brian to check my work on the GPU side.

[reed1, 2012-03-23 14:20:23]

me too. Brian?

[TomH, 2012-03-23 15:17:19]

Putting on my Brian hat, the SkGpuDevice changes almost look OK to me (although
really putting on my Brian hat would require a lot more digging into semantics
than Mike wants to wait for).

https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.cpp
File trunk/src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
trunk/src/gpu/SkGpuDevice.cpp:1401: static GrTexture* filterTexture(GrContext*
context, GrTexture* texture, SkImageFilter* filter, const GrRect& rect) {
nit: 80col
also nit: filter_texture(), not filterTexture() - although it looks like Ganesh
isn't as consistent about that as Skia is.

https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
trunk/src/gpu/SkGpuDevice.cpp:1404: 
Can I hear a GrAssert(filter)?

https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
trunk/src/gpu/SkGpuDevice.cpp:1536: // The above needs to be consistent with our
canHandleImageFilter()
Then let's call canHandleImageFilter(), instead of duplicating code!
(You don't seem to be using size or radius?)

[reed1, 2012-03-23 15:27:04]

On 2012/03/23 15:17:19, TomH wrote:
> Putting on my Brian hat, the SkGpuDevice changes almost look OK to me
(although
> really putting on my Brian hat would require a lot more digging into semantics
> than Mike wants to wait for).
> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.cpp
> File trunk/src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> trunk/src/gpu/SkGpuDevice.cpp:1401: static GrTexture* filterTexture(GrContext*
> context, GrTexture* texture, SkImageFilter* filter, const GrRect& rect) {
> nit: 80col
> also nit: filter_texture(), not filterTexture() - although it looks like
Ganesh
> isn't as consistent about that as Skia is.

Done.

> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> trunk/src/gpu/SkGpuDevice.cpp:1404: 
> Can I hear a GrAssert(filter)?

Done.

> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> trunk/src/gpu/SkGpuDevice.cpp:1536: // The above needs to be consistent with
our
> canHandleImageFilter()
> Then let's call canHandleImageFilter(), instead of duplicating code!
> (You don't seem to be using size or radius?)

Done.

[TomH, 2012-03-23 15:27:39]

LGTM

[Stephen White, 2012-03-23 15:28:23]

On 2012/03/23 15:17:19, TomH wrote:
> Putting on my Brian hat, the SkGpuDevice changes almost look OK to me
(although
> really putting on my Brian hat would require a lot more digging into semantics
> than Mike wants to wait for).
> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.cpp
> File trunk/src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> trunk/src/gpu/SkGpuDevice.cpp:1401: static GrTexture* filterTexture(GrContext*
> context, GrTexture* texture, SkImageFilter* filter, const GrRect& rect) {
> nit: 80col
> also nit: filter_texture(), not filterTexture() - although it looks like
Ganesh
> isn't as consistent about that as Skia is.

This could also be in the unnamed namespace, rather than static, since the
former is more consistent with the rest of the file.

https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> trunk/src/gpu/SkGpuDevice.cpp:1404: 
> Can I hear a GrAssert(filter)?
> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> trunk/src/gpu/SkGpuDevice.cpp:1536: // The above needs to be consistent with
our
> canHandleImageFilter()
> Then let's call canHandleImageFilter(), instead of duplicating code!
> (You don't seem to be using size or radius?)

Those all sound reasonable to me.  Mike, do you want to make those changes,
since it's in your CL?

[Stephen White, 2012-03-23 15:32:13]

On 2012/03/23 15:28:23, Stephen White wrote:
> On 2012/03/23 15:17:19, TomH wrote:
> > Putting on my Brian hat, the SkGpuDevice changes almost look OK to me
> (although
> > really putting on my Brian hat would require a lot more digging into
semantics
> > than Mike wants to wait for).
> > 
> >
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.cpp
> > File trunk/src/gpu/SkGpuDevice.cpp (right):
> > 

> >
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> > trunk/src/gpu/SkGpuDevice.cpp:1401: static GrTexture*
filterTexture(GrContext*
> > context, GrTexture* texture, SkImageFilter* filter, const GrRect& rect) {
> > nit: 80col
> > also nit: filter_texture(), not filterTexture() - although it looks like
> Ganesh
> > isn't as consistent about that as Skia is.
> 
> This could also be in the unnamed namespace, rather than static, since the
> former is more consistent with the rest of the file.
> 
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> > trunk/src/gpu/SkGpuDevice.cpp:1404: 
> > Can I hear a GrAssert(filter)?
> > 
> >
>
https://codereview.appspot.com/5856048/diff/12001/trunk/src/gpu/SkGpuDevice.c...
> > trunk/src/gpu/SkGpuDevice.cpp:1536: // The above needs to be consistent with
> our
> > canHandleImageFilter()

> > Then let's call canHandleImageFilter(), instead of duplicating code!
> > (You don't seem to be using size or radius?)
> 
> Those all sound reasonable to me.  Mike, do you
 want to make those changes,
> since it's in your CL?

Never mind, they're all done (I'm a few messages behind), except for the unnamed
namespace -- you could fix that on landing if you like.

[reed1, 2012-03-23 15:33:11]

All are done in patch #7

I count (sans my new function) 9 static functions and 7 in namespaces, so I will
leave it as is, and allow the ganesh boys to decide if they want to unify around
one style or the other (skia for the most part uses just static).

[bsalomon, 2012-03-26 13:14:50]

On 2012/03/23 15:33:11, reed1 wrote:
> All are done in patch #7
> 
> I count (sans my new function) 9 static functions and 7 in namespaces, so I
will
> leave it as is, and allow the ganesh boys to decide if they want to unify
around
> one style or the other (skia for the most part uses just static).

My feeling is that the world is moving (has moved) to unnamed namespaces and I
don't want to try to hold the world back. AFAIK the reason is that two static
functions with the same name/signature in different trans. units would either
cause a link error or be conflated but not so with unnamed namespaces.