Bug 109098 – [CSS Filters] Filter outsets clipped on composited layers when filter is applied after first layout

• Component: Layout and Rendering
• Status: REOPENED

[Max Vujovic]

Created attachment 186928 [details]
Reproduction

See the attached reproduction. This only reproduces in Chrome, not in Safari.

Possibly related to bug 107708.

[Max Vujovic]

Corresponding Chromium bug: http://code.google.com/p/chromium/issues/detail?id=174788

I think I figured out the problem. RenderLayer::styleChanged does not notice a change in filter outsets and does not update the compositing bounds for Chromium. In there, we should call compositor()->setCompositingLayersNeedRebuild() if the outsets changed. I will assign this bug to myself and work on the fix there.

[Max Vujovic]

Created attachment 189209 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189209 [details]
Patch

Attachment 189209 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16639109

New failing tests:
platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-apply-hw-shadow.html
platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-move-hw-shadow.html
css3/filters/filter-change-repaint-composited.html

[Max Vujovic]

(In reply to comment #3)
> (From update of attachment 189209 [details])
> Attachment 189209 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16639109
> 
> New failing tests:
> platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-apply-hw-shadow.html
> platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-move-hw-shadow.html
> css3/filters/filter-change-repaint-composited.html

filter-change-repaint-composited.html is very similar to the tests I just added. It has an incorrect result checked in, so I will rebaseline its result and remove my tests.

[Max Vujovic]

Created attachment 190362 [details]
Patch for Bots

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/css3/filters/filter-change-repaint-composited.html', u'LayoutTests/platform/chromium-linux/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium-win/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp']" exit_code: 1
LayoutTests/css3/filters/filter-change-repaint-composited-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


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

[Max Vujovic]

Created attachment 191520 [details]
Patch for Bots

Let's see if the style-bot likes this patch.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/css3/filters/filter-change-repaint-composited.html', u'LayoutTests/platform/chromium-linux/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium-win/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp']" exit_code: 1
LayoutTests/css3/filters/filter-change-repaint-composited-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


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

[Alexandru Chiculita]

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

> Source/WebCore/ChangeLog:16
> +            When we update the graphics layer geometry, we should also call

You should explain the two problems that you fix with this patch: 
1. FilterOutsets are used by some of the compositors to calculate the sizes used by their backing surfaces. And they end up using old values.
2. We need to recompute the overlap map in order to promote the right layers after the outsets have been updated.

> Source/WebCore/rendering/RenderLayer.cpp:5619
> +        compositor()->setCompositingLayersNeedRebuild();

All the branches seem to have a single same line. Can we combine in only one branch?

You also need a test for the case when the layer is promoted to run in the GPU because it overlays some other content, You have two cases:
1) the filter used to be computed in software, so you need to check that it is applied on hardware and it is not applied twice (both software and hardware).
2) sometimes the filter can only be computed in software mode (ie. drop-shadow(..) sepia(..) on Mac), so it will need to preserve the software mode.

> Source/WebCore/rendering/RenderLayerBacking.cpp:584
> +    m_owningLayer->updateOrRemoveFilterEffectRenderer();

It might be better to put this in the updateFilters itself.
Looks like updateFilters is also called from the transitions code (RenderLayerBacking::startTransition). Should you patch that path too?  What happens in that case?

Is there a bug added for computing the overlap maps for filter animations/transitions correctly? I think that still reproduces.

[Max Vujovic]

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=190362&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +            When we update the graphics layer geometry, we should also call
> 
> You should explain the two problems that you fix with this patch: 
> 1. FilterOutsets are used by some of the compositors to calculate the sizes used by their backing surfaces. And they end up using old values.
> 2. We need to recompute the overlap map in order to promote the right layers after the outsets have been updated.

Yup, that's a great suggestion.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:5619
> > +        compositor()->setCompositingLayersNeedRebuild();
> 
> All the branches seem to have a single same line. Can we combine in only one branch?

Sure, I'll do that in my final patch.

> 
> You also need a test for the case when the layer is promoted to run in the GPU because it overlays some other content, You have two cases:
> 1) the filter used to be computed in software, so you need to check that it is applied on hardware and it is not applied twice (both software and hardware).
> 2) sometimes the filter can only be computed in software mode (ie. drop-shadow(..) sepia(..) on Mac), so it will need to preserve the software mode.

Yes, these are good tests. I'll make sure I have them in my final patch.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:584
> > +    m_owningLayer->updateOrRemoveFilterEffectRenderer();
> 
> It might be better to put this in the updateFilters itself.

Yeah, that would be better.

> Looks like updateFilters is also called from the transitions code (RenderLayerBacking::startTransition). Should you patch that path too?  What happens in that case?

