649440 - Fire OnStopFrame notifications per-'frame' (draw-request) in SVG images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

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]]

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]]

Attachment: 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]]

(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 [:bzbarsky]]

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)]

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)]

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]]

(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]]

Attachment: 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]]

Attachment: 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]]

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)]

(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]]

Attachment: 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]]

(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)]

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 [:bzbarsky]]

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]]

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]]

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]]

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