Bug 80323

Created attachment 130194 [details] Test file

Created attachment 130195 [details] Screenshot

Created attachment 131921 [details] Patch V1 The patch may seem to fix multiple bugs, but they are only a consequence of the fact that now I made the filtered layers act as repaint container. That is similar to how composited layers work. Using that we can intercept all the repaints in a single place. Also in order to make those repaints useful (and testable) I needed to fix the filter painting to keep a full source image of the original RenderLayer, so that drop shadow and blur can apply correctly.

Created attachment 131927 [details] Patch V2 Rebased the patch.

Comment on attachment 131927 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=131927&action=review I think this patch is too big. You should break it up, and fix one part at a time. > Source/WebCore/ChangeLog:11 > + The problem is that shadow and blur (and custom filters - although not treated in this patch) need the full source image of > + the surface that needs to be filtered. Until now the filter was computed only using the area defined by the dirty repaint rectangle. > + Those filters need full image source because they displace pixel positions, meaning that pixels in the current dirty rectangle > + have a dependency on pixels from the RenderLayer outside the dirty rect. See the bug pictures for an example of how that could go wrong. But that should already be taken care of by the inflation of the dirty rect, no?

(In reply to comment #5) > (From update of attachment 131927 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131927&action=review > > I think this patch is too big. You should break it up, and fix one part at a time. There are only two things that I do in this patch: 1. Intercept the repaint rectangle for filtered layers 2. Repaint using the intercepted rectangle instead To me it doesn't feel like one could go without the other. There are some consequences of doing #1 above: I had to remove the rect inflations from all the places and do them in a single place. > > > Source/WebCore/ChangeLog:11 > > + The problem is that shadow and blur (and custom filters - although not treated in this patch) need the full source image of > > + the surface that needs to be filtered. Until now the filter was computed only using the area defined by the dirty repaint rectangle. > > + Those filters need full image source because they displace pixel positions, meaning that pixels in the current dirty rectangle > > + have a dependency on pixels from the RenderLayer outside the dirty rect. See the bug pictures for an example of how that could go wrong. > > But that should already be taken care of by the inflation of the dirty rect, no? Rect inflation alone doesn't fix it. Look at the screenshot that I've attached to this bug. Take an example of 3 rectangles and a drop shadow applied on the container. When the middle rectangle changes, it repaints only itself plus the outsets of the drop-shadow filter. The initial problem was that when the filter was computed it only used the area of the repainted rect. Meaning that it might now know nothing about the other two rectangles, ending up redrawing the shadow without taking into account the other two boxes, but in some cases the shadow of the previous rectangle may be rendered under the second box.

Created attachment 131930 [details] Patch V3 Removed filter-repaint-transforms from this patch. It was not part of this change.

Created attachment 131931 [details] Screenshot 2 Added a new screenshot to show the example in my previous comment.

Comment on attachment 131930 [details] Patch V3 Attachment 131930 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11954379 New failing tests: css3/filters/filter-repaint.html css3/filters/effect-blur-hw.html css3/filters/effect-combined-hw.html css3/filters/filter-with-transform.html css3/filters/filter-repaint-sepia.html css3/filters/filter-repaint-shadow-rotated.html css3/filters/filter-repaint-composited-fallback-crash.html css3/filters/effect-drop-shadow-hw.html css3/filters/filter-empty-element-crash.html css3/filters/filter-repaint-shadow.html css3/filters/filter-repaint-blur.html css3/filters/filter-repaint-shadow-clipped.html css3/filters/nested-filter.html css3/filters/filter-repaint-composited-fallback.html

(In reply to comment #9) > (From update of attachment 131930 [details]) > Attachment 131930 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11954379 > > New failing tests: > css3/filters/filter-repaint.html > css3/filters/effect-blur-hw.html > css3/filters/effect-combined-hw.html > css3/filters/filter-with-transform.html > css3/filters/filter-repaint-sepia.html > css3/filters/filter-repaint-shadow-rotated.html > css3/filters/filter-repaint-composited-fallback-crash.html > css3/filters/effect-drop-shadow-hw.html > css3/filters/filter-empty-element-crash.html > css3/filters/filter-repaint-shadow.html > css3/filters/filter-repaint-blur.html > css3/filters/filter-repaint-shadow-clipped.html > css3/filters/nested-filter.html > css3/filters/filter-repaint-composited-fallback.html Most probably those tests need updated platform snapshots. I will look into it.

(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 131930 [details] [details]) > > Attachment 131930 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/11954379 > > > > New failing tests: > > css3/filters/filter-repaint.html > > css3/filters/effect-blur-hw.html > > css3/filters/effect-combined-hw.html > > css3/filters/filter-with-transform.html > > css3/filters/filter-repaint-sepia.html > > css3/filters/filter-repaint-shadow-rotated.html > > css3/filters/filter-repaint-composited-fallback-crash.html > > css3/filters/effect-drop-shadow-hw.html > > css3/filters/filter-empty-element-crash.html > > css3/filters/filter-repaint-shadow.html > > css3/filters/filter-repaint-blur.html > > css3/filters/filter-repaint-shadow-clipped.html > > css3/filters/nested-filter.html > > css3/filters/filter-repaint-composited-fallback.html > > Most probably those tests need updated platform snapshots. I will look into it. I've just built chromium and those fails are because of the following message: CONSOLE MESSAGE: Invalid name for preference: WebKitAcceleratedCompositingEnabled I'm using WebKitAcceleratedCompositingEnabled to make sure the tests are not hitting the accelerated path. Most probably it is now safe to remove that, because filters are not composited by default anymore.

Created attachment 132088 [details] Patch V4 Fixed Chromium bugs and also added https://bugs.webkit.org/show_bug.cgi?id=81239 to take care of composited layers size when Chromium will the outsets in platform code.

Comment on attachment 132088 [details] Patch V4 Attachment 132088 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11953931 New failing tests: css3/filters/filter-repaint.html css3/filters/filter-with-transform.html css3/filters/filter-repaint-composited-fallback-crash.html css3/filters/filter-repaint-sepia.html css3/filters/filter-repaint-shadow-rotated.html css3/filters/filter-empty-element-crash.html css3/filters/filter-repaint-shadow.html css3/filters/filter-repaint-child-layers.html css3/filters/filter-repaint-blur.html css3/filters/filter-repaint-shadow-clipped.html css3/filters/nested-filter.html css3/filters/filter-repaint-composited-fallback.html

Created attachment 132107 [details] Canvas + filter test case I've attached another problematic test case which this change may fix (run in webkit nightly, or chrome with --disable-accelerated-canvas to see the problem).

Comment on attachment 132088 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=132088&action=review > Source/WebCore/ChangeLog:14 > + The fix is to always keep a copy of the RenderLayer representation in memory. When repaint is needed we still invalidate > + only the parts that changed, but the filter is computed using the full source image and not only the dirty rectangle. I don't know enough about the render tree to know if this is a good approach or not, so I'll leave that for someone who does. > Source/WebCore/rendering/FilterEffectRenderer.cpp:32 > +// Use this flag to debug the output of the filters. It will output base64 encoded images > +// to "/tmp/filters_output.html". Make sure the rendering process has rights to write there. > +#define DEBUG_FILTER_RENDERING 0 This debugging code probably shouldn't be checked in. > Source/WebCore/rendering/FilterEffectRenderer.cpp:248 > + m_needsFullImageSource = true; There's a virtual on FilterOperation that I think gives this same information: FilterOperation::movesPixels(). It returns true for drop-shadow, blur and reference (custom may need to be added). Then you wouldn't need to encode the same knowledge here. > Source/WebCore/rendering/FilterEffectRenderer.cpp:394 > + // Some filters need the whole original area in order to recalculate correctly. > + // Such filters include blur, drop-shadow and shaders. For that reason, > + // we keep the whole image buffer in memory and repaint only dirty areas. > + bool needsFullImageSource = filter->needsFullImageSource(); Rather than requiring the full image, is there a way to determine the minimum required source rect, at least for the blur and drop shadow? (I can see how it would be problematic for custom filters, since you'd need to know what the shaders are going to do). If this dirty rect is also used for intermediate filters, and that intermediate filter is not pixel-moving, it seems like you'd be doing a lot of extra processing that you won't use (e.g., a sepia followed by a blur).

(In reply to comment #15) > (From update of attachment 132088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132088&action=review > > > Source/WebCore/ChangeLog:14 > > + The fix is to always keep a copy of the RenderLayer representation in memory. When repaint is needed we still invalidate > > + only the parts that changed, but the filter is computed using the full source image and not only the dirty rectangle. > > I don't know enough about the render tree to know if this is a good approach or not, so I'll leave that for someone who does. > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:32 > > +// Use this flag to debug the output of the filters. It will output base64 encoded images > > +// to "/tmp/filters_output.html". Make sure the rendering process has rights to write there. > > +#define DEBUG_FILTER_RENDERING 0 > > This debugging code probably shouldn't be checked in. Yeah, just that I had to rewrite it a couple of times and just refactored it to stay inside some ifdefs. I can delete if this is the policy. > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:248 > > + m_needsFullImageSource = true; > > There's a virtual on FilterOperation that I think gives this same information: FilterOperation::movesPixels(). It returns true for drop-shadow, blur and reference (custom may need to be added). Then you wouldn't need to encode the same knowledge here. Ok. Will use that. > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:394 > > + // Some filters need the whole original area in order to recalculate correctly. > > + // Such filters include blur, drop-shadow and shaders. For that reason, > > + // we keep the whole image buffer in memory and repaint only dirty areas. > > + bool needsFullImageSource = filter->needsFullImageSource(); > > Rather than requiring the full image, is there a way to determine the minimum required source rect, at least for the blur and drop shadow? (I can see how it would be problematic for custom filters, since you'd need to know what the shaders are going to do). If this dirty rect is also used for intermediate filters, and that intermediate filter is not pixel-moving, it seems like you'd be doing a lot of extra processing that you won't use (e.g., a sepia followed by a blur). Yes, I thought about that, but I would prefer to do it in a different step. I've added https://bugs.webkit.org/show_bug.cgi?id=81263 .

Comment on attachment 132088 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=132088&action=review I think it would be possible to split this into multiple patches. For example, the changes to computeRectForRepaint and adding calculateLayerBounds seem independent of the changes in the filter repainting code. Similarly beginFilterEffect and applyFilterEffect -> prepareFilterEffect is standalone. Anyway, it's done now - just worth considering for the future. I think one more patch with the change Stephen suggests and we're good to go. > LayoutTests/css3/filters/filter-repaint-composited-fallback-crash.html:4 > + NOTE: It is using the fact that Safari can draw drop-shadows in GPU only if the filter is the last one in the filter chain. I don't like the fact that this test requires Safari's drop-shadow behaviour. We'll want to fix that asap and it would be a pain to break unrelated tests. > LayoutTests/css3/filters/filter-repaint-composited-fallback.html:4 > + NOTE: It is using the fact that Safari can draw drop-shadows in GPU only if the filter is the last one in the filter chain. ditto >> Source/WebCore/rendering/FilterEffectRenderer.cpp:32 >> +#define DEBUG_FILTER_RENDERING 0 > > This debugging code probably shouldn't be checked in. It is useful though. I wonder if we could extract it into DRT the same way the layer/compositing tree is? > Source/WebCore/rendering/FilterEffectRenderer.cpp:354 > +#ifdef DEBUG_FILTER_RENDERING > +// In order to use this option, make sure this process has access to file API (ie. single process). > +static void logFiltersMessage(const String& message) > +{ Repeating comment about moving this into a layoutTestController property/function. Could we file a bug on that? Or maybe it is useful to content as well? I could imagine that people would find it handy if the Web Inspector showed each step of a filter chain (especially for SVG filters). >>> Source/WebCore/rendering/FilterEffectRenderer.cpp:394 >>> + bool needsFullImageSource = filter->needsFullImageSource(); >> >> Rather than requiring the full image, is there a way to determine the minimum required source rect, at least for the blur and drop shadow? (I can see how it would be problematic for custom filters, since you'd need to know what the shaders are going to do). If this dirty rect is also used for intermediate filters, and that intermediate filter is not pixel-moving, it seems like you'd be doing a lot of extra processing that you won't use (e.g., a sepia followed by a blur). > > Yes, I thought about that, but I would prefer to do it in a different step. I've added https://bugs.webkit.org/show_bug.cgi?id=81263 . Agree with Stephen. This might be a bigger project to implement a pull model for filter operations. > Source/WebCore/rendering/RenderLayer.cpp:953 > + for (const RenderLayer* curr = parent(); curr; curr = curr->parent()) { > + if (curr->requiresFullLayerImageForFilters()) > + return const_cast<RenderLayer*>(curr); > + } Link here to the bug about optimizing regions. > Source/WebCore/rendering/RenderLayer.cpp:986 > + m_filterRepaintRect.unite(rectForRepaint); What happens if I reduce the outsets on a filter? Do we ever shrink the repaint rect (obviously we need a full repaint at least once)? > Source/WebCore/rendering/RenderLayer.cpp:4058 > + LayoutUnit rw = frameView->contentsWidth(); > + LayoutUnit rh = frameView->contentsHeight(); > + > + boundingBoxRect.setWidth(max(boundingBoxRect.width(), rw - boundingBoxRect.x())); > + boundingBoxRect.setHeight(max(boundingBoxRect.height(), rh - boundingBoxRect.y())); Nit: name these vars contentsWidth or something. (I know it was copied from the original)

(In reply to comment #17) > (From update of attachment 132088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132088&action=review > > I think it would be possible to split this into multiple patches. For example, the changes to computeRectForRepaint and adding calculateLayerBounds seem independent of the changes in the filter repainting code. Similarly beginFilterEffect and applyFilterEffect -> prepareFilterEffect is standalone. Anyway, it's done now - just worth considering for the future. > > I think one more patch with the change Stephen suggests and we're good to go. > > > LayoutTests/css3/filters/filter-repaint-composited-fallback-crash.html:4 > > + NOTE: It is using the fact that Safari can draw drop-shadows in GPU only if the filter is the last one in the filter chain. > > I don't like the fact that this test requires Safari's drop-shadow behaviour. We'll want to fix that asap and it would be a pain to break unrelated tests. I will change the tests to use animations instead. It should to the same thing (change from GPU to CPU). > > > LayoutTests/css3/filters/filter-repaint-composited-fallback.html:4 > > + NOTE: It is using the fact that Safari can draw drop-shadows in GPU only if the filter is the last one in the filter chain. > > ditto > > >> Source/WebCore/rendering/FilterEffectRenderer.cpp:32 > >> +#define DEBUG_FILTER_RENDERING 0 > > > > This debugging code probably shouldn't be checked in. > > It is useful though. I wonder if we could extract it into DRT the same way the layer/compositing tree is? I added https://bugs.webkit.org/show_bug.cgi?id=82492 to track that and I'm going to delete the code from this patch. > > > Source/WebCore/rendering/FilterEffectRenderer.cpp:354 > > +#ifdef DEBUG_FILTER_RENDERING > > +// In order to use this option, make sure this process has access to file API (ie. single process). > > +static void logFiltersMessage(const String& message) > > +{ > > Repeating comment about moving this into a layoutTestController property/function. Could we file a bug on that? > > Or maybe it is useful to content as well? I could imagine that people would find it handy if the Web Inspector showed each step of a filter chain (especially for SVG filters). > > >>> Source/WebCore/rendering/FilterEffectRenderer.cpp:394 > >>> + bool needsFullImageSource = filter->needsFullImageSource(); > >> > >> Rather than requiring the full image, is there a way to determine the minimum required source rect, at least for the blur and drop shadow? (I can see how it would be problematic for custom filters, since you'd need to know what the shaders are going to do). If this dirty rect is also used for intermediate filters, and that intermediate filter is not pixel-moving, it seems like you'd be doing a lot of extra processing that you won't use (e.g., a sepia followed by a blur). > > > > Yes, I thought about that, but I would prefer to do it in a different step. I've added https://bugs.webkit.org/show_bug.cgi?id=81263 . > > Agree with Stephen. This might be a bigger project to implement a pull model for filter operations. > > > Source/WebCore/rendering/RenderLayer.cpp:953 > > + for (const RenderLayer* curr = parent(); curr; curr = curr->parent()) { > > + if (curr->requiresFullLayerImageForFilters()) > > + return const_cast<RenderLayer*>(curr); > > + } > > Link here to the bug about optimizing regions. > Done. Will post patch shortly. > > Source/WebCore/rendering/RenderLayer.cpp:986 > > + m_filterRepaintRect.unite(rectForRepaint); > > What happens if I reduce the outsets on a filter? Do we ever shrink the repaint rect (obviously we need a full repaint at least once)? > The next paint will shrink the repaint rect back to zero. We need full repaints when the backing buffer is resized. The full repaint is triggered by the filter->updateBackingStore that is called in FilterEffectRendererHelper::prepareFilterEffect. > > Source/WebCore/rendering/RenderLayer.cpp:4058 > > + LayoutUnit rw = frameView->contentsWidth(); > > + LayoutUnit rh = frameView->contentsHeight(); > > + > > + boundingBoxRect.setWidth(max(boundingBoxRect.width(), rw - boundingBoxRect.x())); > > + boundingBoxRect.setHeight(max(boundingBoxRect.height(), rh - boundingBoxRect.y())); > > Nit: name these vars contentsWidth or something. (I know it was copied from the original) Ok.

(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 132088 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=132088&action=review > > > > > > > LayoutTests/css3/filters/filter-repaint-composited-fallback-crash.html:4 > > > + NOTE: It is using the fact that Safari can draw drop-shadows in GPU only if the filter is the last one in the filter chain. > > > > I don't like the fact that this test requires Safari's drop-shadow behaviour. We'll want to fix that asap and it would be a pain to break unrelated tests. > I will change the tests to use animations instead. It should to the same thing (change from GPU to CPU). Actually, I take that back. The test case used composited layers, but the actual fiter rendering was done in software. The only cases when that happens is when using CSS Shaders (Custom Filters) or this case with drop-shadows on Safari. Both should be temporary, so I'm not sure how to proceed with this one. Any ideas? One idea would be to add a flag to window.internals that tells the page to always force fallback to software for drawing filters in composited layers. What do you think?

(In reply to comment #19) > > > I don't like the fact that this test requires Safari's drop-shadow behaviour. We'll want to fix that asap and it would be a pain to break unrelated tests. > > I will change the tests to use animations instead. It should to the same thing (change from GPU to CPU). > > Actually, I take that back. The test case used composited layers, but the actual fiter rendering was done in software. The only cases when that happens is when using CSS Shaders (Custom Filters) or this case with drop-shadows on Safari. Both should be temporary, so I'm not sure how to proceed with this one. Any ideas? > > One idea would be to add a flag to window.internals that tells the page to always force fallback to software for drawing filters in composited layers. What do you think? I think stick with the current approach for now, and we'll just have to deal with it when we fix drop shadow.

*** Bug 77622 has been marked as a duplicate of this bug. ***

Created attachment 134865 [details] Patch V5 Rebased the patch, removed debug code and added fixes for the other comments. Also added the chromium test expectations for the new tests. They need rebaselines.

*** Bug 77635 has been marked as a duplicate of this bug. ***

Comment on attachment 134865 [details] Patch V5 View in context: https://bugs.webkit.org/attachment.cgi?id=134865&action=review BTW - instead of "document.getElementsByClassName('before')[0]" you could just do "document.querySelector('.before')" as it always returns the first match. > LayoutTests/platform/chromium/test_expectations.txt:3210 > +BUGWK80323 SKIP : css3/filters/filter-repaint-blur.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint-child-layers.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint-composited-fallback-crash.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint-composited-fallback.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint-shadow-clipped.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint-shadow-rotated.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint-shadow.html = FAIL > +BUGWK80323 SKIP : css3/filters/filter-repaint.html = FAIL I'm not sure what the convention for chromium is. You say they need rebaselining, but you're linking to this bug not to a bug that should create the rebaseline? > Source/WebCore/ChangeLog:84 > + * rendering/RenderLayer.h: > + (RenderLayer): Maybe describe m_filterRepaintRect and how it gets accumulated and reset? > Source/WebCore/rendering/RenderLayer.cpp:955 > +{ > + if (includeSelf && requiresFullLayerImageForFilters()) > + return const_cast<RenderLayer*>(this); > + > + for (const RenderLayer* curr = parent(); curr; curr = curr->parent()) { > + if (curr->requiresFullLayerImageForFilters()) > + return const_cast<RenderLayer*>(curr); > + } I guess we could have combined this into a const RenderLayer* curr = includeSelf ? this : parent(); then for (; curr; curr = curr->parent()) right?

Alex, all good. Didn't mark cq as not sure if you want to change after feedback.

Comment on attachment 134865 [details] Patch V5 Attachment 134865 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12266522 New failing tests: css3/filters/filter-with-transform.html compositing/geometry/clip.html css3/filters/custom/effect-custom-parameters.html css3/filters/custom/custom-filter-shader-cache.html css3/filters/filter-repaint-sepia.html compositing/clip-child-by-non-stacking-ancestor.html compositing/overflow/clip-descendents.html compositing/images/clip-on-directly-composited-image.html css3/filters/custom/effect-custom.html css3/filters/nested-filter.html css3/filters/custom/effect-custom-combined-missing.html

Created attachment 134885 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Landed in http://trac.webkit.org/changeset/112745

Weird, somehow I missed the rest of the review email on this bug (maybe I un-cc'ed myself by mistake). Anyway, just as a followup: the accelerated drop-shadow implementation in Chrome has no limitation on where it can be placed in the filter chain.

This patch breaks the current Chrome build badly: a full layer is very expensive memory-wise, and with this, we end up allocating several, full-sized bitmaps for every update. I guess we need to roll out the change, and consider an implementation that correctly determines the rects that need to be updated.

(In reply to comment #30) > This patch breaks the current Chrome build badly: a full layer is very expensive memory-wise, and with this, we end up allocating several, full-sized bitmaps for every update. > > I guess we need to roll out the change, and consider an implementation that correctly determines the rects that need to be updated. As a workaround, you could try using -webkit-transform: translateZ(0) on the element. This will kick in the accelerated filter implementation, which is much faster. It will still use as much VRAM as the software path uses RAM, though (it's not any smarter about invalidation, it's just faster).

(In reply to comment #30) > This patch breaks the current Chrome build badly: a full layer is very expensive memory-wise, and with this, we end up allocating several, full-sized bitmaps for every update. > > I guess we need to roll out the change, and consider an implementation that correctly determines the rects that need to be updated. This is fixed in https://bugs.webkit.org/show_bug.cgi?id=83815 and https://bugs.webkit.org/show_bug.cgi?id=81263, so I think it's safe to close this bug again.

Bug 109783 is related.