Bug 75842

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

Created attachment 121668 [details] Patch V1

Created attachment 121670 [details] Patch V2 This time with "git diff --binary" :)

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.

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?

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

Created attachment 121851 [details] Patch V3

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.

Created attachment 121855 [details] Patch V4 Fixing code style.

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.

(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.

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.

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.

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