The beginning or end of a transition can cause a filter render mode change (sw->hw painted filters or vice-versa). I realized we also need to call setContentsNeedDisplay() whenever the filter render mode changes. In a fix I'm working on, updateFilters calls setContentsNeedDisplay() in this situation. 

> 
> Is there a bug added for computing the overlap maps for filter animations/transitions correctly? I think that still reproduces.

I'll have to try that case out. I haven't been able to locate a bug for overlap map + filter animation, but I thought you filed one some time ago?

Locally, I've been seeing issues with filter outsets being clipped during animation. These issues are unrelated to overlap testing. I'm working on automating and expanding a test suite I've built locally that reproduces these issues. I've fixed the issue locally by adding a little more to this fix. I'll upload the tests in a patch when I have them automated.

[Max Vujovic]

Created attachment 192993 [details]
Patch for Bots

Trying out some additions to my fix on EWS to see if they break existing tests.

[Max Vujovic]

Created attachment 193000 [details]
Patch for Bots

[Simon Fraser (smfr)]

Comment on attachment 193000 [details]
Patch for Bots

The [Chromium] on the title seems wrong given that the patch touches cross-platform code.

[Max Vujovic]

(In reply to comment #13)
> (From update of attachment 193000 [details])
> The [Chromium] on the title seems wrong given that the patch touches cross-platform code.

[Max Vujovic]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 193000 [details] [details])
> > The [Chromium] on the title seems wrong given that the patch touches cross-platform code.

Yup- updated.

[Build Bot]

Comment on attachment 193000 [details]
Patch for Bots

Attachment 193000 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17140125

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
compositing/filters/sw-layer-overlaps-hw-shadow.html

[Build Bot]

Comment on attachment 193000 [details]
Patch for Bots

Attachment 193000 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16997782

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
compositing/filters/sw-layer-overlaps-hw-shadow.html

[Build Bot]

Comment on attachment 193000 [details]
Patch for Bots

Attachment 193000 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17120126

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
compositing/filters/sw-layer-overlaps-hw-shadow.html

[Max Vujovic]

Created attachment 193385 [details]
Patch for Bots (Work in Progress)

This patch is still a work in progress. I've added new tests to it, and I've added some ChangeLog comments (in case someone wants a preview explanation of the changes).

This patch makes layer bounds smaller on Safari when using composited blurs and drop-shadows. When Safari's CoreAnimation compositor paints filters, it computes filter outsets
internally, so I don't include them in the composited layer bounds anymore.

Currently, other platforms' compositors still need filter outsets included in their composited layer bounds. Thus, I've added different expectations for Chromium. I'm expecting failures from the other platforms that enable filters.

On a separate note, the conditions for calling setCompositingLayersNeedRebuild() in RenderLayer::styleChanged are getting even harder to read with this patch, as Alex noticed. I think the conditions should be factored into functions like:
- needsCompositingLayersRebuiltForClip
- needsCompositingLayersRebuiltForOverflow
- needsCompositingLayersRebuiltForFilters

I'll create a separate bug to do that refactor so this patch doesn't touch too much.

[Max Vujovic]

Created attachment 193909 [details]
Patch for Review

Updated the patch after the refactor in bug 112606. Added a few more comments in the ChangeLog entries.

[Alexandru Chiculita]

Comment on attachment 193909 [details]
Patch for Review

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

Looks good Max! Thanks for working on this.
I did a quick review on the code. A couple of nits on reorganizing the code.

> Source/WTF/wtf/Platform.h:928
> +/* FIXME: Once all platforms' compositors compute their own filter outsets, we should remove this define. */

Let's add a bug number to track this new flag. Eventually all platforms would need to do this and the flag would need to be removed.

> Source/WebCore/rendering/RenderLayer.cpp:6009
> +    if (oldStyle)
> +        oldOutsets = oldStyle->filterOutsets();

You should return false for oldStyle == 0, so maybe you want to promote that to the first if condition.

> Source/WebCore/rendering/RenderLayer.cpp:6012
> +    return (oldOutsets != newOutsets) || (didChangeFilterPaintMode && !newOutsets.isZero());

You could break this into 2 statements and add more comments about the cases that you handle.

> Source/WebCore/rendering/RenderLayer.cpp:6069
>      updateOrRemoveFilterClients();

These functions are expensive, maybe we could enclose them all in a boolean that checks if we had filters or going to have filters.

> Source/WebCore/rendering/RenderLayer.cpp:6076
> +    bool didChangeFilterPaintMode = false;
> +    bool isRunningAcceleratedFilterAnimation = renderer()->animation()->isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyWebkitFilter);
> +    if (isComposited() && !isRunningAcceleratedFilterAnimation)
> +        didChangeFilterPaintMode = backing()->updateFilters(renderer()->style());
> +#endif
> +    updateOrRemoveFilterEffectRenderer();

