nsChangeHint_RepaintFrame should only invalidate the bounds of SVG frames, not update them

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

({perf})

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

It seems that we are currently calling nsSVGUtils::UpdateGraphic on SVG frames that are being restyled with the nsChangeHint_RepaintFrame hint. nsChangeHint_RepaintFrame is supposed to indicate a simple repaint though, and should not require the expensive UpdateCoveredRegion call in nsSVGUtils::UpdateGraphic.
Created attachment 604156 [details] [diff] [review]
patch

This patch changes the nsSVGUtils::UpdateGraphic call to an nsSVGUtils::InvalidateCoveredRegion call. The only issue with this is that when markers change, we don't currently have any code to update the covered regions of frames that reference the markers (markers are the one SVG effect that are included in covered regions). (We have basically been depending on the broken use of nsSVGUtils::UpdateGraphic to do this.) I've fixed that in the patch too.
Attachment #604156 - Flags: review?(dholbert)
UpdateGraphic calls :InvalidateRenderingObservers(aFrame), too.  That seems like something we might still need to do, but maybe we don't?  (or maybe it'll still get called somehow?  It doesn't look like InvalidateCoveredRegion directly calls it.)
(Or maybe the sorts of changes that would require rendering-observer-invalidation will send additional notifications which will make that invalidation happen elsewhere?)
Created attachment 604228 [details] [diff] [review]
patch

Oops, I remembered InvalidateRenderingObservers in bug 734079, but I forgot it here. :/ Yeah, we need that, and in fact we need to invalidate the old area too, not just the new. So might as well use nsSVGUtils::UpdateGraphic (but only when we end up in nsSVGEffects::UpdateEffects while handling the nsChangeHint_UpdateEffects hint).

Pushed this to Try to make sure we pass everything:

https://tbpl.mozilla.org/?tree=Try&rev=824862ef0e45
Attachment #604156 - Attachment is obsolete: true
Attachment #604228 - Flags: review?(dholbert)
Attachment #604156 - Flags: review?(dholbert)
Removed the include directive locally.
Created attachment 604251 [details] [diff] [review]
patch

Actually this probably feels better. (The nsChangeHint_RepaintFrame and nsChangeHint_UpdateEffects hints are passed together when an effect changes.)
Attachment #604228 - Attachment is obsolete: true
Attachment #604251 - Flags: review?(dholbert)
Attachment #604228 - Flags: review?(dholbert)
https://tbpl.mozilla.org/?tree=Try&rev=e6c90bebeb32
Comment on attachment 604251 [details] [diff] [review]
patch

Seems reasonable. r=me
Attachment #604251 - Flags: review?(dholbert) → review+
If you invalidate rendering observers you'll get an infinite loop won't you? We update shape with a gradient or pattern, resulting in us doing an update effects change hint which then when if it calls InvalidateRenderingObservers will result in another update. This is async so you won't run out of stack.
The pre-patch code was already doing the rendering observer invalidation since it called nsSVGUtils::UpdateGraphic. The patch just makes us do less work in the case that the nsChangeHint_UpdateEffects hint is not passsed.

The details of how SVG effects change notifications and invalidations work are not entirely clear to me just yet, so I'm not sure if there is indeed a (pre-existing) issue.
dynamic-filter-contents-01b.svg is the sort of thing I was worried about but that seems to work OK. Looks like we remove the rendering observer on the element itself before we update then it gets added back on paint thus avoiding looping.
https://hg.mozilla.org/mozilla-central/rev/9d97a3d7788a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.