Implement SkImageFilter support in the compositor.

Implement SkImageFilter support in the compositor.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163507

[Stephen White, 2012-10-19 18:49:58]

PTAL.  For the threaded compositor case, according to my read of SkRefCnt.h (and
confirming with Mike), it should be safe to hand off SkImageFilters and later
unref them on the impl thread.  But please check if I'm doing something stupid.

[jamesr, 2012-10-19 19:15:30]

Is SkImageFilter supposed to be instead of or in addition to the existing
filters property?  Is there a backgroundFilter equivalent?

I can't tell where in the compositor code we're supposed to unref the
SkImageFilter.

We have a software backend now too (cc/software_renderer.h).  It doesn't support
filters, but if it's easy to support please keep it in mind.

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc#newcode358
cc/gl_renderer.cc:358: GrContext* grContext = CCProxy::hasImplThread() ?
WebSharedGraphicsContext3D::compositorThreadGrContext() :
WebSharedGraphicsContext3D::mainThreadGrContext();
this is exactly the same (with different variable names) as lines 339-343 in
applyFilters() above. Can you figure out a way to share this code instead of
copy-paste?

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h
File cc/layer.h (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h#newcode343
cc/layer.h:343: SkImageFilter* m_filter;
Should this be a SkTAutoUnref<> or something of that nature?  What should happen
if a filter is set on a Layer and then the layer is destroyed before we
pushPropertiesTo - we should drop a ref, right?

http://codereview.chromium.org/11175009/diff/2001/cc/layer_impl.h
File cc/layer_impl.h (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer_impl.h#newcode368
cc/layer_impl.h:368: SkImageFilter* m_filter;
Same question here - are we supposed to unref in ~LayerImpl() ?

http://codereview.chromium.org/11175009/diff/2001/webkit/compositor_bindings/...
File webkit/compositor_bindings/web_layer_impl.h (right):

http://codereview.chromium.org/11175009/diff/2001/webkit/compositor_bindings/...
webkit/compositor_bindings/web_layer_impl.h:63: // TODO(senorblanco):  uncomment
OVERRIDE once WebKit changes have landed.
No, leave the OVERRIDE out.  OVERRIDE's aren't worth it cross-repo (the rest of
this file has the OVERRIDEs due to historical accident)

[danakj, 2012-10-19 19:34:24]

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc#newcode26
cc/gl_renderer.cc:26: #include "SkGrTexturePixelRef.h"
nit: sorting

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc#newcode352
cc/gl_renderer.cc:352: static SkBitmap applyFilter(CCRendererGL* renderer,
SkImageFilter* filter, CCScopedTexture* sourceTexture)
can we rename this to applyImageFilter?

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h
File cc/layer.h (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h#newcode100
cc/layer.h:100: void setFilter(SkImageFilter* filter);
same, setImageFilter?

http://codereview.chromium.org/11175009/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:581: if
(layer->filters().hasFilterThatMovesPixels() || layer->filter())
There are a few other places that check for this: occlusion tracker, damage
tracker.

I see you got the opacity check in occlusion tracker, but not the moves pixels.

What does it mean for the filter outsets? In damage tracker and occlusion
tracker we add damage/remove occlusion based on the filter outsets.

Can an image filter move pixels? Or does it just change opacity/color values for
each pixel indepdently. If it can move color values from one pixel into another,
how far away? Is it completely arbitrary?

If arbitrary, then any damage in the layer needs to damage the whole layer in
damage tracker. Any non-occluded area on the layer needs to un-occlude the
entire layer in the occlusion tracker.

Maybe to help make common code we could add some templated static methods to
RenderSurfaceFilters to make queries about what filter effects a layer has, such
as hasFilterThatMovesPixels(), hasFilterThatAffectsOpacity(), filterOutsets(),
etc?

http://codereview.chromium.org/11175009/diff/2001/cc/render_pass.cc
File cc/render_pass.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/render_pass.cc#newcode111
cc/render_pass.cc:111: }
Same question as james, who frees this one?

http://codereview.chromium.org/11175009/diff/2001/cc/render_pass.h
File cc/render_pass.h (right):

http://codereview.chromium.org/11175009/diff/2001/cc/render_pass.h#newcode95
cc/render_pass.h:95: SkImageFilter* filter() const { return m_filter; }
can we rename this to imageFilter()?

[Stephen White, 2012-10-19 21:34:50]

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc#newcode26
cc/gl_renderer.cc:26: #include "SkGrTexturePixelRef.h"
On 2012/10/19 19:34:24, danakj wrote:
> nit: sorting

Done.

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc#newcode352
cc/gl_renderer.cc:352: static SkBitmap applyFilter(CCRendererGL* renderer,
SkImageFilter* filter, CCScopedTexture* sourceTexture)
On 2012/10/19 19:34:24, danakj wrote:
> can we rename this to applyImageFilter?

(I think I convinced Dana offline to let me keep it this way, since setFilters()
is going away.)

http://codereview.chromium.org/11175009/diff/2001/cc/gl_renderer.cc#newcode358
cc/gl_renderer.cc:358: GrContext* grContext = CCProxy::hasImplThread() ?
WebSharedGraphicsContext3D::compositorThreadGrContext() :
WebSharedGraphicsContext3D::mainThreadGrContext();
On 2012/10/19 19:15:30, jamesr wrote:
> this is exactly the same (with different variable names) as lines 339-343 in
> applyFilters() above. Can you figure out a way to share this code instead of
> copy-paste?

My goal is that when this is done, we'll just rip out the old code
(setFilters()) entirely, and we'll use SkImageFilter for everything.  So I
wasn't too concerned about refactoring.

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h
File cc/layer.h (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h#newcode100
cc/layer.h:100: void setFilter(SkImageFilter* filter);
On 2012/10/19 19:34:24, danakj wrote:
> same, setImageFilter?

Same as above.

http://codereview.chromium.org/11175009/diff/2001/cc/layer.h#newcode343
cc/layer.h:343: SkImageFilter* m_filter;
On 2012/10/19 19:15:30, jamesr wrote:
> Should this be a SkTAutoUnref<> or something of that nature?  What should
happen
> if a filter is set on a Layer and then the layer is destroyed before we
> pushPropertiesTo - we should drop a ref, right?

You're totally right.  Wow, what a doozy.

I've added explicit unrefs in the destructor so I can still do forward decl's,
and because Skia's smart pointers are so dumb.  :|

http://codereview.chromium.org/11175009/diff/2001/cc/layer_impl.h
File cc/layer_impl.h (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer_impl.h#newcode368
cc/layer_impl.h:368: SkImageFilter* m_filter;
On 2012/10/19 19:15:30, jamesr wrote:
> Same question here - are we supposed to unref in ~LayerImpl() ?

Done.

http://codereview.chromium.org/11175009/diff/2001/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/layer_tree_host_common.c...
cc/layer_tree_host_common.cc:581: if
(layer->filters().hasFilterThatMovesPixels() || layer->filter())
On 2012/10/19 19:34:24, danakj wrote:
> There are a few other places that check for this: occlusion tracker, damage
> tracker.

> I see you got the opacity check in occlusion tracker, but not the moves
pixels.

I think I got them all now.  Please let me know if I'm being appropriately
pessimistic.

> What does it mean for the filter outsets? In damage tracker and occlusion
> tracker we add damage/remove occlusion based on the filter outsets.
> 
> Can an image filter move pixels? Or does it just change opacity/color values
for
> each pixel indepdently. If it can move color values from one pixel into
another,
> how far away? Is it completely arbitrary?

An SkImageFilter can do anything it wants, really (although we only have a
limited set for SVG).  I think what we need to do is add movesPixels() and
affectsOpacity() virtuals to SkImageFilter, and do the right thing on all
subclasses over in Skia.

> If arbitrary, then any damage in the layer needs to damage the whole layer in
> damage tracker. Any non-occluded area on the layer needs to un-occlude the
> entire layer in the occlusion tracker.

Could you check if I'm being pessimistic enough yet?  (Until the above functions
are implemented).

> Maybe to help make common code we could add some templated static methods to
> RenderSurfaceFilters to make queries about what filter effects a layer has,
such
> as hasFilterThatMovesPixels(), hasFilterThatAffectsOpacity(), filterOutsets(),
> etc?

See above:  I think it needs to be on the SkImageFilter subclasses.

http://codereview.chromium.org/11175009/diff/2001/cc/render_pass.cc
File cc/render_pass.cc (right):

http://codereview.chromium.org/11175009/diff/2001/cc/render_pass.cc#newcode111
cc/render_pass.cc:111: }
On 2012/10/19 19:34:24, danakj wrote:
> Same question as james, who frees this one?

Done.

http://codereview.chromium.org/11175009/diff/2001/webkit/compositor_bindings/...
File webkit/compositor_bindings/web_layer_impl.h (right):

http://codereview.chromium.org/11175009/diff/2001/webkit/compositor_bindings/...
webkit/compositor_bindings/web_layer_impl.h:63: // TODO(senorblanco):  uncomment
OVERRIDE once WebKit changes have landed.
On 2012/10/19 19:15:30, jamesr wrote:
> No, leave the OVERRIDE out.  OVERRIDE's aren't worth it cross-repo (the rest
of
> this file has the OVERRIDEs due to historical accident)

Done.

[danakj, 2012-10-20 00:16:20]

On Fri, Oct 19, 2012 at 5:34 PM,  <senorblanco@chromium.org> wrote:
> Could you check if I'm being pessimistic enough yet?  (Until the above
> functions are implemented).

I don't see any change in the damage tracker except the function
signature. Missing an upload?

[Stephen White, 2012-10-20 00:18:27]

On 2012/10/19 19:15:30, jamesr wrote:
> Is SkImageFilter supposed to be instead of or in addition to the existing
> filters property?  Is there a backgroundFilter equivalent?

Not yet.  I've added it to the work items in the design doc.

> I can't tell where in the compositor code we're supposed to unref the
> SkImageFilter.

Should be fixed now.

> We have a software backend now too (cc/software_renderer.h).  It doesn't
support
> filters, but if it's easy to support please keep it in mind.

Good point; I've added an item to the design doc.  There are software
implementations for all the filters in Skia, so it should be pretty
straightforward.

[Stephen White, 2012-10-20 00:19:57]

On 2012/10/19 19:15:30, jamesr wrote:
> Is SkImageFilter supposed to be instead of or in addition to the existing
> filters property?

(whoops, forgot to answer this one).  Medium-term, I want to replace everything
with SkImageFilter.  In the short-term, they'll coexist.

[Stephen White, 2012-10-22 16:10:41]

On 2012/10/20 00:16:20, danakj wrote:
> On Fri, Oct 19, 2012 at 5:34 PM,  <mailto:senorblanco@chromium.org> wrote:
> > Could you check if I'm being pessimistic enough yet?  (Until the above
> > functions are implemented).
> 
> I don't see any change in the damage tracker except the function
> signature. Missing an upload?