These lines could go into a separate function.

> Source/WebCore/rendering/RenderLayer.cpp:6086
> +        || needsCompositingLayersRebuiltForFilters(oldStyle, newStyle, isRunningAcceleratedFilterAnimation, didChangeFilterPaintMode)

You could use the same boolean here to guard against getting in filters code.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:751
> +    // If the compositor computes its own filter outsets, don't include them in the composited bounds.

Let's add a bug number to track this new flag. Eventually all platforms would need to do this.

[Max Vujovic]

Comment on attachment 193909 [details]
Patch for Review

Removing r? to address Alex's suggestions.

[EFL EWS Bot]

Comment on attachment 193909 [details]
Patch for Review

Attachment 193909 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17122732

[Max Vujovic]

Created attachment 194103 [details]
Patch for Review

New patch to address Alex's suggestions. Also, added TestExpectations entries for Qt, since Qt will need rebaselines for the new and updated tests.

[Max Vujovic]

Comment on attachment 193909 [details]
Patch for Review

Thanks for the suggestions, Alex! I've taken another pass at the code and tried to factor it better. I've also added the ideas from the ChangeLog as comments where appropriate.

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

>> Source/WTF/wtf/Platform.h:928
>> +/* FIXME: Once all platforms' compositors compute their own filter outsets, we should remove this define. */
> 
> Let's add a bug number to track this new flag. Eventually all platforms would need to do this and the flag would need to be removed.

Good idea. Added bug 112830.

>> Source/WebCore/rendering/RenderLayer.cpp:6009
>> +        oldOutsets = oldStyle->filterOutsets();
> 
> You should return false for oldStyle == 0, so maybe you want to promote that to the first if condition.

I've restructured this code quite a bit. I ended up combining the !oldStyle check with the (oldOutsets != newOutsets) check. It seems to make more sense there.

>> Source/WebCore/rendering/RenderLayer.cpp:6012
>> +    return (oldOutsets != newOutsets) || (didChangeFilterPaintMode && !newOutsets.isZero());
> 
> You could break this into 2 statements and add more comments about the cases that you handle.

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:6069
>>      updateOrRemoveFilterClients();
> 
> These functions are expensive, maybe we could enclose them all in a boolean that checks if we had filters or going to have filters.

Right. I've added a hasOrHadFilters(oldStyle, newStyle) helper function to help us exit early. I tried writing it with the boolean, but it didn't look as good.

>> Source/WebCore/rendering/RenderLayer.cpp:6076
>> +    updateOrRemoveFilterEffectRenderer();
> 
> These lines could go into a separate function.

Done: RenderLayer::updateFilters.

>> Source/WebCore/rendering/RenderLayer.cpp:6086
>> +        || needsCompositingLayersRebuiltForFilters(oldStyle, newStyle, isRunningAcceleratedFilterAnimation, didChangeFilterPaintMode)
> 
> You could use the same boolean here to guard against getting in filters code.

Now I call hasOrHadFilters in that function and exit early if it's false.

[Max Vujovic]

Comment on attachment 194103 [details]
Patch for Review

Canceling review. I realized some of combinations of my #if ENABLE(CSS_FILTERS) / #if USE(ACCELERATED_COMPOSITING) blocks won't work.

[EFL EWS Bot]

Comment on attachment 194103 [details]
Patch for Review

Attachment 194103 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17161899

[Stephen White]

Comment on attachment 194103 [details]
Patch for Review

Sorry I'm late to this patch.

If the problem is that the Chromium compositor doesn't compute filter outsets internally, that's something that could be fixed in the Chrome compositor, if it would make the Safari and Chrome codepaths closer.  It's something we've talked about doing for Skia (independently of Chrome).  Presumably, we still need to have a way to override the filter outsets in Skia, since SVG filters on SVG content for example have non-automatic bounds, as I understand it.

