Bug 81263 – [CSS Filters] Drop-shadow and blur can avoid using full source image

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Calculate the exact rectangle that's needed to paint the blur or the drop-shadow and use just that to compute the filters.

[Alexandru Chiculita]

Created attachment 137638 [details]
Patch V1

[Dean Jackson]

Comment on attachment 137638 [details]
Patch V1

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

Just one small question on naming, and some typos.

> Source/WebCore/ChangeLog:10
> +        Even if the element is completely offscreen, but it's shadow is in the viewport, we can still compute the source 

typo it's -> its

> Source/WebCore/ChangeLog:14
> +        No new tests, this change doesn't have visible results and the functionality should
> +        already be covered by previous filter tests.

We couldn't have a repaint test specifically for this?  I'm not worried if not.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:370
> +        // Note that the outsets are reveresed here because we are going backwards -> we have the dirty rect and

Typo - reveresed -> reversed.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:379
> +bool FilterEffectRendererHelper::prepareFilterEffect(RenderLayer* renderLayer, const LayoutRect& filterBoxRect, const LayoutRect& dirtyRect, const LayoutRect& layerRepaintRect)

Unless I'm missing something, but this method no longer calls prepare(), so maybe it should be renamed?

[Alexandru Chiculita]

(In reply to comment #2)
Thanks for the review.

> Unless I'm missing something, but this method no longer calls prepare(), so maybe it should be renamed?

It's not calling prepare anymore, but it's preparing the "repaint" rectangle and updating the backing store size.

I actually think that we should rename the other functions:

FilterEffectRenderer::updateBackingStore -> updateBackingStoreRect
FilterEffectRenderer:: prepare -> allocateBackingStoreIfNeeded

FilterEffectRenderer:: prepare should not be required to call clearIntermediateResults() anymore, because we do that when we finish in FilterEffectRendererHelper::applyFilterEffect, so next time it should already be cleaned up.

What do you think?

[Dean Jackson]

(In reply to comment #3)
> (In reply to comment #2)
> Thanks for the review.
> 
> > Unless I'm missing something, but this method no longer calls prepare(), so maybe it should be renamed?
> 
> It's not calling prepare anymore, but it's preparing the "repaint" rectangle and updating the backing store size.
> 
> I actually think that we should rename the other functions:
> 
> FilterEffectRenderer::updateBackingStore -> updateBackingStoreRect
> FilterEffectRenderer:: prepare -> allocateBackingStoreIfNeeded
> 
> FilterEffectRenderer:: prepare should not be required to call clearIntermediateResults() anymore, because we do that when we finish in FilterEffectRendererHelper::applyFilterEffect, so next time it should already be cleaned up.
> 
> What do you think?

Sounds good.

[Alexandru Chiculita]

(In reply to comment #2)
> (From update of attachment 137638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137638&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        No new tests, this change doesn't have visible results and the functionality should
> > +        already be covered by previous filter tests.
> 
> We couldn't have a repaint test specifically for this?  I'm not worried if not.

There's already a test that makes sure that memory is not exploding for offscreen buffers. Also, blur repaints are already covered. I'm not sure what else could we do for it. I think we need something in DRT to dump the intermediate filter bitmaps. I will add a bug for that

http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/offscreen-filters-memory-usage.html

[Alexandru Chiculita]

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

[Rafael Brandao]

(In reply to comment #6)
> Landed in http://trac.webkit.org/changeset/114529 .

Build fix for Qt was landed in http://trac.webkit.org/changeset/114534

[Alexandru Chiculita]

Created attachment 137736 [details]
QT Fix

I've already committed this, unreviewed. QT build is also using FilterEffectRenderer::prepare and I've updated it to the new name.