Bug 77266 – [chromium] Implement CSS filters on composited layers.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Stephen White]

[chromium] Implement CSS filters on composited layers.

[Stephen White]

Created attachment 124412 [details]
Patch

[Stephen White]

Comment on attachment 124412 [details]
Patch

James || Enne || Vangelis, could you take a look?

[James Robinson]

Comment on attachment 124412 [details]
Patch

I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.

Can we move the drawing logic into its own file? it's a lot of LOC.

Do we always have a ganesh context on the compositor context?  What does that imply resource-wise?

[Stephen White]

(In reply to comment #3)
> (From update of attachment 124412 [details])
> I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> 
> Can we move the drawing logic into its own file? it's a lot of LOC.

Can do.


> Do we always have a ganesh context on the compositor context?  What does that imply resource-wise?

No, it's created lazily, at least according to GraphicsContext3DPrivate::grContext().

[Dana Jansens]

(In reply to comment #3)
> (From update of attachment 124412 [details])
> I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.

For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.
https://bugs.webkit.org/attachment.cgi?id=124751&action=review

Blur is a headache.. IIUC what filters are doing, a blur on a RenderSurface would make a boundary where occlusion computation from outside the RenderSurface's subtree would not be valid inside the subtree and vice versa. Line 467 in the above CL should not copy occlusion into such a subtree but detecting it here is not obvious.

What if we put some properties on RenderSurfaces that are computed in calcDrawTransformsAndVisibility?
 - has opaque-destroying filter
 - is in blur subtree (a pointer to the nearest blurring ancestor RenderSurface)

[Stephen White]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 124412 [details] [details])
> > I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> 
> For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.

For now, could we simply be uber-conservative, and assume any layer with a filter is entirely opaque?  Then we can refine that in subsequent work.

> https://bugs.webkit.org/attachment.cgi?id=124751&action=review
> 
> Blur is a headache.. IIUC what filters are doing, a blur on a RenderSurface would make a boundary where occlusion computation from outside the RenderSurface's subtree would not be valid inside the subtree and vice versa. Line 467 in the above CL should not copy occlusion into such a subtree but detecting it here is not obvious.
> 
> What if we put some properties on RenderSurfaces that are computed in calcDrawTransformsAndVisibility?
>  - has opaque-destroying filter
>  - is in blur subtree (a pointer to the nearest blurring ancestor RenderSurface)

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 124412 [details] [details])
> > I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> 
> For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.
> https://bugs.webkit.org/attachment.cgi?id=124751&action=review
> 
> Blur is a headache.. IIUC what filters are doing, a blur on a RenderSurface would make a boundary where occlusion computation from outside the RenderSurface's subtree would not be valid inside the subtree and vice versa. Line 467 in the above CL should not copy occlusion into such a subtree but detecting it here is not obvious.
> 
> What if we put some properties on RenderSurfaces that are computed in calcDrawTransformsAndVisibility?
>  - has opaque-destroying filter
>  - is in blur subtree (a pointer to the nearest blurring ancestor RenderSurface)

[Stephen White]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 124412 [details] [details] [details])
> > > I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> > 
> > For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.
> 
> For now, could we simply be uber-conservative, and assume any layer with a filter is entirely opaque?  Then we can refine that in subsequent work.

Er, non-opaque.  Whatever the conservative assumption would be.  :)

> > https://bugs.webkit.org/attachment.cgi?id=124751&action=review
> > 
> > Blur is a headache.. IIUC what filters are doing, a blur on a RenderSurface would make a boundary where occlusion computation from outside the RenderSurface's subtree would not be valid inside the subtree and vice versa. Line 467 in the above CL should not copy occlusion into such a subtree but detecting it here is not obvious.
> > 
> > What if we put some properties on RenderSurfaces that are computed in calcDrawTransformsAndVisibility?
> >  - has opaque-destroying filter
> >  - is in blur subtree (a pointer to the nearest blurring ancestor RenderSurface)
> 
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 124412 [details] [details] [details])
> > > I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> > 
> > For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.
> > https://bugs.webkit.org/attachment.cgi?id=124751&action=review
> > 
> > Blur is a headache.. IIUC what filters are doing, a blur on a RenderSurface would make a boundary where occlusion computation from outside the RenderSurface's subtree would not be valid inside the subtree and vice versa. Line 467 in the above CL should not copy occlusion into such a subtree but detecting it here is not obvious.
> > 
> > What if we put some properties on RenderSurfaces that are computed in calcDrawTransformsAndVisibility?
> >  - has opaque-destroying filter
> >  - is in blur subtree (a pointer to the nearest blurring ancestor RenderSurface)

