Bug 94022 – [CSS Filters] RenderLayerCompositor::addToOverlapMap should take into account the filters outsets (ie. blur and drop-shadow)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

There are cases when an element gets a GraphicsLayer, but when the overlap map is computed, the shadow is not taken into account. Because of that, elements that need to display on top of the GraphicsLayer are painted behind the shadow.

[Alexandru Chiculita]

This goes in both ways:

1. The shadow element has the GPU layer but it needs to display in the back.
2. The shadow element has no GPU layer, but the shadow is overlapping with another GPU layer.

[Max Vujovic]

Created attachment 172165 [details]
Patch

A first attempt at fixing the issue, not for formal review. I'm new to this code, so I'll be looking into it more and discussing it with Alex.

[Max Vujovic]

Created attachment 175018 [details]
Patch

After a chat with Alex, factored the patch a little differently. Also, added tests and ChangeLog entries.

[Simon Fraser (smfr)]

Comment on attachment 175018 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.h:572
> +#if ENABLE(CSS_FILTERS)
> +    // Bounding box including filter outsets in the coordinates of this layer.
> +    LayoutRect localFilterBox() const { return expandRectForFilterOutsets(localBoundingBox()); };
> +#else
> +    LayoutRect localFilterBox() const { return localBoundingBox(); }
> +#endif

Why is this different to, say, box-shadow? That's already accounted for in localBoundingBox, so I'm not sure why filters are not.

[Max Vujovic]

(In reply to comment #4)
> (From update of attachment 175018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175018&action=review
> 
> > Source/WebCore/rendering/RenderLayer.h:572
> > +#if ENABLE(CSS_FILTERS)
> > +    // Bounding box including filter outsets in the coordinates of this layer.
> > +    LayoutRect localFilterBox() const { return expandRectForFilterOutsets(localBoundingBox()); };
> > +#else
> > +    LayoutRect localFilterBox() const { return localBoundingBox(); }
> > +#endif
> 
> Why is this different to, say, box-shadow? That's already accounted for in localBoundingBox, so I'm not sure why filters are not.

Thanks for pointing that out. Based on the box-shadow code in RenderBox::addVisualEffectOverflow, I'm thinking filter outsets should be accounted for similarly and end up in localBoundingBox. I'll work on that. 

If I understand correctly, box-shadow operates on the border-box and filters operate on the entire visual overflow box.

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 175018 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175018&action=review
> > 
> > > Source/WebCore/rendering/RenderLayer.h:572
> > > +#if ENABLE(CSS_FILTERS)
> > > +    // Bounding box including filter outsets in the coordinates of this layer.
> > > +    LayoutRect localFilterBox() const { return expandRectForFilterOutsets(localBoundingBox()); };
> > > +#else
> > > +    LayoutRect localFilterBox() const { return localBoundingBox(); }
> > > +#endif
> > 
> > Why is this different to, say, box-shadow? That's already accounted for in localBoundingBox, so I'm not sure why filters are not.
> 
> Thanks for pointing that out. Based on the box-shadow code in RenderBox::addVisualEffectOverflow, I'm thinking filter outsets should be accounted for similarly and end up in localBoundingBox. I'll work on that. 
> 
> If I understand correctly, box-shadow operates on the border-box and filters operate on the entire visual overflow box.

True, that is a difference. It might be better to look at some of the outline invalidation code.

[Max Vujovic]

Created attachment 181593 [details]
Patch

[Max Vujovic]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 175018 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=175018&action=review
> > > 
> > > > Source/WebCore/rendering/RenderLayer.h:572
> > > > +#if ENABLE(CSS_FILTERS)
> > > > +    // Bounding box including filter outsets in the coordinates of this layer.
> > > > +    LayoutRect localFilterBox() const { return expandRectForFilterOutsets(localBoundingBox()); };
> > > > +#else
> > > > +    LayoutRect localFilterBox() const { return localBoundingBox(); }
> > > > +#endif
> > > 
> > > Why is this different to, say, box-shadow? That's already accounted for in localBoundingBox, so I'm not sure why filters are not.
> > 
> > Thanks for pointing that out. Based on the box-shadow code in RenderBox::addVisualEffectOverflow, I'm thinking filter outsets should be accounted for similarly and end up in localBoundingBox. I'll work on that. 
> > 
> > If I understand correctly, box-shadow operates on the border-box and filters operate on the entire visual overflow box.
> 
> True, that is a difference. It might be better to look at some of the outline invalidation code.

I think the outline invalidation approach would lead to larger textures when we don't necessarily need them. From what I understand, the outline code takes the maximal outline size and expands all the layers' bounds by that amount.

I also realized that the both the outline approach and my previous patch would fail in certain cases. For example, if we have nested drop shadows like the following:

- A parent element with a drop shadow.
- A child element with a drop shadow, extending out of the parent element.

The parent element's drop shadow will extend off of the child element's drop shadow, and the filter outsets will be farther out than we expected. Thus, we can't just add the filter outsets to a layer as in my previous patch.

I've uploaded another patch that fixes this case. In the patch, there's a test for it called "sw-nested-shadow-overlaps-hw-nested-shadow.html".

[Simon Fraser (smfr)]

(In reply to comment #8)
> I think the outline invalidation approach would lead to larger textures when we don't necessarily need them. From what I understand, the outline code takes the maximal outline size and expands all the layers' bounds by that amount.

...which is terrible and we should fix.

[EFL EWS Bot]

Comment on attachment 181593 [details]
Patch

Attachment 181593 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15741876

[WebKit Review Bot]

Comment on attachment 181593 [details]
Patch

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

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
platform/chromium/virtual/softwarecompositing/filters/sw-layer-overlaps-hw-shadow.html
platform/chromium/virtual/softwarecompositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-layer-overlaps-hw-shadow.html

[Max Vujovic]

(In reply to comment #9)
> (In reply to comment #8)
> > I think the outline invalidation approach would lead to larger textures when we don't necessarily need them. From what I understand, the outline code takes the maximal outline size and expands all the layers' bounds by that amount.
> 
> ...which is terrible and we should fix.

Yes, we should. I've filed bug 106397 at least.

Do you think the approach in this latest patch is ok? (Ignoring the bot failures for now.)

[Max Vujovic]

Thanks! I'll clean up the failures.

[Max Vujovic]

Created attachment 182025 [details]
Patch

Re-running the bots.

[Max Vujovic]

Comment on attachment 182025 [details]
Patch

Bots look good. Setting cq+. I had to add an #if ENABLE(CSS_FILTERS) guard and some Chromium expectations. Chromium outputs (drawsContent 1) on some layers when Safari does not.

[WebKit Review Bot]

Comment on attachment 182025 [details]
Patch

Rejecting attachment 182025 [details] from commit-queue.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15766896

[Max Vujovic]

Created attachment 182157 [details]
Patch

Add reviewer entry to ChangeLogs.

[WebKit Review Bot]

Comment on attachment 182157 [details]
Patch

Rejecting attachment 182157 [details] from commit-queue.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/webkit-commit-queue/Source/WebKit/chromium/win8 --revision 175535 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
55>At revision 175535.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15761979

[WebKit Review Bot]

Comment on attachment 182157 [details]
Patch

Clearing flags on attachment: 182157

Committed r139330: <http://trac.webkit.org/changeset/139330>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.