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.

Created attachment 189209 [details] Patch

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

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

Created attachment 190362 [details] Patch for Bots

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.

Created attachment 191520 [details] Patch for Bots Let's see if the style-bot likes this patch.

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.

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.

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

Created attachment 192993 [details] Patch for Bots Trying out some additions to my fix on EWS to see if they break existing tests.

Created attachment 193000 [details] Patch for Bots

Comment on attachment 193000 [details] Patch for Bots The [Chromium] on the title seems wrong given that the patch touches cross-platform code.

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

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

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

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

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

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.

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.

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.

Comment on attachment 193909 [details] Patch for Review Removing r? to address Alex's suggestions.

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

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.

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.

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.

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

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

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

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

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

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

Created attachment 196194 [details] Patch for Landing Rebased patch for landing. Running it through EWS.

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

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

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.

Comment on attachment 196221 [details] Patch for Landing Chromium EWS looks good. Setting cq+.

Comment on attachment 196221 [details] Patch for Landing Clearing flags on attachment: 196221 Committed r147502: <http://trac.webkit.org/changeset/147502>

All reviewed patches have been landed. Closing bug.

This broke animations of CAFilters on Mac.

(In reply to comment #40) > This broke animations of CAFilters on Mac. Linking to bug 114067 for more details.

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.

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.