911862 - Make VectorImage use the refresh driver to send out image frame change notifications

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

It seems to me that calling imgDecoderObserver::OnStopFrame() in VectorImage::InvalidateObserver() is bad. Calling it there is way too early, and means that we can do things like:

  mozilla::RestyleManager::ProcessPendingRestyles
  PresShell::ResizeReflowIgnoreOverride
  PresShell::ResizeReflow
  nsViewManager::DoSetWindowDimensions
  nsViewManager::SetWindowDimensions
  nsDocumentViewer::SetBounds
  mozilla::image::SVGDocumentWrapper::UpdateViewportBounds
  mozilla::image::VectorImage::Draw
  DrawImageInternal
  nsLayoutUtils::DrawSingleImage
  nsImageFrame::PaintImage
  nsDisplayImage::Paint
  mozilla::FrameLayerBuilder::DrawThebesLayer
  mozilla::layers::BasicLayerManager::PaintSelfOrChildren
  mozilla::layers::BasicLayerManager::PaintSelfOrChildren
  nsLayoutUtils::PaintFrame
  PresShell::RenderDocument
  mozilla::dom::CanvasRenderingContext2D::DrawWindow
  mozilla::dom::CanvasRenderingContext2DBinding::drawWindow
  mozilla::dom::CanvasRenderingContext2DBinding::genericMethod
  js::Invoke
  js::Invoke
  js::DirectProxyHandler::call
  js::CrossCompartmentWrapper::call
  js::Proxy::call
  proxy_Call
  js::Invoke
  Interpret
  js::RunScript
  js::Invoke
  js::Invoke
  js::DirectProxyHandler::call
  js::CrossCompartmentWrapper::call
  js::Proxy::call
  proxy_Call
  js::Invoke
  Interpret
  js::RunScript
  js::Invoke
  js::Invoke
  JS_CallFunctionValue
  nsXPCWrappedJSClass::CallMethod
  nsXPCWrappedJS::CallMethod
  mozilla::image::ScriptedNotificationObserver::Notify
  nsImageLoadingContent::Notify
  imgRequestProxy::OnStopFrame
  imgStatusTracker::SendStopFrame
  imgStatusTracker::OnStopFrame
  mozilla::image::VectorImage::InvalidateObserver
  mozilla::image::SVGRootRenderingObserver::DoUpdate
  nsSVGRenderingObserver::InvalidateViaReferencedElement
  nsSVGRenderingObserverList::InvalidateAll
  nsSVGEffects::InvalidateRenderingObservers
  nsFrame::DidSetStyleContext
  nsSVGPathGeometryFrame::DidSetStyleContext
  nsIFrame::SetStyleContext
  mozilla::ElementRestyler::RestyleSelf
  mozilla::ElementRestyler::Restyle
  mozilla::RestyleManager::ComputeStyleChangeFor
  mozilla::RestyleManager::RestyleElement
  mozilla::RestyleTracker::ProcessOneRestyle
  mozilla::RestyleTracker::DoProcessRestyles
  mozilla::RestyleTracker::ProcessRestyles
  mozilla::RestyleManager::ProcessPendingRestyles
  PresShell::FlushPendingNotifications
  PresShell::FlushPendingNotifications
  nsDocument::FlushPendingNotifications
  nsComputedDOMStyle::GetPropertyCSSValue
  nsComputedDOMStyle::GetPropertyValue
  nsComputedDOMStyle::GetPropertyValue
  GetCSSComputedValue
  nsSMILCompositor::ComposeAttribute
  DoComposeAttribute
  nsTHashtable<nsSMILCompositor>::s_EnumStub
  nsSMILAnimationController::DoSample
  nsSMILAnimationController::DoSample
  nsSMILTimeContainer::Sample
  nsSMILAnimationController::Resume
  mozilla::image::SVGDocumentWrapper::StartAnimation
  mozilla::image::VectorImage::StartAnimation
  mozilla::image::ImageResource::EvaluateAnimation
  mozilla::image::VectorImage::RequestRefresh
  nsRefreshDriver::ImageRequestEnumerator
  nsTHashtable<nsISupportsHashKey>::s_EnumStub
  nsRefreshDriver::Tick
  mozilla::RefreshDriverTimer::TickDriver
  mozilla::RefreshDriverTimer::Tick
  mozilla::RefreshDriverTimer::TimerTick
  nsTimerImpl::Fire
  nsTimerEvent::Run
  nsThread::ProcessNextEvent

In this stack nsSMILAnimationController::DoSample starts updating the attributes/properties in the document for SMIL animation changes, but only gets as far as updating the first attribute/property before the OnStopFrame() call causes us to end up running script that calls drawWindow(). Ugh!

FWIW the OnStopFrame call was originally added to VectorImage::InvalidateObserver in bug 649440.

Anything that changes the rendering of an SVG-as-an-image document causes its refresh driver to tick, which calls VectorImage::RequestRefresh. This seems like a much better place to put the FrameChanged and OnStopFrame calls that are currently in VectorImage::InvalidateObserver. That way these calls will occur _after_ all the changes that should be included in the new "image frame".

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

