846028 - SVG images generate too many OnStopFrame calls

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

I noticed as part of investigating bug 835658 that test_animSVGImage.html (introduced in bug 649440) generates way more VectorImage::Draw calls than I expected. These calls are happening even when nothing has changed about the underyling SVG document (ie, the current time of the SVG document is unchanged). 

The reason this is happening is that SVGRootRenderingObserver adds itself immediately back to the list of observers of its target in DoUpdate. That means that it receives duplicate invalidation notifications (since a single event that invalidates the underlying SVG document can generate many of them) and it calls VectorImage::InvalidateObserver for each one, generating OnStopFrame events each time. test_animSVGImage takes a screenshot each time, causing an additional call to VectorImage::Draw.

The real problem is that there are too many OnStopFrame events being generated. My recommended fix is to leave SVGRootRenderingObserver out of the rendering observer list until VectorImage::Draw gets called. This seems logical to me - an SVG frame is "generated" whenever VectorImage::Draw gets called, so there should be a 1-to-1 correspondence between OnStopFrame and VectorImage::Draw.

Comment 1

[Daniel Holbert [:dholbert]]

paraphrasing IRC:
{
<dholbert>
 seth, just to be sure I understand:
 Are you simply proposing that we delay adding ourselves back to the list of observers until Draw gets called?

<seth>
 correct.

<dholbert>
 so we'd effectively be coalescing invalidates

<seth>
 yup, exactly

<dholbert>
 I sort of assumed that would already be happening at a higher level, I think.
 I'm a bit surprised that it's not.
 But even if this *is* supposed to happen elsewhere, this seems reasonable
 No sense in sending invalidations that we already know are redundant.

<seth>
 yeah, that's my view too.
}

Comment 2

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

Attachment: Coalesce invalidations in VectorImage.

Proposed patch. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=de222d1a0941

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 719202 [details] [diff] [review]
Coalesce invalidations in VectorImage.

>+  void ResumeListening()
>+  {
>+    if (!mInObserverList) {
>+      Element* elem = GetTarget();
>+      if (elem) {
>+        nsSVGEffects::AddRenderingObserver(elem, this);
>+        mInObserverList = true;
>+      }
>+    }
>+  }

It looks like nsSVGRenderingObserver::GetReferencedElement() basically does all of this already (by design).

I wonder if we should just call that (perhaps directly from Draw), to share code?

>       mVectorImage->InvalidateObserver();
>     }
[...]
>+    // We may have been removed from the observer list by our caller. Rather
>+    // than add ourselves back here, we wait until Draw gets called, ensuring
>+    // that we generate FrameChanged and OnStopFrame at most once per Draw call.
>   }

I don't think that last line is quite what you want to say.  Technically, we already *do* generate one FrameChanged/OnStopFrame per draw call, since they're what trigger the Draw calls. :)  The problem is we're generating lots of them (and, consequently, lots of draw calls).

Maybe replace that last line to say "...that we coalesce invalidations between Draw calls"

