Make image decoders be totally responsible for their own frame allocations

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
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
change:

- 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
  AllocateFrame.
Attachment #8543728 - Flags: review?(tnikkel)
(Assignee)

Comment 3

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

Updated

4 years ago
Attachment #8543728 - Attachment is obsolete: true
Attachment #8543728 - Flags: review?(tnikkel)
(Assignee)

Updated

4 years ago
Blocks: 1118694
(Assignee)

Comment 4

4 years ago
The oranges above seem to all be from other patches. Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=2ba4f2233740
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+
(Assignee)

Comment 6

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

Updated

4 years ago
Attachment #8543761 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Hopefully I'm not wrong about this but I think those failures are from bug 1118087, so I repushed this one:

https://hg.mozilla.org/integration/mozilla-inbound/rev/17fc30214d84
(Assignee)

Updated

4 years ago
Blocks: 1120141
(Assignee)

Updated

4 years ago
Blocks: 1120144
(Assignee)

Comment 12

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

Updated

4 years ago
Attachment #8546444 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Pushed again. Fingers crossed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ebefd0f7e3
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:

https://treeherder.mozilla.org/logviewer.html#?job_id=5320407&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=5321175&repo=mozilla-inbound
Flags: needinfo?(seth)
(Assignee)

Updated

4 years ago
No longer blocks: 1079627
(Assignee)

Updated

4 years ago
See Also: → 1171356
(Assignee)

Updated

4 years ago
Blocks: 1176124
Flags: needinfo?(seth)
(Assignee)

Comment 16

4 years ago
It's time to reland this.
(Assignee)

Updated

4 years ago
Attachment #8547120 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Comment 19

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

Updated

4 years ago
Attachment #8628603 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1181909
(Assignee)

Updated

4 years ago
Depends on: 1160819
(Assignee)

Updated

4 years ago
Depends on: 1182533
(Assignee)

Updated

4 years ago
Depends on: 1182545
(Assignee)

Comment 34

4 years ago
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
RasterImage::FinalizeDecoder().
(Assignee)

Updated

4 years ago
Attachment #8629022 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
I'd like to do one more -p all try job, but right now try is closed.
(Assignee)

Comment 37

4 years ago
Alright, that try job looks reasonably green. I'm almost daring to hope that this patch might stick this time.
https://hg.mozilla.org/mozilla-central/rev/aea2836ce9fe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

4 years ago
Depends on: 1182951

Updated

4 years ago
Blocks: 1183390
(Assignee)

Updated

4 years ago
Depends on: 1195745
Depends on: 1382685
You need to log in before you can comment on or make changes to this bug.