Closed Bug 796847 Opened 9 years ago Closed 9 years ago

invalidation problems with SVG content inside a filter

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 + verified
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: heycam, Assigned: mattwoodrow)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 1 obsolete file)

Attached image 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.
This is a regression, it works in Firefox 15.
Keywords: regression
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
Forgot that User-Agent does not contain build date anymore.
Built from http://hg.mozilla.org/mozilla-central/rev/beee809b7ade
Confirmed on WinXP/x86 m-i changeset 80f9522602c5.
I can reproduce in linux-x64/basic and win7/d3d10.
Keywords: reproducible
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.
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.
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.
Is this still broken on trunk/FF18?

We definitely need to make sure this is fixed.
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.
Assignee: nobody → matt.woodrow
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.
Attachment #683796 - Flags: review?(jwatt)
OS: Mac OS X → All
Flags: in-testsuite?
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.
Attachment #683796 - Flags: review?(jwatt) → review+
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?
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
Attachment #683796 - Attachment is obsolete: true
Attachment #685907 - Flags: review+
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 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/)
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.
(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)
(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
Flags: in-testsuite? → in-testsuite+
Please nominate for uplift when confident in the changes on m-c. Thanks!
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
Attachment #685907 - Flags: approval-mozilla-beta?
Attachment #685907 - Flags: approval-mozilla-aurora?
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!
Attachment #685907 - Flags: approval-mozilla-beta?
Attachment #685907 - Flags: approval-mozilla-beta+
Attachment #685907 - Flags: approval-mozilla-aurora?
Attachment #685907 - Flags: approval-mozilla-aurora+
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
QA Contact: manuela.muntean
You need to log in before you can comment on or make changes to this bug.