Dunno what happened there (maybe I dreamed I wrote the code?)  Anyway, should be
there now.

[danakj, 2012-10-22 18:00:43]

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker.cc#newcode139
cc/damage_tracker.cc:139: else if (filter)
nit: add braces {}

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker_unittest.cc
File cc/damage_tracker_unittest.cc (right):

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker_unittest.cc...
cc/damage_tracker_unittest.cc:395: TEST_F(CCDamageTrackerTest,
verifyDamageForBackgroundBlurredChild)
Can you add a simple test that verifies if a filter is present, the entire layer
is damaged?

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.cc
File cc/render_pass.cc (right):

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.cc#newcode15
cc/render_pass.cc:15: #include "SkImageFilter.h"
nit: sorting after rebase

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.h
File cc/render_pass.h (right):

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.h#newcode117
cc/render_pass.h:117: SkImageFilter* m_filter;
This data type is passed over IPC, we can't add pointers here.

[Stephen White, 2012-10-22 18:51:00]

PTAL

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker.cc#newcode139
cc/damage_tracker.cc:139: else if (filter)
On 2012/10/22 18:00:43, danakj wrote:
> nit: add braces {}

Done.

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker_unittest.cc
File cc/damage_tracker_unittest.cc (right):

http://codereview.chromium.org/11175009/diff/19/cc/damage_tracker_unittest.cc...
cc/damage_tracker_unittest.cc:395: TEST_F(CCDamageTrackerTest,
verifyDamageForBackgroundBlurredChild)
On 2012/10/22 18:00:43, danakj wrote:
> Can you add a simple test that verifies if a filter is present, the entire
layer
> is damaged?

