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

RESOLVED FIXED in mozilla6

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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 }
(Assignee)

Updated

6 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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.)
(Assignee)

Comment 2

6 years ago
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)
Attachment #526870 - Flags: review?(joe)
(Assignee)

Comment 3

6 years ago
(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. :))
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).
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 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.
Attachment #526870 - Flags: review?(joe) → review+
(Assignee)

Comment 7

6 years ago
(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). :)
(Assignee)

Comment 8

6 years ago
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.
Attachment #528179 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Blocks: 612214
(Assignee)

Comment 9

6 years ago
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 "{}".
Attachment #528179 - Attachment is obsolete: true
Attachment #528179 - Flags: review?(joe)
(Assignee)

Comment 10

6 years ago
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.
(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.....
(Assignee)

Comment 12

6 years ago
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).
Attachment #528182 - Attachment is obsolete: true
Attachment #528371 - Flags: review?(joe)
(Assignee)

Comment 13

6 years ago
(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 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.
Attachment #528371 - Flags: review?(joe) → review+
It's async in general, but certain operations will force it to be processed; taking a snapshot of the window is one of those.
(Assignee)

Comment 16

6 years ago
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Comment 17

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/fa06ce52a09e
You need to log in before you can comment on or make changes to this bug.