Bug 81263 - [CSS Filters] Drop-shadow and blur can avoid using full source image
Summary: [CSS Filters] Drop-shadow and blur can avoid using full source image
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-15 13:49 PDT by Alexandru Chiculita
Modified: 2012-04-18 11:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch V1 (12.64 KB, patch)
2012-04-17 16:56 PDT, Alexandru Chiculita
dino: review+
dino: commit-queue-
Details | Formatted Diff | Diff
QT Fix (1.49 KB, patch)
2012-04-18 11:56 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2012-03-15 13:49:06 PDT
Calculate the exact rectangle that's needed to paint the blur or the drop-shadow and use just that to compute the filters.
Comment 1 Alexandru Chiculita 2012-04-17 16:56:25 PDT
Created attachment 137638 [details]
Patch V1
Comment 2 Dean Jackson 2012-04-17 17:09:19 PDT
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?
Comment 3 Alexandru Chiculita 2012-04-17 17:18:27 PDT
(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?
Comment 4 Dean Jackson 2012-04-17 18:31:56 PDT
(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.
Comment 5 Alexandru Chiculita 2012-04-18 10:30:44 PDT
(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
Comment 6 Alexandru Chiculita 2012-04-18 10:38:18 PDT
Landed in http://trac.webkit.org/changeset/114529 .
Comment 7 Rafael Brandao 2012-04-18 11:42:09 PDT
(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
Comment 8 Alexandru Chiculita 2012-04-18 11:56:53 PDT
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.