Done.

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.h
File cc/render_pass.h (right):

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.h#newcode117
cc/render_pass.h:117: SkImageFilter* m_filter;
On 2012/10/22 18:00:43, danakj wrote:
> This data type is passed over IPC, we can't add pointers here.

I understand that this is only a problem for the ubercompositor?  Longer-term,
the plan is to use Skia's serialization infrastructure:  SkImageFilter can
already be flattened via SkFlattenable, so it would just be a matter of writing
that over IPC.  I've added this as a work item on the design doc.

It's ok if the SkImageFilter doesn't work in the ubercompositor for now.  Will
the code as-is cause problems (eg., make ubercomp crash)?

[danakj, 2012-10-22 18:56:02]

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.h
File cc/render_pass.h (right):

http://codereview.chromium.org/11175009/diff/19/cc/render_pass.h#newcode117
cc/render_pass.h:117: SkImageFilter* m_filter;
On 2012/10/22 18:51:00, Stephen White wrote:
> On 2012/10/22 18:00:43, danakj wrote:
> > This data type is passed over IPC, we can't add pointers here.
> 
> I understand that this is only a problem for the ubercompositor?  Longer-term,
> the plan is to use Skia's serialization infrastructure:  SkImageFilter can
> already be flattened via SkFlattenable, so it would just be a matter of
writing
> that over IPC.  I've added this as a work item on the design doc.
> 
> It's ok if the SkImageFilter doesn't work in the ubercompositor for now.  Will
> the code as-is cause problems (eg., make ubercomp crash)?

