Last Comment Bug 649440 - Fire OnStopFrame notifications per-'frame' (draw-request) in SVG images
: Fire OnStopFrame notifications per-'frame' (draw-request) in SVG images
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
Depends on:
Blocks: 612214
  Show dependency treegraph
 
Reported: 2011-04-12 11:41 PDT by Daniel Holbert [:dholbert]
Modified: 2011-12-08 08:45 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (3.82 KB, patch)
2011-04-18 16:32 PDT, Daniel Holbert [:dholbert]
joe: review+
Details | Diff | Splinter Review
test as patch (9.70 KB, patch)
2011-04-25 14:31 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
test as patch (9.70 KB, patch)
2011-04-25 14:40 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
test as patch (now with more consistent snapshotWindow calls) (9.72 KB, patch)
2011-04-26 11:41 PDT, Daniel Holbert [:dholbert]
joe: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-04-12 11:41:28 PDT
Joe says in IRC that we should be firing OnStopFrame notifications for SVG Images - that'd make testing animated vector images easier and more consistent between vector & raster images.

We probably want to do this with the existing FrameChanged call here:
> 712 VectorImage::InvalidateObserver()
> 713 {
> 714   nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
> 715   if (observer) {
> 716     observer->FrameChanged(this, &nsIntRect::GetMaxSizedIntRect());
> 717   }
> 718 }
Comment 1 Daniel Holbert [:dholbert] 2011-04-12 11:51:03 PDT
Taking, so I don't lose track of this -- I hope to follow up in the next week or so.  (Feel free to steal this, though.)
Comment 2 Daniel Holbert [:dholbert] 2011-04-18 16:32:48 PDT
Created attachment 526870 [details] [diff] [review]
fix v1

I believe this is all that's needed here.  (I've thrown in a few MOZ_LIBXUL ifdef removals in VectorImage, too.)

I'm actually not sure how to test this, though.  (but once I find out, I'm hoping it'll help with randomorange fixing in bug 612214!)  IIUC, I should be able to implement the imgIDecoderObserver interface in JS, and somehow register to receive callbacks, but I'm not sure how to register.

It looks like modules/libpr0n/test/unit/image_load_helpers.js is the only testing code that tries to do something like this right now, but I don't think it's quite the same as what I'd need to do.

