Closed Bug 1217266 Opened 4 years ago Closed 3 years ago

Filters in resource documents don't animate

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox44 --- affected
firefox49 --- fixed

People

(Reporter: birtles, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Attached image Resource document (obsolete) —
I seem to recall this being a deliberate decision at some point but I can't find that reference now so I'm filing this until I can.

In the upcoming test case a filter that is referenced locally animates, but the same content in a resource document does not animate.
Attached file Test case (obsolete) —
Attached image Resource document
Updating to make the animation infinitely repeat.
Attachment #8677217 - Attachment is obsolete: true
Attached file Test case
(Updating the test case too)
Attachment #8677218 - Attachment is obsolete: true
Marking this as a regression since I recall it working many years ago.
Adding Robert and Daniel since one of them may remember what the latest is with all this.
Keywords: regression
Yup -- locally, this works in Firefox 17, but is broken in Firefox 28 (and in Nightly).   (I don't have any builds between 17 and 28 on my laptop).  So, it is indeed a regression.

--> adding regressionwindow-wanted to identify what broke this.
SMIL and CSS animation in images has been broken in various ways for a while c.f. bug 908634, bug 1190881, bug 1008598, bug 1121478.
Blocks: 854765
Thanks Alice!
The "Remote" item is updated not only when changing tabs but also clicking it quickly can produce the same effect.
Attached patch ugly.txt (obsolete) — Splinter Review
This fixes it, but there must be a better way. Any ideas Jonathan?
Attachment #8678446 - Flags: feedback?(jwatt)
The style change happens on the feFlood element (that is invalidated by the style change) but the filter element, which is what the main document is listening for changes on is never invalidated.
In both the local and remote resource case the nsSVGRenderingObserverList is created under:

  nsSVGRenderingObserverList::nsSVGRenderingObserverList
  nsSVGEffects::AddRenderingObserver
  nsSVGRenderingObserver::GetReferencedElement
  nsSVGRenderingObserver::GetReferencedFrame
  nsSVGRenderingObserver::GetReferencedFrame
  nsSVGFilterReference::GetFilterFrame
  nsSVGFilterReference::ReferencesValidResource
  nsSVGFilterChainObserver::ReferencesValidResources
  nsSVGEffects::EffectProperties::HasValidFilter
  nsSVGIntegrationUtils::ComputePostEffectsVisualOverflowRect
  ComputeEffectsRect
  nsIFrame::FinishAndStoreOverflow
  nsBlockFrame::UpdateOverflow      <-- for <h1>

In the local case, we invalidate the SVGFilterElement under:

  nsSVGEffects::InvalidateDirectRenderingObservers
  nsSVGEffects::InvalidateDirectRenderingObservers
  InvalidateFrameInternal
  nsIFrame::InvalidateFrame
  nsIFrame::InvalidateFrameSubtree
  mozilla::DoApplyRenderingChangeToTree

when InvalidateDirectRenderingObservers(parent) is called here:

  static void InvalidateFrameInternal(nsIFrame *aFrame, bool aHasDisplayItem = true)
  {
    if (aHasDisplayItem) {
      aFrame->AddStateBits(NS_FRAME_NEEDS_PAINT);
    }
    nsSVGEffects::InvalidateDirectRenderingObservers(aFrame);
    bool needsSchedulePaint = false;
    if (nsLayoutUtils::IsPopup(aFrame)) {
      needsSchedulePaint = true;
    } else {
      nsIFrame *parent = nsLayoutUtils::GetCrossDocParentFrame(aFrame);
      while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)) {
        if (aHasDisplayItem) {
          parent->AddStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
        }
>       nsSVGEffects::InvalidateDirectRenderingObservers(parent);

Things go wrong in the remote resource case because the |!parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)| check returns false (that bit is set) so we don't enter this |while| loop and call InvalidateDirectRenderingObservers(parent) to invalidate the nsSVGFilterFrame's rendering observers.

Back in the local case, the reason that the !HasAnyStateBits check returns true is because we clear that bit from the nsSVGFilterFrame under:

  nsIFrame::RemoveStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsIFrame::ClearInvalidationStateBits
  nsDisplayList::PaintRoot
  nsLayoutUtils::PaintFrame

In the remote resource case we never paint the document directly, so nsDisplayList::PaintRoot is never called on the document in that case, and therefore once the NS_FRAME_DESCENDANT_NEEDS_PAINT bit is set on the nsSVGFilterFrame we never clear it again.
It seems like we shouldn't ever make use of the NS_FRAME_ALL_DESCENDANTS_NEED_PAINT bit on filters and their contents, but hacking the (SVG agnostic) code that deals with this bit to have specialist handling for some SVG cases is undesirable too. Perhaps we need a new NS_STATE_SVG_RESOURCE state bit.
Comment on attachment 8678446 [details] [diff] [review]
ugly.txt

As you note yourself, this is an ugly hack, and hopefully the analysis I've written above can move us towards a better solution.
Attachment #8678446 - Flags: feedback?(jwatt) → feedback-
I can help do this investigation.
(In reply to Jonathan Watt [:jwatt] from comment #13)
> It seems like we shouldn't ever make use of the
> NS_FRAME_ALL_DESCENDANTS_NEED_PAINT bit on filters and their contents, but
> hacking the (SVG agnostic) code that deals with this bit to have specialist
> handling for some SVG cases is undesirable too. Perhaps we need a new
> NS_STATE_SVG_RESOURCE state bit.

All such frames are NS_FRAME_IS_NONDISPLAY which is a global (i.e. non SVG specific bit) we could simply refuse to set NS_FRAME_ALL_DESCENDANTS_NEED_PAINT if NS_FRAME_IS_NONDISPLAY or alternatively ignore NS_FRAME_ALL_DESCENDANTS_NEED_PAINT if NS_FRAME_IS_NONDISPLAY is set (or both).
Assignee: nobody → dmu
Compare the ClearInvalidationStateBits call stack between the local and remote SVG. The stack are the same between them.

svgFEleafFrame::ClearInvalidationStateBits();
nsSVGFilterFrame ->
nsSVGOuterSVGAnonChildFrame ->
nsSVGOuterSVGFrame ->
nsBlockFrame ->
nsBlockFrame ->
nsCanvasFrame::ClearInvalidationStateBits ->
nsHTMLScrollFrame ->
viewportFrame ->
nsSubDocumentFrame ->
nsStackFrame ->
nsBoxFrame ->
nsBoxFrame ->
nsBoxFrame ->
nsDeckFrame ->
nsBoxFrame ->
nsBoxFrame ->
nsBoxFrame ->
nsBoxFrame ->
nsDeckFrame ->
nsBoxFrame ->
nsDeckFrame ->
nsDocElementBoxFrame ->
nsRootBoxFrame ->
viewportFrame ->

I think the main problem is like Jonathan mentioned why it can't pass

> while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)) 
{
 nsSVGEffects::InvalidateDirectRenderingObservers(parent);
}

It might be at somewhere, it is set back NS_FRAME_DESCENDANT_NEEDS_PAINT to this SVG leaf again.
(In reply to Daosheng Mu[:daoshengmu] from comment #17)
> Compare the ClearInvalidationStateBits call stack between the local and
> remote SVG. The stack are the same between them.
> 
> svgFEleafFrame::ClearInvalidationStateBits();
> nsSVGFilterFrame ->
> nsSVGOuterSVGAnonChildFrame ->
> nsSVGOuterSVGFrame ->
> nsBlockFrame ->
> nsBlockFrame ->
> nsCanvasFrame::ClearInvalidationStateBits ->
> nsHTMLScrollFrame ->
> viewportFrame ->
> nsSubDocumentFrame ->
> nsStackFrame ->
> nsBoxFrame ->
> nsBoxFrame ->
> nsBoxFrame ->
> nsDeckFrame ->
> nsBoxFrame ->
> nsBoxFrame ->
> nsBoxFrame ->
> nsBoxFrame ->
> nsDeckFrame ->
> nsBoxFrame ->
> nsDeckFrame ->
> nsDocElementBoxFrame ->
> nsRootBoxFrame ->
> viewportFrame ->
> 
> I think the main problem is like Jonathan mentioned why it can't pass
> 
> > while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)) 
> {
>  nsSVGEffects::InvalidateDirectRenderingObservers(parent);
> }
> 
> It might be at somewhere, it is set back NS_FRAME_DESCENDANT_NEEDS_PAINT to
> this SVG leaf again.