[Dana Jansens]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #3)
> > > > (From update of attachment 124412 [details] [details] [details] [details])
> > > > I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> > > 
> > > For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.
> > 
> > For now, could we simply be uber-conservative, and assume any layer with a filter is entirely opaque?  Then we can refine that in subsequent work.
> 
> Er, non-opaque.  Whatever the conservative assumption would be.  :)

Yes that's fine, but it doesn't work for blur. We do need to know when we're entering a subtree of a RenderSurface that has blur (aka looking at a layer that will eventually be blurred at some point).

[Stephen White]

Created attachment 124833 [details]
move filters application to helper class

[Adrienne Walker]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #3)
> > > > > (From update of attachment 124412 [details] [details] [details] [details] [details])
> > > > > I think culling / occlusions need to be aware of filters since blur can cause different source pixels to be referenced and other filters can change opacity.
> > > > 
> > > > For opacity changes, line 510 of CCLayerTreeHost.cpp in the below CL needs to know if the target surface has a filter that would make any opaque pixels non-opaque. This CL is in CQ so should end up on ToT shortly.
> > > 
> > > For now, could we simply be uber-conservative, and assume any layer with a filter is entirely opaque?  Then we can refine that in subsequent work.
> > 
> > Er, non-opaque.  Whatever the conservative assumption would be.  :)
> 
> Yes that's fine, but it doesn't work for blur. We do need to know when we're entering a subtree of a RenderSurface that has blur (aka looking at a layer that will eventually be blurred at some point).

On the draw culling side of things, we don't cull outside of a render surface, so blur shouldn't affect anything at the moment.  On the paint culling side, does a blur need to be handled much differently than a mask would be? They're both modifications to the opaque regions of a surface, and we can ignore them both for now.