>@@ -749,16 +757,20 @@ VectorImage::Draw(gfxContext* aContext,
> 
>+  // Allow ourselves to generate FrameChanged and OnStopFrame events again.
>+  if (mRenderingObserver)
>+    mRenderingObserver->ResumeListening();

No need to null-check mRenderingObserver. It's initialized in OnSVGDocumentLoaded(), which is the same place we set mIsFullyLoaded to true -- and if we didn't visit that code, we'd have already bailed out of Draw right at its initial mIsFullyLoaded check.

If you like, you could add MOZ_ASSERT(mRenderingObserver, "should have initialized rendering observer when document loaded"); or something to that effect, though.

Also: As above, I wonder if this should just call mRenderingObserver->GetReferencedElement()? (and ignore the result)

Comment 4

[Daniel Holbert [:dholbert]]

MXR link for GetReferencedElement impl:
 https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGEffects.cpp#102

Comment 5

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

Thanks for the review!

(In reply to Daniel Holbert [:dholbert] from comment #3)
> It looks like nsSVGRenderingObserver::GetReferencedElement() basically does
> all of this already (by design).
> 
> I wonder if we should just call that (perhaps directly from Draw), to share
> code?

Ugh, I'm not a big fan of getters that have implicit side effects like that. I guess we could use it with a comment explaining what's happening.

> Maybe replace that last line to say "...that we coalesce invalidations
> between Draw calls"

Sounds good.

> If you like, you could add MOZ_ASSERT(mRenderingObserver, "should have
> initialized rendering observer when document loaded"); or something to that
> effect, though.

Cool. I'll switch it to an assert.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #5)
> Ugh, I'm not a big fan of getters that have implicit side effects like that.

Yeah... It's a bit gross, but it's nice for the code-sharing (and assertion-sharing) benefits.  (It'd be nicer if GetReferencedElement() were split into a helper registration-method and a getter or something.)

Maybe we could keep ResumeListening(), and just make it call GetReferencedElement() (with a one-line comment explaining why)?

Comment 7

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

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Maybe we could keep ResumeListening(), and just make it call
> GetReferencedElement() (with a one-line comment explaining why)?

Sounds like a plan.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 719202 [details] [diff] [review]
Coalesce invalidations in VectorImage.

cool. r=me w/ that & my other notes from comment 3 addressed.

Thanks!

Comment 9

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

Attachment: Coalesce invalidations in VectorImage.

Updated patch. No meaningful changes to behavior, so I don't think this needs another try run. If try looks good this is ready to go.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 719293 [details] [diff] [review]
Coalesce invalidations in VectorImage.

Two minor nits:

>+  // Allow ourselves to another FrameChanged and OnStopFrame event.

I think there's a word missing there -- s/another/fire another/?

>+  NS_ABORT_IF_FALSE(mRenderingObserver, "Should have initialized rendering "
>+                                        "observer in OnSVGDocumentLoaded");

Just use MOZ_ASSERT. It's fewer characters and the New Hotness for fatal assertions.

(We should probably just switch over all the NS_ABORT_IF_FALSE's in this file; feel free to file a bug on that, or maybe I will at some point.)

Comment 11

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

Thanks Daniel; I'll take care of those things.

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Just use MOZ_ASSERT. It's fewer characters and the New Hotness for fatal
> assertions.
> 
> (We should probably just switch over all the NS_ABORT_IF_FALSE's in this
> file; feel free to file a bug on that, or maybe I will at some point.)

Sweet. I didn't realize MOZ_ASSERT was fatal; I thought it was one of those (IMO poorly named) "nonfatal assertions". I'll switch things over in this patch, and start using MOZ_ASSERT in the future.

Proposition: you could do the switchover of the other instances as part of bug 846078. Consider that r=me in advance if you do.

Comment 12

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

Attachment: Coalesce invalidations in VectorImage.

Updated patch.

Comment 13

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

Try looks OK.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf12e699dc7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing reftest crashes (with an assertion that I'm betting you'll find interesting).
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2b8a955b46

https://tbpl.mozilla.org/php/getParsedLog.php?id=20197743&tree=Mozilla-Inbound

14:56:54     INFO -  REFTEST TEST-START | file:///Users/cltbld/talos-slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/background-image-rect-1svg.html | 6721 / 9177 (73%)
14:56:54     INFO -  ++DOMWINDOW == 445 (0x14790ea48) [serial = 18339] [outer = 0x1108048c8]
14:56:54     INFO -  Assertion failure: mRenderingObserver (Should have initialized rendering observer in OnSVGDocumentLoaded), at ../../../image/src/VectorImage.cpp:769
14:56:56  WARNING -  TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/background-image-rect-1svg.html | Exited with code 1 during test run
14:56:56  WARNING -  This is a harness error.
14:56:56     INFO -  INFO | automation.py | Application ran for: 0:35:54.094942
14:56:56     INFO -  INFO | automation.py | Reading PID log: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmp2UFHtEpidlog
14:57:12     INFO -  PROCESS-CRASH | file:///Users/cltbld/talos-slave/test/build/tests/reftest/tests/layout/reftests/svg/as-image/background-image-rect-1svg.html | application crashed [@ mozilla::image::VectorImage::Draw(gfxContext*, gfxPattern::GraphicsFilter, gfxMatrix const&, gfxRect const&, nsIntRect const&, nsIntSize const&, mozilla::SVGImageContext const*, unsigned int)]
14:57:12     INFO -  Crash dump filename: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmp4tsrTz/minidumps/68ADE2E9-C2DB-433B-BA20-4F2071C5C77E.dmp
14:57:12     INFO -  Operating system: Mac OS X
14:57:12     INFO -                    10.7.2 11C74
14:57:12     INFO -  CPU: amd64
14:57:12     INFO -       family 6 model 23 stepping 10
14:57:12     INFO -       2 CPUs
14:57:12     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
14:57:12     INFO -  Crash address: 0x0
14:57:12     INFO -  Thread 0 (crashed)
14:57:12     INFO -   0  XUL!mozilla::image::VectorImage::Draw(gfxContext*, gfxPattern::GraphicsFilter, gfxMatrix const&, gfxRect const&, nsIntRect const&, nsIntSize const&, mozilla::SVGImageContext const*, unsigned int) [VectorImage.cpp:57fc2a37629f : 768 + 0x0]
14:57:12     INFO -      rbx = 0x00007fff747ec630   r12 = 0x0000003200000032
14:57:12     INFO -      r13 = 0x000000011e00b0d0   r14 = 0x000000011a973e10
14:57:12     INFO -      r15 = 0x000000011a61aaa0   rip = 0x00000001012b770f
14:57:12     INFO -      rsp = 0x00007fff5fbfb330   rbp = 0x00007fff5fbfb430
14:57:12     INFO -      Found by: given as instruction pointer in context
14:57:12     INFO -   1  XUL!_ZZN7mozilla5imageL14get_header_strEPcS1_mE3hex + 0x2c081
14:57:12     INFO -      rip = 0x0000000103783a92   rsp = 0x00007fff5fbfb428
14:57:12     INFO -      rbp = 0x00007fff5fbfb430
14:57:12     INFO -      Found by: stack scanning
14:57:12     INFO -   2  XUL!DrawImageInternal [nsLayoutUtils.cpp:57fc2a37629f : 4001 + 0x38]
14:57:12     INFO -      rip = 0x0000000101392871   rsp = 0x00007fff5fbfb440
14:57:12     INFO -      rbp = 0x00007fff5fbfb580
14:57:12     INFO -      Found by: stack scanning

Comment 15

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

Consider the problem identified. When a VectorImage is created via ExtractFrame, it doesn't have a rendering observer. That means we should assert (mRenderingObserver || mRestrictedRegion), and we need an if after the assert instead of blinding dereferencing mRenderingObserver. New patch coming up.

Comment 16

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

Attachment: Coalesce invalidations in VectorImage.

Updated patch. Note that by switching back to an if statement we are back to the situation in the version of the patch that had green reftests on try, so I don't think we need another try run here.

Comment 17

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

Pushed to inbound again.

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Scoobidiver (away)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a92339a24437