This patch moves the FrameChanged() and OnStopFrame() calls that are currently in VectorImage::InvalidateObserver to VectorImage::RequestRefresh. It also removes a bunch of code that's no longer needed as a result.

Comment 2

[Jonathan Watt [:jwatt]]

https://tbpl.mozilla.org/?tree=Try&rev=41c9f6e57fe9

Comment 3

[Jonathan Watt [:jwatt]]

Try run looks good.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #0)
> Anything that changes the rendering of an SVG-as-an-image document causes
> its refresh driver to tick, which calls VectorImage::RequestRefresh.

I'm not sure that's true...

I think RequestRefresh is triggered by the *outer* refresh driver, in the host document. That method was added in http://hg.mozilla.org/mozilla-central/rev/3f8fbd2c0aa8 as a way for the host refresh driver to throttle the animated GIF frame-change-rate.  (and, importantly, to avoid spinning our wheels on an animated GIF that no host document is hooked up to)

So I don't think RequestRefresh is the right place to do things here.  I think the only place we can detect "anything that changes the rendering of an SVG-as-an-image document" is in the rendering observer, in the code that your patch removes.

Comment 5

[Daniel Holbert [:dholbert]]

I suspect we want to do the following:
 a) Make SVGRootRenderingObserver derive from nsARefreshObserver, and register it as a refresh observer at construction time. (and unregister it at destruction time).
 b) In DoUpdate(), don't call InvalidateObserver() -- instead, just set a flag.
 c) In the refresh observer's WillRefresh() method, check that flag, and if it's true, clear it & call mVectorImage->InvalidateObserver().

We could also make the nsARefreshObserver be a helper-object, but I'm currently thinking it'd be simpler to just have it be the same object as SVGRootRenderingObserver.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I think RequestRefresh is triggered by the *outer* refresh driver, in the
> host document.

Indeed. That's the reason for the infinite invalidation I mentioned on IRC - the FrameChanged()/OnStopFrame() call in RequestRefresh() causes us to tell the embedding document that we've changed, causing it to schedule another tick, causing RequestRefresh() to be called again.

Comment 7

[Jonathan Watt [:jwatt]]

Why do you think using a refresh observer and calling OnStopFrame under WillRefresh is better than calling it under RequestRefresh? As I understand it, given this setup each WillRefresh would be followed by a RequestRefresh call, so doing it in RequestRefresh is just doing it later. Also note that the SVG-as-an-image's nsSMILAnimationController is an nsARefreshObserver that resamples and updates animated attributes/properties under WillRefresh, so we'd then have the issue of needing to make sure that the nsSMILAnimationController was registered before the SVGRootRenderingObserver or, more precisely], that we'd need WillRefresh to be called on the two observers in that order.

Comment 8

[Jonathan Watt [:jwatt]]

Specifically, would you see any problems with the current patch, but with the SVGRootRenderingObserver code kept, and the FrameChanged()/OnStopFrame() calls gated on the setting of a bit by SVGRootRenderingObserver?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #7)
> Why do you think using a refresh observer and calling OnStopFrame under
> WillRefresh is better than calling it under RequestRefresh?

Hmm... I actually am fine with either, I think.

My suggested refresh observer solution is basically a representation of this idea: "every time the internal document changes its rendering in a way that would be painted onscreen if you were viewing it directly, consider it a new image frame."

Your suggestion of using RequestRefresh is similar, except with possibly-less-frequent frames, gated by the host document's refresh driver instead of the image's internal refresh driver.

> As I understand
> it, given this setup each WillRefresh would be followed by a RequestRefresh
> call

(Not quite -- those calls are fired by different refresh drivers, so e.g. if we're in a background tab, we'll receive very infrequent RequestRefresh calls, from the background tab's refresh driver, but we'd receive more-frequent WillRefresh calls from our own internal refresh driver.  For that reason, it's probably good to put this in RequestRefresh, for some image-in-background-tab perf wins.)

> Also note that
> the SVG-as-an-image's nsSMILAnimationController is an nsARefreshObserver
> that resamples and updates animated attributes/properties under WillRefresh,
> so we'd then have the issue of needing to make sure that the
> nsSMILAnimationController was registered before the SVGRootRenderingObserver
> or, more precisely], that we'd need WillRefresh to be called on the two
> observers in that order.

(That's not a problem -- the refresh driver's registration method lets you specify a "flush type", and we could use that to ensure our new observer gets notified after nsSMILAnimationController is done refreshing.)

