Closed Bug 1117607 Opened 10 years ago Closed 9 years ago

Make image decoders be totally responsible for their own frame allocations

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

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

Details

Attachments

(1 file, 6 obsolete files)

Make decoders responsible for their own frame allocations
53.99 KB, patch
Details | Diff | Splinter Review
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 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)
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: 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+
(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: https://hg.mozilla.org/integration/mozilla-inbound/rev/17fc30214d84
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: 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)
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 remains.
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 RasterImage::FinalizeDecoder().
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.
https://hg.mozilla.org/mozilla-central/rev/aea2836ce9fe
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: