Bug 75842 – CSS Filters: apply the filters in RenderLayerBacking::paintIntoLayer when filters cannot be composited in hardware

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

If the platform decides it cannot render a list of filters, WebKit fallbacks to software. However, RenderLayerBacking::paintIntoLayer doesn't take into account the filters when backing the texture of the layer, so there will be no filter applied.

This was triggered by https://bugs.webkit.org/show_bug.cgi?id=75658 which forced all the layers with filters use composited mode. The issue could be reproduced before using other techniques to trigger the composited mode.

Currently, only CSS Shaders on Mac render using only the software pipeline.

[Noam Rosenthal]

I was under the impression that other ports that have CSS_FILTERS compiled don't turn on the compositing trigger for filters yet.

[Alexandru Chiculita]

Created attachment 121668 [details]
Patch V1

[Alexandru Chiculita]

Created attachment 121670 [details]
Patch V2

This time with "git diff --binary" :)

[Chris Marrin]

Comment on attachment 121670 [details]
Patch V2

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

r- for the multiple calls to filtersCanBeComposited. I'd also like Simon's take on the rest...

> Source/WebCore/ChangeLog:6
> +        Layers have two possible states: composited or not. When composited the RenderLayerBacking::paintIntoLayer is used

You should say "RenderLayers have two possible states", to avoid confusion with the underlying composited layer implementations

> Source/WebCore/ChangeLog:7
> +        to render the result inside a memory buffer. When not composited the RenderLayer::paintLayer method will draw

Painting doesn't necessarily go into a memory buffer. It is painted into the passed GraphicsContext, which paints to the approrpiate platform specific place.

> Source/WebCore/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

Please put this at the top, right under the bug URL.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:711
> +    if (!filtersCanBeComposited(filters)) {

This will cause filtersCanBeComposited() to be called ar least twice for hardware accelerated layers: once in GraphicsLayerCA::setFilters and once here. This function is non-trivial now and will get more expensive as we handle more cases. I don't see why this is needed. If filtersCanBeComposited() returned false from GraphicsLayerCA then this call should be made with a null filter list and the check at the top of the function should take care of getting rid of any leftover filters. If that is not happening (setFilters(null) is not getting called when filters can't be composited) then the error is up in GraphicsLayer or RenderLayerBacking and should be dealt with there.

Either way, you should avoid multiple calls to filtersCanBeComposited() per style change.