I notice the remote one can still jump into
while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)) 
{
> nsSVGEffects::InvalidateDirectRenderingObservers(parent);
}

And its frame and element is
> aFrame	   SVGFELeafFrame *	0x1222b1408	0x00000001222b1408
> aElement mozilla::dom::SVGFEFloodElement *	0x12f567900	0x000000012f567900

Therefore, I think the problem is why the SVG element doesn't has renderingObservers. That is also can make sense why Robert's patch works.

>  if (aElement->HasRenderingObservers()) {
    nsSVGRenderingObserverList *observerList = GetObserverList(aElement);
    if (observerList) {
      if (aFlags & INVALIDATE_REFLOW) {
        observerList->InvalidateAllForReflow();
      } else {
        observerList->InvalidateAll();
      }
    }
  }
Attached patch anim.txtSplinter Review
This seems to fix the testcase
Assignee: dmu → longsonr
Attachment #8748721 - Flags: review?(jwatt)
Attachment #8678446 - Attachment is obsolete: true
Local SVG:

nsSVGEffects::AddRenderingObserver
nsSVGRenderingObserver::GetReferencedFrame->
nsSVGRenderingObserver::GetReferencedFrame->
nsSVGFilterReference::GetFilterFrame->
nsSVGFilterReference::ReferencesValidResources->
nsSVGFilterChainObserver::ReferencesValidResources->
nsSVGEffects::EffectProperties::HasValidFilter->
nsSVGIntegrationUtils::ComputePostEffectsVisualOverflowRect->
ComputeEffectsRect->
nsIFrame::FinishAndStoreOverflow->
nsBlockFrame::UpdateOverflow->
OverflowChangedTracker::Flush->