(Note that I'm not against this patch as-is, just that it seems unfortunate to diverge if Chrome is the outlier.)

[Max Vujovic]

(In reply to comment #28)
> (From update of attachment 194103 [details])
> Sorry I'm late to this patch.

No worries. Thanks for looking! :)

> 
> If the problem is that the Chromium compositor doesn't compute filter outsets internally, that's something that could be fixed in the Chrome compositor, if it would make the Safari and Chrome codepaths closer.  

Computing filter outsets in the Chromium compositor was the next problem Alex and I wanted to tackle after this patch. This patch is intended to set up that work. The idea is, after Chromium can compute its own filter outsets, we can add Chromium to the HAVE(COMPOSITOR_FILTER_OUTSETS) flag. Chromium is in the majority right now- all of the compositors besides CoreAnimation (e.g. Qt) can't compute their own filter outsets yet.

Luckily, the codepaths aren't too different. I use the flag in two places. For platforms with compositor filter outsets:
(1) Avoid including filter outsets in the composited layer bounds.
(2) Invalidate the compositing layer tree if compositor-painted filters fall back to software painting, since WebKit will be drawing the filters and needs a bigger backing. (Also, vice-versa.)

> It's something we've talked about doing for Skia (independently of Chrome).  Presumably, we still need to have a way to override the filter outsets in Skia, since SVG filters on SVG content for example have non automatic bounds, as I understand it.
> 
> (Note that I'm not against this patch as-is, just that it seems unfortunate to diverge if Chrome is the outlier.)

Chrome is the majority :). This patch tries to fix a couple of things that are pretty coupled:
(1) Filter outset changes go unnoticed by WebKit
(2) We always include filter outsets in the composited layer bounds.
(3) When filters with outsets fall back to software painting, we don't update the layer's compositing bounds.

[Stephen White]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 194103 [details] [details])
> > Sorry I'm late to this patch.
> 
> No worries. Thanks for looking! :)
> 
> > 
> > If the problem is that the Chromium compositor doesn't compute filter outsets internally, that's something that could be fixed in the Chrome compositor, if it would make the Safari and Chrome codepaths closer.  
> 
> Computing filter outsets in the Chromium compositor was the next problem Alex and I wanted to tackle after this patch. This patch is intended to set up that work. The idea is, after Chromium can compute its own filter outsets, we can add Chromium to the HAVE(COMPOSITOR_FILTER_OUTSETS) flag. Chromium is in the majority right now- all of the compositors besides CoreAnimation (e.g. Qt) can't compute their own filter outsets yet.
> 
> Luckily, the codepaths aren't too different. I use the flag in two places. For platforms with compositor filter outsets:
> (1) Avoid including filter outsets in the composited layer bounds.
> (2) Invalidate the compositing layer tree if compositor-painted filters fall back to software painting, since WebKit will be drawing the filters and needs a bigger backing. (Also, vice-versa.)
> 
> > It's something we've talked about doing for Skia (independently of Chrome).  Presumably, we still need to have a way to override the filter outsets in Skia, since SVG filters on SVG content for example have non automatic bounds, as I understand it.
> > 
> > (Note that I'm not against this patch as-is, just that it seems unfortunate to diverge if Chrome is the outlier.)
> 
> Chrome is the majority :). This patch tries to fix a couple of things that are pretty coupled:
> (1) Filter outset changes go unnoticed by WebKit
> (2) We always include filter outsets in the composited layer bounds.
> (3) When filters with outsets fall back to software painting, we don't update the layer's compositing bounds.

Sounds great.  Thanks for the explanation.

[Max Vujovic]

Created attachment 194131 [details]
Patch for Review

Fixed up the #if / #endif issues. Reworded the comments in the tests to clarify when we're testing something Safari specific (e.g. Safari falls back to painting filters in software when drop-shadow is not the last filter in the chain).

[Max Vujovic]

CC-ing Dean because he might find this patch interesting.

[Max Vujovic]

Created attachment 196194 [details]
Patch for Landing

Rebased patch for landing. Running it through EWS.

[WebKit Review Bot]

Comment on attachment 196194 [details]
Patch for Landing

Attachment 196194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17376264

New failing tests:
css3/filters/composited-layer-child-bounds-after-composited-to-sw-shadow-change.html
css3/filters/composited-layer-promotion-after-outset-overlap-change-using-composited-shadow.html
css3/filters/composited-layer-promotion-after-outset-overlap-change-using-sw-shadow.html

[WebKit Review Bot]

Created attachment 196219 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Max Vujovic]

Created attachment 196221 [details]
Patch for Landing

Updated Chromium expectations for new tests. Since my last update, DRT has started printing "(contentsOpaque 1)" in some of the test results.

[Max Vujovic]

Comment on attachment 196221 [details]
Patch for Landing

Chromium EWS looks good. Setting cq+.

[WebKit Review Bot]

Comment on attachment 196221 [details]
Patch for Landing

Clearing flags on attachment: 196221

Committed r147502: <http://trac.webkit.org/changeset/147502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke animations of CAFilters on Mac.

[Max Vujovic]

(In reply to comment #40)
> This broke animations of CAFilters on Mac.

Linking to bug 114067 for more details.

[Max Vujovic]

Reopening this bug because its changeset was reverted in bug 114067.

I believe the next steps for this bug are to automate and check in the regression test that smfr provided. See bug 114179.

[Csaba Osztrogonác]

Comment on attachment 194131 [details]
Patch for Review

Cleared Dean Jackson's review+ from obsolete attachment 194131 [details] so that this bug does not appear in http://webkit.org/pending-commit.