Closed Bug 911862 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 + fixed
firefox26 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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".
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #798653 - Flags: review?(dholbert)
Try run looks good.
(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.
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.
(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.
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.
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?
(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.
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?))
Flags: needinfo?(bzbarsky)
(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...
Attached patch patchSplinter Review
Patch implementing the change described in comment 8.
Attachment #798653 - Attachment is obsolete: true
Attachment #798653 - Flags: review?(dholbert)
Attachment #799192 - Flags: review?(dholbert)
This bug seems like it's moving us in a very good direction. Thanks Jonathan.
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.
Attachment #799192 - Flags: review?(dholbert) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/f12a3d9e4cd6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
> 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?
Flags: needinfo?(bzbarsky) → needinfo?(dholbert)
Yikes, good catch!  Sorry for missing that. I'll file a followup for that later today. [leaving needinfo=me until I do]
(Filed followup bug 913247)
Flags: needinfo?(dholbert)
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.
Attachment #799192 - Flags: approval-mozilla-aurora?
Blocks: 907503
Attachment #799192 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking here too since this is related to tracked bug 907503
Does this need any testing to verify it's working correctly?
Flags: needinfo?(jwatt)
I don't think so; this is basically a refactoring, with no real behavior changes, in support of bug 907503.
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.
Flags: needinfo?(jwatt)
Whiteboard: [qa-]
Depends on: 922899
Depends on: 1008598
Depends on: 1113083
You need to log in before you can comment on or make changes to this bug.