> Source/WebCore/rendering/RenderLayer.cpp:2768
> +       && (!isComposited() || backing()->paintingGoesToWindow() || shouldDoSoftwarePaint(this, paintFlags & PaintLayerPaintingReflection))) {

This is an awfully confusing if test. I'd like Simon's take on how it can be better structured.

[Simon Fraser (smfr)]

Comment on attachment 121670 [details]
Patch V2

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:1184
> +#if ENABLE(CSS_FILTERS)
> +    GraphicsContext* savedLayerGraphicsContext = 0;
> +    bool applySoftwareFilter = m_owningLayer->paintsWithFilters();
> +    if (applySoftwareFilter) {
> +        // For some reason the platform code decided not to render the filters
> +        // using the composited mode, but we could apply the filter in software here.
> +        // Note: Child layers that are composited will not be captured here, so they
> +        // will not apply the filters.
> +        FilterEffectRenderer* filter = m_owningLayer->filter();
> +        ASSERT(filter);
> +        
> +        FloatRect filterSourceRect = layerBounds;
> +        filterSourceRect.setLocation(LayoutPoint());
> +        m_owningLayer->updateFilterBackingStore(filterSourceRect);
> +        filter->prepare();
> +        
> +        if (filter->inputContext()) {
> +            savedLayerGraphicsContext = context;
> +            context = filter->inputContext();
> +            context->clearRect(filterSourceRect);
> +        }
> +    }
> +#endif

Rather than add yet another call to filter code here, why not make RenderLayer::paintsWithFilters() smarter?

[Alexandru Chiculita]

Comment on attachment 121670 [details]
Patch V2

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

>> Source/WebCore/rendering/RenderLayer.cpp:2768
>> +       && (!isComposited() || backing()->paintingGoesToWindow() || shouldDoSoftwarePaint(this, paintFlags & PaintLayerPaintingReflection))) {
> 
> This is an awfully confusing if test. I'd like Simon's take on how it can be better structured.

This is actually trying to avoid applying the filter when the RenderLayer::paintLayerContents returns early in the #if USE(ACCELERATED_COMPOSITING) block. I will try to refactor it and also enclose it into #if USE(ACCELERATED_COMPOSITING).  

BTW, I think paintsWithFilters() should be renamed to something else. It's a little confusing when filters are composited in hardware. Is that ok with you?

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1184
>> +#endif
> 
> Rather than add yet another call to filter code here, why not make RenderLayer::paintsWithFilters() smarter?

Those two methods do mostly the same thing (RenderLayerBacking::paintIntoLayer and RenderLayer::paintLayer). I can try to merge them into a single method. Do you want me add a new bug and fix that first?

Otherwise, I could just create two methods (or wrap them in a helper class) that:
1. will prepare the filter and save the old context
2. another one that will apply it back to the original context

[Alexandru Chiculita]

Created attachment 121851 [details]
Patch V3

[WebKit Review Bot]

Attachment 121851 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1

Source/WebCore/rendering/FilterEffectRenderer.h:67:  The parameter name "renderLayer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Created attachment 121855 [details]
Patch V4

Fixing code style.

[Simon Fraser (smfr)]

The real issue here is that RenderLayerBacking::paintIntoLayer() duplicates code in RenderLayer::paintLayer()/paintLayerContents(). If we could refactor to avoid duplication, fixing this would be simpler.

[Alexandru Chiculita]

(In reply to comment #10)
> The real issue here is that RenderLayerBacking::paintIntoLayer() duplicates code in RenderLayer::paintLayer()/paintLayerContents(). If we could refactor to avoid duplication, fixing this would be simpler.

I added https://bugs.webkit.org/show_bug.cgi?id=75983 to track that change. That seems to be a larger problem to solve and may require multiple patches. I would be happy to fix it after finishing with this bug.

[Simon Fraser (smfr)]

Comment on attachment 121855 [details]
Patch V4

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

> LayoutTests/css3/filters/effect-blur.html:4
> +    window.layoutTestController.overridePreference("WebKitAcceleratedCompositingEnabled", "0");

This should not longer be necessary now that bug 75658 was fixed.

> LayoutTests/css3/filters/effect-custom.html:35
> +        <p style="color: blue">
> +            Testing that simple custom filters work in software mode. You should see 5 blocks of colored bars with different effects applied, from left to right: 
> +            offset to the right with washed out colors, just offset to the right with normal colors, 4 smaller blocks attached to the corners, 4 smaller blocks 
> +            attached to the corners with washed out colors, normal block with washed out colors.
> +            Note: WebGL needs to be enabled for this test to succeed.

It's best to avoid text in pixel tests if possible, because text rendering differs between OSes and OS versions so much.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:582
> +        DEFINE_STATIC_LOCAL(FilterOperations, emptyFilterList, ());
> +        GraphicsLayer::setFilters(emptyFilterList);

I think we should add a clearFilters() method rather than this hack.

[Alexandru Chiculita]

Comment on attachment 121855 [details]
Patch V4

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

>> LayoutTests/css3/filters/effect-blur.html:4
>> +    window.layoutTestController.overridePreference("WebKitAcceleratedCompositingEnabled", "0");
> 
> This should not longer be necessary now that bug 75658 was fixed.

I think we still need it to test that the "software" path is still hit. If I remove that all the tests will use CoreImage filters instead and the new code is not tested. There are some tests that will check the composited mode. That tests have a "-hw.html" suffix in the name.

>> LayoutTests/css3/filters/effect-custom.html:35
>> +            Note: WebGL needs to be enabled for this test to succeed.
> 
> It's best to avoid text in pixel tests if possible, because text rendering differs between OSes and OS versions so much.

In the review for https://bugs.webkit.org/show_bug.cgi?id=74652 I was asked to add some text :) I've just changed it to a comment.

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:582
>> +        GraphicsLayer::setFilters(emptyFilterList);
> 
> I think we should add a clearFilters() method rather than this hack.

Ok.

[Alexandru Chiculita]

Landed in http://trac.webkit.org/changeset/104698. Closing bug.