796847 - invalidation problems with SVG content inside a filter

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: test

In the attached test, there is a <rect> that is the child of a <g> that has a filter applied to it.  When the <rect> is modified (a few seconds after loading), it looks like only the top half of it is invalidated.

Comment 1

[Robert Longson [:longsonr]]

This is a regression, it works in Firefox 15.

Comment 2

[Stefan Fleiter (:sfleiter)]

For me the rect only changes color on tab switch or moving another window over the rect.
Does no matter whether HWA is enabled or not.

Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/18.0 Firefox/18.0

Comment 3

[Stefan Fleiter (:sfleiter)]

Forgot that User-Agent does not contain build date anymore.
Built from http://hg.mozilla.org/mozilla-central/rev/beee809b7ade

Comment 4

[Toshihiro Yamada]

Confirmed on WinXP/x86 m-i changeset 80f9522602c5.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I can reproduce in linux-x64/basic and win7/d3d10.

Comment 6

[Jonathan Watt [:jwatt]]

We invalidate the correct area, minus a strip from the bottom of the invalidation rect that is equal to the height of the UI that's above the content area. When moving the rect to a different location, it's always the same height of strip that is removed from the bottom of the invalidation rect.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: testcase I've been using for debugging

Comment 8

[Jonathan Watt [:jwatt]]

I've been tracking the issue back starting from the following stack:

  nsDisplaySVGEffects::ComputeVisibility
  nsDisplayItem::RecomputeVisibility
  mozilla::FrameLayerBuilder::DrawThebesLayer
  mozilla::layers::ThebesLayerOGL::RenderLayer
  mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerOGL>
  mozilla::layers::ContainerLayerOGL::RenderLayer
  mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerOGL>
  mozilla::layers::ContainerLayerOGL::RenderLayer
  mozilla::layers::LayerManagerOGL::Render
  mozilla::layers::LayerManagerOGL::EndTransaction
  nsDisplayList::PaintForFrame

In nsDisplaySVGEffects::ComputeVisibility we end up with a dirtyRect of {0,3900,6000,2640} (the height should be 6000, not 2640). This is because mVisibleRect has bounds of {0,3900,6000,2100}.

This in turn is because nsDisplayItem::RecomputeVisibility is passed an aVisibleRegion with bounds {0,3900,6000,2100}.

This happens because mozilla::FrameLayerBuilder::DrawThebesLayer is passed an aRegionToDraw with bounds {0,0,100,35}.

This comes from the mRegionToDraw member of the |state| object in ThebesLayerOGL::RenderLayer, which comes from the mBuffer->BeginPaint() call that function.

Tracking back past this point is proving much slower work since I'm not familiar enough with the layers code to speculate about where to jump to, and gdb/llvm are just hanging for me all the time trying to trace things back past this point manually.

Comment 9

[Matt Woodrow (:mattwoodrow)]

The value passed to DrawThebesLayer is most likely the result of invalidation.

Enable the debugging code described here: https://wiki.mozilla.org/Gecko:DisplayListBasedInvalidation#Debugging_Invalidations_Problems

To see what's being invalidated, and why.

It sounds like something is being offset incorrectly.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this still broken on trunk/FF18?

We definitely need to make sure this is fixed.

Comment 11

[Alex Keybl [:akeybl]]

Sending this DLBI-related regression over to Matt. If this is on the SVG side of things, let's move components and reassign to jwatt for investigation.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space

AdjustInvalidAreaForSVGEffects was expecting a pixel rect relative to the frames coordinate space, and we weren't giving it that.

The rect being passed in was 0,65,100,100 and we were intersecting that with 0,0,100,100 resulting in 0,65,100,35 which is the invalid area you see on screen.

There may be other ways to do this, I'm open to suggestions.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 683796 [details] [diff] [review]
Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space