Ok. We can serialize this as a 0 until then.

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker_unittest.cc
File cc/damage_tracker_unittest.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker_unittest....
cc/damage_tracker_unittest.cc:416: root->setUpdateRect(FloatRect(297, 297, 2,
2));
This is a copy/paste from background filters. Does this pass? Seems like it
shouldn't..

You want to setUpdateRect() on the child. And then the damageRect below and
above should be the same, I think? They are both the entire child layer's area.

[danakj, 2012-10-22 18:56:27]

+piman FYI for renderpass serialization.

[jamesr, 2012-10-22 19:14:17]

lgtm

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc#newcod...
cc/damage_tracker.cc:139: } else if (filter) {
Why is this "else"? should we DCHECK() that we don't have stuff in filters and
filter if we don't correctly handle both?

http://codereview.chromium.org/11175009/diff/1048/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/gl_renderer.cc#newcode361
cc/gl_renderer.cc:361: GrContext* grContext = CCProxy::hasImplThread() ?
WebSharedGraphicsContext3D::compositorThreadGrContext() :
WebSharedGraphicsContext3D::mainThreadGrContext();
please refactor this to share code with lines 342-343 in applyFilters(). This
context lookup stuff will need to be refactored soon and I don't want to have to
do it twice

http://codereview.chromium.org/11175009/diff/1048/cc/gl_renderer.cc#newcode502
cc/gl_renderer.cc:502: } else {
same comment here re 'else'

http://codereview.chromium.org/11175009/diff/14005/cc/damage_tracker_unittest.cc
File cc/damage_tracker_unittest.cc (right):

http://codereview.chromium.org/11175009/diff/14005/cc/damage_tracker_unittest...
cc/damage_tracker_unittest.cc:12: #include "SkBlurImageFilter.h"
here and everywher else with this include: we're using the
"third_party/skia/include/core/..." style paths for all #includes of skia code
to be in line with the rest of chromium code

[danakj, 2012-10-22 19:15:43]

lgtm also, thanks for the test!

[Stephen White, 2012-10-22 20:11:20]

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc#newcod...
cc/damage_tracker.cc:139: } else if (filter) {
On 2012/10/22 19:14:17, jamesr wrote:
> Why is this "else"? should we DCHECK() that we don't have stuff in filters and
> filter if we don't correctly handle both?

Yeah, I'll do that. The damage tracker is probably the wrong place, though. 
I'll do it in Layer::setFilter() and setFilters().

http://codereview.chromium.org/11175009/diff/1048/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/gl_renderer.cc#newcode361
cc/gl_renderer.cc:361: GrContext* grContext = CCProxy::hasImplThread() ?
WebSharedGraphicsContext3D::compositorThreadGrContext() :
WebSharedGraphicsContext3D::mainThreadGrContext();
On 2012/10/22 19:14:17, jamesr wrote:
> please refactor this to share code with lines 342-343 in applyFilters(). This
> context lookup stuff will need to be refactored soon and I don't want to have
to
> do it twice

Done.

http://codereview.chromium.org/11175009/diff/1048/cc/gl_renderer.cc#newcode502
cc/gl_renderer.cc:502: } else {
On 2012/10/22 19:14:17, jamesr wrote:
> same comment here re 'else'

Done in setFilter()/setFilters() (see above).

http://codereview.chromium.org/11175009/diff/14005/cc/damage_tracker_unittest.cc
File cc/damage_tracker_unittest.cc (right):

http://codereview.chromium.org/11175009/diff/14005/cc/damage_tracker_unittest...
cc/damage_tracker_unittest.cc:12: #include "SkBlurImageFilter.h"
On 2012/10/22 19:14:17, jamesr wrote:
> here and everywher else with this include: we're using the
> "third_party/skia/include/core/..." style paths for all #includes of skia code
> to be in line with the rest of chromium code

Done.

[danakj, 2012-10-22 20:12:45]

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc#newcod...
cc/damage_tracker.cc:139: } else if (filter) {
On 2012/10/22 20:11:20, Stephen White wrote:
> On 2012/10/22 19:14:17, jamesr wrote:
> > Why is this "else"? should we DCHECK() that we don't have stuff in filters
and
> > filter if we don't correctly handle both?
> 
> Yeah, I'll do that. The damage tracker is probably the wrong place, though. 
> I'll do it in Layer::setFilter() and setFilters().

Can you please also do it in LayerImpl?

[Stephen White, 2012-10-22 21:13:56]

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc
File cc/damage_tracker.cc (right):

http://codereview.chromium.org/11175009/diff/1048/cc/damage_tracker.cc#newcod...
cc/damage_tracker.cc:139: } else if (filter) {
On 2012/10/22 20:12:46, danakj wrote:
> On 2012/10/22 20:11:20, Stephen White wrote:
> > On 2012/10/22 19:14:17, jamesr wrote:
> > > Why is this "else"? should we DCHECK() that we don't have stuff in filters
> and
> > > filter if we don't correctly handle both?
> > 
> > Yeah, I'll do that. The damage tracker is probably the wrong place, though. 
> > I'll do it in Layer::setFilter() and setFilters().
> 
> Can you please also do it in LayerImpl?

Done.

[commit-bot: I haz the power, 2012-10-22 21:18:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/11175009/13020

[commit-bot: I haz the power, 2012-10-22 23:37:45]

Retried try job too often for step(s) browser_tests, interactive_ui_tests,
sync_integration_tests

[commit-bot: I haz the power, 2012-10-23 02:46:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/11175009/9012

[commit-bot: I haz the power, 2012-10-23 02:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/11175009/9012

[commit-bot: I haz the power, 2012-10-23 04:45:47]

Change committed as 163507