The Remote SVG doesn't be executed updating so it can't have a renderingObserver.

I have confirmed nsSVGEffects::GetEffectProperties(nsIFrame *aFrame) which gives the effect properties correctly to the remote SVG. We just need to make the nsblockerFrame be added to mEntryList (https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.h#118) while RestyleManager calls flush. This should can be the loading time work and needn't check NS_FRAME_IS_NONDISPLAY at runtime.
Comment on attachment 8748721 [details] [diff] [review]
anim.txt

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsFrame.cpp
>@@ -5359,17 +5359,17 @@ static void InvalidateFrameInternal(nsIF
>   }
>   nsSVGEffects::InvalidateDirectRenderingObservers(aFrame);
>   bool needsSchedulePaint = false;
>   if (nsLayoutUtils::IsPopup(aFrame)) {
>     needsSchedulePaint = true;
>   } else {
>     nsIFrame *parent = nsLayoutUtils::GetCrossDocParentFrame(aFrame);
>     while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)) {
>-      if (aHasDisplayItem) {
>+      if (aHasDisplayItem && !parent->HasAnyStateBits(NS_FRAME_IS_NONDISPLAY)) {

In this patch, we seem to just need to add this line to make the remote SVG works. The other changes in these SVG sample tests will not affect the result. I am still thinking about how to make the remote SVG frame have renderingObservers.

>         parent->AddStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
>       }
>       nsSVGEffects::InvalidateDirectRenderingObservers(parent);
> 
>       // If we're inside a popup, then we need to make sure that we
>       // call schedule paint so that the NS_FRAME_UPDATE_LAYER_TREE
>       // flag gets added to the popup display root frame.
>       if (nsLayoutUtils::IsPopup(parent)) {
>@@ -9247,17 +9247,17 @@ nsIFrame::SetParent(nsContainerFrame* aP
>         break;
>       }
>       f->AddStateBits(NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE);
>     }
>   }
> 
>   if (HasInvalidFrameInSubtree()) {
>     for (nsIFrame* f = aParent;
>-         f && !f->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
>+         f && !f->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT | NS_FRAME_IS_NONDISPLAY);
>          f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
>       f->AddStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
>     }
>   }
> 
>   if (aParent->HasAnyStateBits(NS_FRAME_IN_POPUP)) {
>     AddInPopupStateBitToDescendants(this);
>   } else {
>diff --git a/layout/generic/nsSubDocumentFrame.cpp b/layout/generic/nsSubDocumentFrame.cpp
>--- a/layout/generic/nsSubDocumentFrame.cpp
>+++ b/layout/generic/nsSubDocumentFrame.cpp
>@@ -1187,17 +1187,17 @@ EndSwapDocShellsForViews(nsView* aSiblin
>     if (frame) {
>       nsIFrame* parent = nsLayoutUtils::GetCrossDocParentFrame(frame);
>       if (parent->HasAnyStateBits(NS_FRAME_IN_POPUP)) {
>         nsIFrame::AddInPopupStateBitToDescendants(frame);
>       } else {
>         nsIFrame::RemoveInPopupStateBitFromDescendants(frame);
>       }
>       if (frame->HasInvalidFrameInSubtree()) {
>-        while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT)) {
>+        while (parent && !parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT | NS_FRAME_IS_NONDISPLAY)) {
>           parent->AddStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
>           parent = nsLayoutUtils::GetCrossDocParentFrame(parent);
>         }
>       }
>     }
>   }
> }
>
The remote SVG frames do have rendering observers, it's just that the observers are not set on the frame that's altering but on one of its ancestors (or descendants) and then the NS_FRAME_DESCENDANT_NEEDS_PAINT shortcuts everything.