(stealing review, at mattwoodrow's suggestion, in the interests of getting this regression fixed quickly)

>diff --git a/layout/base/FrameLayerBuilder.cpp b/layout/base/FrameLayerBuilder.cpp
>       if (aLayerState == LAYER_SVG_EFFECTS) {
>-        invalid = nsSVGIntegrationUtils::AdjustInvalidAreaForSVGEffects(aItem->GetUnderlyingFrame(), invalid.GetBounds());
>+        invalid = nsSVGIntegrationUtils::AdjustInvalidAreaForSVGEffects(aItem->GetUnderlyingFrame(), 

Remove trailing whitespace @ end of that line^

>+++ b/layout/svg/nsSVGIntegrationUtils.h
>+   * @param aFrame The effects frame.
>+   * @param aToReferenceFrame The offset (in app units) from aFrame to it's reference display item.

s/it's/its/

Also, I think the language here is a bit backwards. Both the naming (aToReferenceFrame) and the description (offset...from aFrame to its reference) makes me think that I'd add this point to aFrame to get to its reference frame -- but really, it's the reverse.

Maybe name this "aOffsetFromReferenceFrame", and change the documentation to "aFrame's offset (in app units) from its reference display item"  (or maybe "with respect to")

>+   * @param aInvalidRect The pre-effects invalid rect in pixels relative to the reference display item.
>+   * @return The post-effects invalid rects in pxiels relative to the reference display item.

s/rects/rect/
s/pxiels/pixels/

Also, these documentation lines are pretty long -- could you wrap them to 80 chars?

>    */
>   static nsIntRect
>-  AdjustInvalidAreaForSVGEffects(nsIFrame* aFrame, const nsIntRect& aInvalidRect);
>+  AdjustInvalidAreaForSVGEffects(nsIFrame* aFrame, const nsPoint& aToReferenceFrame, 

Remove whitespace at end of this^ line

>diff --git a/layout/xul/base/src/nsSliderFrame.h b/layout/xul/base/src/nsSliderFrame.h

(as you noted in IRC, this chunk is unrelated & doesn't go in this patch)

And I think this needs a reftest.

r=me with that.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 683796 [details] [diff] [review]
Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space

Also:

>@@ -300,23 +301,24 @@ nsSVGIntegrationUtils::AdjustInvalidArea
>     // The frame is either not there or not currently available,
>     // perhaps because we're in the middle of tearing stuff down.
>     // Be conservative.
>-    nsRect overflow = aFrame->GetVisualOverflowRect();
>+    nsRect overflow = aFrame->GetVisualOverflowRect() + aToReferenceFrame;
>     return overflow.ToOutsidePixels(appUnitsPerDevPixel);

Perhaps extend the comment there to say something like:
      // Be conservative -- return our full visual overflow rect
      // (with respect to our reference frame's origin).
to be clear about the various rects / transformations that are involved there.

>   // Convert aInvalidRect into "user space" in app units:
>   nsPoint toUserSpace =
>     aFrame->GetOffsetTo(firstFrame) + GetOffsetToUserSpace(firstFrame);
>+  toUserSpace -= aToReferenceFrame;
>   nsRect preEffectsRect = aInvalidRect.ToAppUnits(appUnitsPerDevPixel) + toUserSpace;
> 
>   // Return ther result, relative to aFrame, not in user space:
>   nsRect result = filterFrame->GetPostFilterDirtyArea(firstFrame, preEffectsRect) -
>            toUserSpace;

This is a little bit magic -- I don't immediately see why that particular place is where we want to be subtracting "aToReferenceFrame" (instead of, say, adding aToReferenceFrame when we generate |result| at the end, to parallel what we do for |overflow| up above).

Could you add a comment to explain that?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space v2

Fixed review comments, carrying forward r=dholbert.

Note that I left the aToReferenceFrame parts unchanged, since it matches the terminology used for the rest of the display list code. I agree that it's not entirely obvious, but consistency is more important.

Try:  https://tbpl.mozilla.org/?tree=Try&rev=00c8330c3f0b

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 685907 [details] [diff] [review]
Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space v2

>+++ b/layout/reftests/bugs/reftest.list
>@@ -1720,13 +1720,14 @@ fuzzy-if(true,17,5859) == 759036-2.html 
> == 776265-1b.html 776265-1-ref.html
> == 776265-1c.html 776265-1-ref.html
> == 776265-1d.html 776265-1-ref.html
> == 776265-2a.html 776265-2-ref.html
> == 776265-2b.html 776265-2-ref.html
> == 776265-2c.html 776265-2-ref.html
> == 776265-2d.html 776265-2-ref.html
> == 787947-1.html 787947-1-ref.html
>+== 796847.svg 796847-ref.svg

Could you add a "-1" suffix to the testcase & reference case, to match local style? (& so we can add a -2, -3, etc. variant if we decide to later on)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 685907 [details] [diff] [review]
Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space v2

A few other nits on the reftest:

>+++ b/layout/reftests/bugs/796847-ref.svg
>@@ -0,0 +1,8 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+	<filter id="f" x="0" y="0" width="100%" height="100%">
>+		<feGaussianBlur in="SourceGraphic" stdDeviation="0.1"/>
>+	</filter>
>+  <g filter="url(#f)">

s/tabs/spaces/ in the <filter>...</filter> chunk there

>+++ b/layout/reftests/bugs/796847.svg
>+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" class="reftest-wait">
>+  <!-- From https://bugzilla.mozilla.org/show_bug.cgi?id=463939 -->

Looks like a stale bug number, copypasted from another test probably (?) -- remove or update that.

>+  <title>Test that the area that's covered by a filtered elemnt is invalidated when it moves</title>

This <title> is wrong, too. We don't actually move the element at all, do we?  (Also, s/elemnt/element/)

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fda5071806a

Comment 19

[Matt Woodrow (:mattwoodrow)]

And the typo that I forgot: https://hg.mozilla.org/integration/mozilla-inbound/rev/8708a2a22767

Comment 20

[Daniel Holbert [:dholbert]]

Looks like you forgot to update reftest.list for the "-1" renaming, so this'll be orange, I think. Should be able to fix it w/ a quick followup.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> Looks like a stale bug number, copypasted from another test probably (?)
[...]
> (Also, s/elemnt/element/)

(incidentally, I fixed the "elemnt" typo in the original test, filter-invalidation-01.svg, in bug 815928)

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Looks like you forgot to update reftest.list for the "-1" renaming, so
> this'll be orange, I think. Should be able to fix it w/ a quick followup.

I pushed a fix for that:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fc57427dec00

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8fda5071806a
https://hg.mozilla.org/mozilla-central/rev/8708a2a22767
https://hg.mozilla.org/mozilla-central/rev/fc57427dec00

Comment 24

[Alex Keybl [:akeybl]]

Please nominate for uplift when confident in the changes on m-c. Thanks!

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 685907 [details] [diff] [review]
Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI
User impact if declined: Incorrect invalidation on some svg files.
Testing completed (on m-c, etc.): Been on m-c for a while
Risk to taking this patch (and alternatives if risky): Very low risk.
String or UUID changes made by this patch: None

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 685907 [details] [diff] [review]
Make AdjustInvalidAreaForSVGEffects take a rect in the right coordinate space v2

Low risk fix for a new DLBI-related SVG issue. Landing today will ensure that it makes it into our third beta. Thanks!

Comment 27

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/041c56c24014
https://hg.mozilla.org/releases/mozilla-beta/rev/ccfe74078eb9

Comment 28

[Manuela Muntean [Away]]

Verified fixed on Firefox 18 beta 4.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121212073002