1084679 - Track invalidation rects during image decoding on Decoder, not imgStatusTracker

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

To bring sanity to the imgStatusTracker code (bug 910441), but also because it's going to be necessary in the long term to support multiple decoders for the same RasterImage (bug 1079627), we should track our invalidation rect during decoding on Decoder rather than on imgStatusTracker.

The nice thing is, we basically do this already! We need some refactoring to completely remove the dependency on the imgStatusTracker copy of this information, though.

Comment 1

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

Attachment: Track invalidation rects during decoding on Decoder, not imgStatusTracker

Here's the patch. I think this actually worked out pretty nicely, though adding more complexity to the recursive notification code in FinishedSomeDecoding is a bummer. It'd be nice to get rid of that entirely.

Comment 2

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

Here's a try job. There will likely be some stray oranges left over from bug 1084136. I'll submit another one once that bug is totally green, but I want to get a head start on debugging this patch:

https://tbpl.mozilla.org/?tree=Try&rev=01ed9f193d68

Comment 3

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

OK, bug 1084136 is green now. I'm still debugging the 'test_synchronized_animation.html' failure that the try job in comment 2 revealed, but I think it's worth submitting a new try job to get an idea of where this patch stands now:

https://tbpl.mozilla.org/?tree=Try&rev=b9f595e26e81

Comment 4

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

Attachment: Track invalidation rects during decoding on Decoder, not imgStatusTracker

So I investigated these failures extensively today, and my conclusions boiled
down to:

- I was sending way more invalidations than the old code did, primarily because
  of the old code's system of clearing Decoder::mInvalidRect every frame, which
  had the effect of us not sending invalidations in many of the places a naive
  reading of the code would give the impression that we do. Even when we did
  send invalidations from those places, we would've also sent the same
  invalidations from FinishedSomeDecoding, so I went ahead and cut down
  drastically on the number of places we send invalidations from.

- It's better to decide whether to send invalidations in RasterImage, not in
  imgStatusTracker, as we can be smarter about things there. I've removed all
  filtering of invalidations from imgStatusTracker and replaced it with a simple
  set of explicit filters in RasterImage::FinishedSomeDecoding. This should
  produce more or less the same results as the old code, with the exception that
  we explicitly prohibit some things that were sort-of-only-accidentally
  prohibited by the old code. For one thing, we now definitely only send
  *partial* invalidations during decoding, and let OnStopFrame/OnStopDecode
  trigger full invalidations - 'test_synchronized_animation.html' depended on
  this, and it seems to make sense to me. For another thing, we now explicitly
  refuse to send invalidations for animated frames after the first, leaving that
  work to RequestRefresh. Both of these things capture how the old code ended up
  working in practice, but now they are explicit rules that will always be
  consistently enforced.

These changes simplified the code and made it more understandable (which is
nice!) and they fixed the existing failures for me locally.

Comment 5

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

Here's a new try job. Linux and OS X debug only for now; I'll submit an 'all' job if this looks good:

https://tbpl.mozilla.org/?tree=Try&rev=d14cf4efff0f

Comment 6

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

Attachment: Track invalidation rects during decoding on Decoder, not imgStatusTracker

Whoops! I forgot to qref. This is the final version of the patch (modulo your review comments, of course). On the off chance you've already started reviewing, the only difference is some cleanup in 'imgStatusTracker.cpp'.

Comment 7

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

I resubmitted the try job.

https://tbpl.mozilla.org/?tree=Try&rev=dcfa4424c9da

Comment 8

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

Attachment: Track invalidation rects during decoding on Decoder, not imgStatusTracker

So I changed my mind about not sending full invalidation rects. I decided that it makes sense to do that, and I should correct 'test_synchronized_animation.html' instead; the fact that it failed if I did that seems to me to be a bug in the test. I've made those changes in this new version of the patch.

https://tbpl.mozilla.org/?tree=Try&rev=a090b7e7f4c1

Comment 9

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

Attachment: Track invalidation rects during decoding on Decoder, not imgStatusTracker

When debugging the |list-simple-1.html| failures in bug 1091229, I realized that the first instance of that failure in my patch stack actually occurred in this bug, not later in my patch stack as I had previously believed.

This led me to reexamine this patch, and I believe I may have discovered a possible explanation for the problem. I didn't go far enough with the change I mentioned in comment 8 to send full invalidation rects; it's necessary to ensure that they are sent even if the image has been decoded before, which my previous changes didn't ensure.

This new version of the patch should ensure that.

It should be possible to write this more cleanly after bug 1089046; right now it's a bit awkward, but since that's the very next bug in this series of patches, I think some temporary awkwardness is acceptable.

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=90e77882a3b2

Comment 10

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

Hmm. So somehow this change is causing a new set of test failures. =\ Will debug this in more detail when I have the time. I am particularly amazed that this is causing crashes, which really shouldn't be possible...

Comment 11

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

So I pulled today and tried to reproduce the 'browser_bug666317.js' failure again, and I can't. I could before pulling.

I don't know if these failures were actually from something unrelated that got fixed when I pulled, or if perhaps some timing just changed and now it'll be harder to reproduce locally. Here's another try job to try to answer that question:

https://tbpl.mozilla.org/?tree=Try&rev=b111d5e935e8

Comment 12

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

That try run looks good to me. So it seems like this patch is good to go, modulo review of course.

Comment 13

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

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1096bda40e76

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1096bda40e76