(I don't really have additional comments about this patch; it looks good in general to me!)

[Dana Jansens]

Blur is only different because it pushes occluded pixels out from behind occlusion. So the accumulated occlusion needs to account for that before it gets to the blurred stuff.

[Vangelis Kokkevis]

Comment on attachment 124833 [details]
move filters application to helper class

View in context: https://bugs.webkit.org/attachment.cgi?id=124833&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:287
> +            renderSurface->setFilters(layer->filters());

You'll need to call setFilters regardless of useSurfaceForFilters so that you can clear the filters from an existing RS if the layer ceases to have filters.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:239
> +    // FIXME: Put this in a "LayerRenderChromium::resetContext()" to

This seems pretty fragile indeed.  I would much prefer to have implement the LRC::resetContext() method.  There's some more state that gets set during rendering including the bound vertex and index arrays, scissoring, etc.  We need to make sure that they can be preserved.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:154
> +    GrContext* gr = context3D->grContext();

We'll need to make sure that the resources allocated by this GrContext are also released when the compositor is asked to free up its textures.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:168
> +    source.setPixelRef(new SkGrTexturePixelRef(texture.get()))->unref();

You'll need to make sure that if filters.size() == 0 then you don't end up wrapping the original RS (managed) texture with an SkBitmap.

[Stephen White]

Comment on attachment 124833 [details]
move filters application to helper class

View in context: https://bugs.webkit.org/attachment.cgi?id=124833&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:287
>> +            renderSurface->setFilters(layer->filters());
> 
> You'll need to call setFilters regardless of useSurfaceForFilters so that you can clear the filters from an existing RS if the layer ceases to have filters.

Thanks, fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:239
>> +    // FIXME: Put this in a "LayerRenderChromium::resetContext()" to
> 
> This seems pretty fragile indeed.  I would much prefer to have implement the LRC::resetContext() method.  There's some more state that gets set during rendering including the bound vertex and index arrays, scissoring, etc.  We need to make sure that they can be preserved.

Done.  The buffer bindings should be restored by the call to GeometryBinding::prepareForDraw().

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> 2012

Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:154
>> +    GrContext* gr = context3D->grContext();
> 
> We'll need to make sure that the resources allocated by this GrContext are also released when the compositor is asked to free up its textures.

James and I were talking about that, and we think it could be handled by the mechanism that twiz is working on to reset the GrContext associated with a given GraphicsContext3D.  I can't trigger it from the compositor right now, since just asking for a grContext() will lazily create it, even if all we want to do is flush its resources.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:168
>> +    source.setPixelRef(new SkGrTexturePixelRef(texture.get()))->unref();
> 
> You'll need to make sure that if filters.size() == 0 then you don't end up wrapping the original RS (managed) texture with an SkBitmap.

This function is only called when filters.size() > 0 (previously in CCRenderSurface::draw(), I've moved that to CCRenderSurface::applyFilters()).

[Stephen White]

Created attachment 124960 [details]
Patch

[Stephen White]

(In reply to comment #14)
> Created an attachment (id=124960) [details]
> Patch

This addresses Vangelis's comments, and also removes m_filterBitmap in favour of a parameter plumbed through drawSurface() and drawLayer().  Since the filtered bitmap is not currently cached anyway, this makes the lifetime issues a bit clearer.

This does not yet address Dana's comments, since I haven't been able to reproduce the blur problem yet.

[Vangelis Kokkevis]

Comment on attachment 124960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124960&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791
> +    GLC(context3D, m_context->activeTexture(GraphicsContext3D::TEXTURE0));

It's set at LRC::beginDrawingFrame() sets some more state (colorMask, blendFunc, DEPTH_TEST, CULL_FACE, etc).  I think right now some of it is being re-set before we draw a quad (blendFunc, blend enabled) but some is not.  We need to find the exact state we depend on setting and move it all to this method.  beginDrawingFrame() should call this as well. 
I'm still concerned that any change in the Ganesh code could break the compositor if a state is set in a way we don't expect.  Should we create an additional context to handle filters if we're running the compositor on the thread and re-using the canvas shared context if we're singlethreaded?

[Stephen White]

(In reply to comment #16)
> (From update of attachment 124960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124960&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791
> > +    GLC(context3D, m_context->activeTexture(GraphicsContext3D::TEXTURE0));
> 
> It's set at LRC::beginDrawingFrame() sets some more state (colorMask, blendFunc, DEPTH_TEST, CULL_FACE, etc).  I think right now some of it is being re-set before we draw a quad (blendFunc, blend enabled) but some is not.  We need to find the exact state we depend on setting and move it all to this method.  beginDrawingFrame() should call this as well. 

That sounds like a good idea.  However, is that something we need to do in this change, or something that could wait for a followup?  If it should be done now, could you send a list of all the compositor state you'd like to see reset?

> I'm still concerned that any change in the Ganesh code could break the compositor if a state is set in a way we don't expect. 

Yes, I fully expect that the filters may have state change bugs which will be on me to fix, but if you don't use -webkit-filter, I don't think this patch will affect the compositor.  Note that accelerated painting suffers from some of the same problems.

> Should we create an additional context to handle filters if we're running the compositor on the thread and re-using the canvas shared context if we're singlethreaded?

It could be done (although there was a reason I decided against it that I can't remember right now).    The other issue is that canvas shared context creation is all handled through ImageBuffers right now.  I wasn't sure if that dependency would be a problem for GTFO.  It also makes it a little tricky to extract out the backing store and have it live longer than the ImageBuffer which creates it.

[Stephen White]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 124960 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124960&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791
> > > +    GLC(context3D, m_context->activeTexture(GraphicsContext3D::TEXTURE0));
> > 
> > It's set at LRC::beginDrawingFrame() sets some more state (colorMask, blendFunc, DEPTH_TEST, CULL_FACE, etc).  I think right now some of it is being re-set before we draw a quad (blendFunc, blend enabled) but some is not.  We need to find the exact state we depend on setting and move it all to this method.  beginDrawingFrame() should call this as well. 
> 
> That sounds like a good idea.  However, is that something we need to do in this change, or something that could wait for a followup?  If it should be done now, could you send a list of all the compositor state you'd like to see reset?
> 
> > I'm still concerned that any change in the Ganesh code could break the compositor if a state is set in a way we don't expect. 
> 
> Yes, I fully expect that the filters may have state change bugs which will be on me to fix, but if you don't use -webkit-filter, I don't think this patch will affect the compositor.  Note that accelerated painting suffers from some of the same problems.
> 
> > Should we create an additional context to handle filters if we're running the compositor on the thread and re-using the canvas shared context if we're singlethreaded?
> 
> It could be done (although there was a reason I decided against it that I can't remember right now).    The other issue is that canvas shared context creation is all handled through ImageBuffers right now.  I wasn't sure if that dependency would be a problem for GTFO.  It also makes it a little tricky to extract out the backing store and have it live longer than the ImageBuffer which creates it.

Forget everything I said.  :)

I just realized I don't need any of that ImageBuffer stuff.  I can just use the SharedGraphicsContext3D (and the GrContext on it) directly, and there should be no changes necessary for the filter code at all.  I'll give that a try tomorrow.

For the threaded case, I just need to figure out who will own/create the GC3D it'll use.  I think it should live as long as the one for the compositor, but I'm not sure which class should own it.  Alternatively, I could just leave the feature off in the threaded case for now.

[Stephen White]

Created attachment 126120 [details]
Patch

[Stephen White]

(In reply to comment #19)
> Created an attachment (id=126120) [details]
> Patch

This version uses the SharedGraphicsContext3D as suggested, and leaves the compositor context state alone.  I needed to add a flush on each context change in order to get the texture contents up-to-date (this is expected)).  Filters are disabled for the threaded compositor case for now.

[James Robinson]

Comment on attachment 126120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126120&action=review

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:310
> +    m_filters = filters;

can you compare FilterOperations for equality? our normal pattern for state setters like this is to early-out if the new value is the same as the old, so that even if WebKit pushes this state more frequently than this actually changes we don't produce unnecessary frames

when _does_ webkit push new filters?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:231
> +    // Don't use the utility context if we have a compositor thread, since
> +    // it can race with canvas's use.

can you file a bug on this?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.h:38
> +class CCRenderSurfaceFilters {

nit: newline before class declaration, please.

is class this just for namespacing? if so can you hide the c'tor in a private section so people don't attempt to instantiate it?

[Stephen White]

(In reply to comment #21)
> (From update of attachment 126120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126120&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:310
> > +    m_filters = filters;
> 
> can you compare FilterOperations for equality? our normal pattern for state setters like this is to early-out if the new value is the same as the old, so that even if WebKit pushes this state more frequently than this actually changes we don't produce unnecessary frames

Yes, it does seem to have an operator==.  I'll add that check.

> when _does_ webkit push new filters?

Good question.  Often enough?  :)

> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:231
> > +    // Don't use the utility context if we have a compositor thread, since
> > +    // it can race with canvas's use.
> 
> can you file a bug on this?

Done.  https://bugs.webkit.org/show_bug.cgi?id=78139

> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.h:38
> > +class CCRenderSurfaceFilters {
> 
> nit: newline before class declaration, please.

> is class this just for namespacing?

See Comment #3.

> if so can you hide the c'tor in a private section so people don't attempt to instantiate it?

Will do.

[James Robinson]

Comment on attachment 126120 [details]
Patch

OK, R=me with those changes then.

[Stephen White]

Committed r107480: <http://trac.webkit.org/changeset/107480>