Closed Bug 1117607 Opened 6 years ago Closed 5 years ago

Make image decoders be totally responsible for their own frame allocations


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox42 --- fixed


(Reporter: seth, Assigned: seth)


(Depends on 1 open bug, Blocks 1 open bug)



(1 file, 6 obsolete files)

Right now, even after bug 1116733 and bug 1116735, the first frame of an image is still allocated on the main thread. There's no need to do that, though, and in fact I've determined that it will be problematic for my implementation strategy for bug 1079627. So let's make decoders *totally* responsible for their own frame allocation.

This has some nice side effects in terms of code simplification, as well.
Here's the patch. The key change is that decoders no longer assume that an
initial frame is allocated for them, and that they handle failure directly
(since the inversion of control for frame allocation is removed). In practice
that generally means just setting some internal state to indicate that they're
done and then returning early.

It turns out that we can simplify a lot of things in Decoder because of this

- We no longer need InitSharedDecoder at all, because the nsICODecoder's
  contained decoder no longer needs to grab the initial frame that was
  preallocated for the nsICODecoder. The contained decoder doesn't need to
  behave specially in any way now.

- We don't need NeedNewFrame or NeedsToFlushData, or the related data
  members and NewFrameData type, because that code was related to the inversion
  of control that no longer exists.

- We only needed to separate InternalAddFrame out from EnsureFrame because of
  the possibility of needing to replace the first frame. That possibility is
  gone now, so I've merged them and renamed the combined method to
  AllocateFrameInternal, since it essentially does the real work for
Attachment #8543728 - Flags: review?(tnikkel)
I realized a couple of minor things got left out when I split this patch out of the patch for bug 1079627. In particular:

- Decoder::FinishSharedDecoder is removed. nsICODecoder just calls
  FinishInternal on its contained decoder directly. In the long term I plan to
  refactor things until nsICODecoder can just call Decoder::Finish directly, and
  then it won't have to be a friend of Decoder at all. For now I think we're
  better off doing it this way, as I'd prefer to keep the amount of special
  machinery in Decoder to support nsICODecoder to an absolute minimum.

- A stronger assertion can now be used in RasterImage::OnAddedFrame.
Attachment #8543761 - Flags: review?(tnikkel)
Attachment #8543728 - Attachment is obsolete: true
Attachment #8543728 - Flags: review?(tnikkel)
Blocks: 1118694
The oranges above seem to all be from other patches. Here's a new try job:
Comment on attachment 8543761 [details] [diff] [review]
Make decoders responsible for their own frame allocations

What's the reason we still allocate a full bit depth frame for the first frame of a gif even after we know the actual depth?
Attachment #8543761 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #5)
> What's the reason we still allocate a full bit depth frame for the first
> frame of a gif even after we know the actual depth?

It's because there are assumptions in other parts of the code that we do that. We have to be able to draw the first frame no matter where we are in the playback of the animation - for example, JS can draw an animated GIF into a canvas, or we might need to show in the animated GIF in a print preview, and in those situations we need the first frame. If we didn't have it in RGBA/RGBX, we'd have to synchronously convert it at those points.

That doesn't mean it's a bad idea - those things never happen for most animated GIFs. But it's something that requires additional work, and we'd want to do that in another bug.
Attachment #8543761 - Attachment is obsolete: true
Hopefully I'm not wrong about this but I think those failures are from bug 1118087, so I repushed this one:
Blocks: 1120141
Blocks: 1120144
Alright, I think this is ready to push again. The fixes that were necessary were as follows:

- Some code in canvas somewhere fails if the first frame of PNGs is B8G8R8X8
  instead of B8G8R8A8. I filed bug 1120141 about fixing that; it's really a bug
  in canvas. For now I added a hack to force the first frame to be B8G8R8A8.

- Sync decoding an image that started decoding off-main-thread could cause
  RasterImage::DecodingComplete to run before RasterImage::OnAddedFrame, which
  would cause RasterImage to fail to mark the imgFrame optimizable, which in
  turn caused us to fail to take the "single pixel" code path sometimes in
  reftests, causing rendering differences. I added a hack to fix this for this
  patch in order to avoid some serious rebasing issues. In bug 1079627, which
  totally rewrites this code, I will implement a much cleaner fix.

- I disabled test_bug641198.html, which failures more frequently after this
  patch. There are already multiple open intermittent bugs about this test (bug
  651866, bug 1038109) and it was already marked "skip" on B2G and "disabled" on
  Android. I filed bug 1120144 about fixing and reenabling this test.
Attachment #8546444 - Attachment is obsolete: true
Pushed again. Fingers crossed:
Flags: needinfo?(seth)
Cross harder next time.

I'm not near a computer that can back things out, but this will need to either be fixed or backed out for these failures:
Flags: needinfo?(seth)
No longer blocks: 1079627
See Also: → 1171356
Blocks: 1176124
Flags: needinfo?(seth)
It's time to reland this.
Attachment #8547120 - Attachment is obsolete: true
OK, I've rebased the patch against current m-c. It passes a large number of
tests locally, so I think we're ready for a try job.
OK, a number of the failures on the previous try push were caused by a mistake
in the rebase. I didn't handle the downscale-during-decode case correctly.

This version of the patch is fixed. I'm going to push again, and we'll see what
Attachment #8628603 - Attachment is obsolete: true
Depends on: 1181909
Depends on: 1160819
Depends on: 1182533
Depends on: 1182545
Here's an updated version of the patch. Combined with the blocker bugs, this is
now 100% green on Linux.

Most of the issues were in the tests, and not in the patch. The only real change
that's been made in this version is that we now propagate state from
nsICODecoder's contained decoder to nsICODecoder itself regardless of whether
the contained decoder encountered an error. This means that nsICODecoder's state
should now reliably match the state of its contained decoder when we call
Attachment #8629022 - Attachment is obsolete: true
I'd like to do one more -p all try job, but right now try is closed.
Alright, that try job looks reasonably green. I'm almost daring to hope that this patch might stick this time.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1182951
Blocks: 1183390
Depends on: 1195745
Depends on: 1382685
You need to log in before you can comment on or make changes to this bug.