I'm not sure where you're trying to go with your comments really.
(In reply to Robert Longson from comment #22)
> The remote SVG frames do have rendering observers, it's just that the
> observers are not set on the frame that's altering but on one of its
> ancestors (or descendants) and then the NS_FRAME_DESCENDANT_NEEDS_PAINT
> shortcuts everything.
> 
> I'm not sure where you're trying to go with your comments really.

Yep. that is what I am saying. The observers is not set to nsBlockFrame <-- for <h1> while the frame is altering. The main reason is like you said NS_FRAME_DESCENDANT_NEEDS_PAINT blocks it.
Well, I think I got the point why this bug happened. In my investigation, RestyleManager will process restyle frames at the tick time. The nsSVGFilterFrame do have changes at every tick time, therefore, in the general case, it will destroy its renderingObserver and update its SVG filter property. Then, posting a restyle event to the restyle manager to pend this restyle element to mRestyleRoots. 

Following, the RestyleManger add this svg frame's parent nsBlockFrame to mEntryList. So, it can be added a new RenderingObserver at nsBlockFrame::UpdateOverflow();

Regarding to the remote svg, it has no reference frame at its SVGFilterProperty at the first tick time. (https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGEffects.cpp#319) Therefore, it can't get a new RenderingObserver for the next update.


Go on, I will start to check why it doesn't has this reference frame.
Daosheng, Robert, who is actually working on this bug?
It's assigned to me and the patch is mine.
(In reply to Astley Chen [:astley] (UTC+8) from comment #25)
> Daosheng, Robert, who is actually working on this bug?

Astley, I will move to another bug although this bug is originally assigned to me as Comment 19.
I din't notice i'd taken over, I simply implemented what I'd already suggested in comment 18 which I made before it had been assigned to you.
Sorry, I meant comment 16 there.
Robert, it's fine as long as we keep bug running. Daosheng, thanks for feedback.
Comment on attachment 8748721 [details] [diff] [review]
anim.txt

Sorry I took so long to get to this.
Attachment #8748721 - Flags: review?(jwatt) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8e2b503a96
fix animation of filters in resource documents. r=jwatt
https://hg.mozilla.org/mozilla-central/rev/bc8e2b503a96
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.