(In reply to Jonathan Watt [:jwatt] from comment #8)
> Specifically, would you see any problems with the current patch, but with
> the SVGRootRenderingObserver code kept, and the FrameChanged()/OnStopFrame()
> calls gated on the setting of a bit by SVGRootRenderingObserver?

That'd be fine with me - per above, that's probably more performant than what I suggested, when we're in a background tab.

Comment 10

[Daniel Holbert [:dholbert]]

One caveat... Now I'm second-guessing whether it's even safe to invoke things that can run script (OnStopFrame() in this case) from within a refresh driver tick (via the image's RequestRefresh() in this case, per comment 8, which is called by a refresh observer in the host document).

bz, perhaps you know?  Would it be worth delaying OnStopFrame() to happen after the refresh tick is over using a helper-nsIRunnable? (or maybe there's already a script blocker in play (or we should add one?))

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> One caveat... Now I'm second-guessing whether it's even safe to invoke
> things that can run script

Note that that's exactly what happens now, since OnStopFrame is called under InvalidateObserver, which is called as properties/attributes change as a new SMIL sample is processed under a refresh driver tick. Not that I'm saying this is a good thing...

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

Patch implementing the change described in comment 8.

Comment 13

[Seth Fowler [:seth] [:s2h]]

This bug seems like it's moving us in a very good direction. Thanks Jonathan.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 799192 [details] [diff] [review]
patch

Yup, this looks great! Just a few nits:

> VectorImage::VectorImage(imgStatusTracker* aStatusTracker,
>                          nsIURI* aURI /* = nullptr */) :
[...]
>+  mNotifyRenderingChanged(true)
> {

Why does this new flag start out "true"? I think our existing machinery will get us invalidated when we need to be, so we should be able to start it as "false" and then it should get set to true (via InvalidateObserver) when we actually have something ready to paint.

(In the unlikely case that you're getting problems from starting it as false, you might need to add a call to InvalidateObserver() somewhere, to manually set it to true after we've actually got something ready to paint. I'd feel better about that, than just starting it at "true" which might make us accidentally invalidate too early.)

(See also next comment about "InvalidateObserver" naming) 

> void
> VectorImage::InvalidateObserver()
> {
>-  if (mStatusTracker) {
>-    mStatusTracker->FrameChanged(&nsIntRect::GetMaxSizedIntRect());
>-    mStatusTracker->OnStopFrame();
>-  }
>+  mNotifyRenderingChanged = true;

I'd rather not name this "InvalidateObserver" anymore, since (with your changes) it no longer actually Invalidates the Observer. :)
Maybe "TogglePendingInvalidation", or "InvalidateObserverSoon", or something along those lines?

>diff --git a/image/src/VectorImage.h b/image/src/VectorImage.h
>--- a/image/src/VectorImage.h
>+++ b/image/src/VectorImage.h
>@@ -84,16 +84,17 @@ private:
>   nsRefPtr<SVGLoadEventListener>     mLoadEventListener;
>   nsRefPtr<SVGParseCompleteListener> mParseCompleteListener;
> 
>   bool           mIsInitialized;          // Have we been initalized?
>   bool           mIsFullyLoaded;          // Has the SVG document finished loading?
>   bool           mIsDrawing;              // Are we currently drawing?
>   bool           mHaveAnimations;         // Is our SVG content SMIL-animated?
>                                           // (Only set after mIsFullyLoaded.)
>+  bool           mNotifyRenderingChanged;

I'd prefer that we name this new flag "mHasPendingInvalidation" or something like that (with something conveying "pending" in the name, to indicate that it's a flag that we check & flush periodically).

Also: Please add a comment next to the declaration, to indicate what it's for (like the other bools above it).

r=me with that.

Comment 15

[Jonathan Watt [:jwatt]]

Thanks, Daniel!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f12a3d9e4cd6

Boris, I'd still like to hear your thoughts on comment 10, and I'll follow-up on that if necessary.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f12a3d9e4cd6

Comment 17

[Boris Zbarsky [:bzbarsky]]

> bz, perhaps you know

Running script during a refresh tick is generally ok: requestAnimationFrame does, as do some other things.

But doing it specifically during RequestRefresh is NOT OK right now, I suspect, because the code that calls RequestRefresh is a hashtable enumerator.  So if script can modify the hashtable while it's being enumerated, we lose.

Please file a followup to change to enumerating the hashtable to gather up all the things to notify and then doing a separate notification pass?

Comment 18

[Daniel Holbert [:dholbert]]

Yikes, good catch!  Sorry for missing that. I'll file a followup for that later today. [leaving needinfo=me until I do]

Comment 19

[Daniel Holbert [:dholbert]]

(Filed followup bug 913247)

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 799192 [details] [diff] [review]
patch

Requesting approval as this was split out from bug 907503, so the patch there depends on this patch. See bug 907503 comment 43 for the approval request there.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking here too since this is related to tracked bug 907503

Comment 22

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d9e7eb92b36

Comment 23

[u279076]

Does this need any testing to verify it's working correctly?

Comment 24

[Daniel Holbert [:dholbert]]

I don't think so; this is basically a refactoring, with no real behavior changes, in support of bug 907503.

Comment 25

[Jonathan Watt [:jwatt]]

Strictly speaking it is a behavior change, but the change in behavior should not really be detectable by QA. It's conceivable that the change could have introduced bugs, but any testing to verify bug 907503 should also cover this bug. So no, I agree with Daniel that no special testing is required here. Thanks, Anthony.