joe, any tips on how I'd register to receive OnStopFrame notifications in a test? (in a mochitest-chrome test, I presume)
Comment 3 Daniel Holbert [:dholbert] 2011-04-20 12:27:49 PDT
(In reply to comment #2)
> joe, any tips on how I'd register to receive OnStopFrame notifications in a
> test? (in a mochitest-chrome test, I presume)

CC'ing bz in case he can help on the above. (I think I want to do what bz suggested in bug 641198 comment 24 -- but per comment 2, I'm not sure how, and I can't find any tests to crib from. :))
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-04-20 19:07:02 PDT
Assuming you have an <html:img> or some such, QI it to nsIImageLoadingContent and use addObserver on the result to add your imgIDecoderObserver.  You may need to removeObserver by hand to make sure you don't leak; I dunno that we cycle-collect those (we probably should).
Comment 5 Joe Drew (not getting mail) 2011-04-21 13:59:08 PDT
modules/libpr0n/test/unit/async_load_tests.js is another, utterly over-the-top way of getting those callbacks (by calling LoadImage manually and passing your own object as the observer). But Boris' suggestion is much much better.
Comment 6 Joe Drew (not getting mail) 2011-04-21 14:00:14 PDT
Comment on attachment 526870 [details] [diff] [review]
fix v1

I would not cry if the MOZ_ENABLE_LIBXUL changes were in a separate patch in the final push.
Comment 7 Daniel Holbert [:dholbert] 2011-04-25 14:26:39 PDT
(In reply to comment #6)
> I would not cry if the MOZ_ENABLE_LIBXUL changes were in a separate patch in
> the final push.

I've split them out locally, to NS_ENSURE(joe_will_not_cry). :)
Comment 8 Daniel Holbert [:dholbert] 2011-04-25 14:31:56 PDT
Created attachment 528179 [details] [diff] [review]
test as patch

Here's a test patch.  This burns down the orange monstrosity that is img-anim-1.html, and from its ashes creates a beautiful mochitest-chrome jewel.
Comment 9 Daniel Holbert [:dholbert] 2011-04-25 14:40:50 PDT
Created attachment 528182 [details] [diff] [review]
test as patch

(this version adds a missing semicolon - didn't affect functionality, but should be there for consistency)

BTW, the stub implementations on ImageDecoderObserverStub probably aren't 100% required, but they're good for cleanliness, because otherwise get get ugly error-spew in the terminal (and probably in the error console too) every time we attempt to call a nonexistent imgIDecoderObserver method on the object.

ImageDecoderObserverStub is basically taken directly from libpr0n/test/unit/image_load_helpers.js, with all the impls replaced with "{}".
Comment 10 Daniel Holbert [:dholbert] 2011-04-25 14:52:07 PDT
A few other clarifying notes on the test patch:

 - When reviewing, start reading in "main()" (at the bottom of the mochitest).  Hopefully it's pretty self-explanatory.

 - I upped the animation-duration in lime-anim-100x100.svg from .001 sec to .1 sec, since (unlike img-anim-1.html) the new test doesn't rely on the animation completing near-instantaneously.  (I could up it more, of course, but I don't want the test to take up too much time.)

 - The mochitest references bug 610419 rather than this bug, since that's what img-anim-1.html was testing.

 - I made the mochitest intentionally time out after 120 seconds, rather than falling back on the harness-imposed timeout, so that in the unlikely event that we take that long, we can print a little extra debug information.
Comment 11 Joe Drew (not getting mail) 2011-04-26 08:04:44 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > I would not cry if the MOZ_ENABLE_LIBXUL changes were in a separate patch in
> > the final push.
> 
> I've split them out locally, to NS_ENSURE(joe_will_not_cry). :)

And then you went and wrote that comment, and now I will cry all the time.....
Comment 12 Daniel Holbert [:dholbert] 2011-04-26 11:41:13 PDT
Created attachment 528371 [details] [diff] [review]
test as patch (now with more consistent snapshotWindow calls)

d'oh! apparently when I posted the updated version of the test patch, I forgot to request review on the new version.

So, as long as I'm adding a review request now, I'll add one more marginal tweak -- I've just made the snapshotWindow() calls more consistent (always passing false as the second "withCaret" argument, rather than omitting it).
Comment 13 Daniel Holbert [:dholbert] 2011-04-26 12:48:46 PDT
(btw, this bug's fix + test had 2 full-green mochitest-oth runs on TryServer yesterday.  I'm doing another TryServer run of the test w/out the fix right now, and it's failed as expected (hitting my internal timeout) on the one mochitest-oth cycle that's completed so far.)
Comment 14 Joe Drew (not getting mail) 2011-04-28 13:33:10 PDT
Comment on attachment 528371 [details] [diff] [review]
test as patch (now with more consistent snapshotWindow calls)

Looks lovely! Make sure that style.display = "none" is done synchronously, though.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-04-28 14:03:42 PDT
It's async in general, but certain operations will force it to be processed; taking a snapshot of the window is one of those.
Comment 16 Daniel Holbert [:dholbert] 2011-04-28 19:29:10 PDT
Landed the patch, split up as requested in comment 6:
   http://hg.mozilla.org/mozilla-central/rev/340131166fbc
   http://hg.mozilla.org/mozilla-central/rev/cdbeb9df92d3
and the test:
 http://hg.mozilla.org/mozilla-central/rev/3b1fa83ffc99

(I made a minor tweak to the test before landing, as I mentioned to joe on IRC -- I just delayed the initialization of gMyDecoderObserver so that it gets set up all in one place, and tweaked some comments accordingly)
Comment 17 Daniel Holbert [:dholbert] 2011-12-07 14:12:01 PST
The tests patch here copied "lime-anim-100x100.svg" from reftests over to mochitests, and I just realized that the reftests version of this file is now unused (as shown by the following mxr search, which only turns up references in mochitests):
http://mxr.mozilla.org/mozilla-central/search?string=lime-anim-100x100.svg

Pushed a followup to remove the old/unused reftest copy:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fa06ce52a09e
Comment 18 Ed Morley [:emorley] 2011-12-08 08:45:50 PST
https://hg.mozilla.org/mozilla-central/rev/fa06ce52a09e

Note You need to log in before you can comment on or make changes to this bug.