1102617 - Replace imgIContainer::FrameIsOpaque with imgIContainer::IsOpaque

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

So building the HAS_TRANSPARENCY patch (bug 1089880) made me realize:

- We determine whether an image is transparent using flags set in the image header in almost all cases.
- When animated images have some transparent frames, encoders generally mark all of the frames as transparent. (And generally, in real animations, most or all of the frames are *actually* transparent if any of them are transparent.)

So having per-frame transparency information doesn't buy us much. Let's move away from that and make HAS_TRANSPARENCY the single source of truth for whether an image is transparent. The plan is to replace imgIContainer::FrameIsOpaque with imigIContainer::IsOpaque, which takes no arguments and just answers based upon whether HAS_TRANSPARENCY has fired.

This cleans up our code slightly, but the real benefits are in patches I haven't landed yet, where routing aWhichFrame around just so that ApplyDecodeFlags can call FrameIsOpaque will cause a substantial increase in complexity if we don't make this change.

Comment 1

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

Attachment: Replace imgIContainer::FrameIsOpaque with imgIContainer::IsOpaque

Here's the patch.

Comment 2

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

Try job here:

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

Comment 3

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

Attachment: Replace imgIContainer::FrameIsOpaque with imgIContainer::IsOpaque

Whoops, forgot to remove the now unused aWhichFrame parameter to ApplyDecodeFlags. I've verified that this builds locally; since the parameter was unused, I don't think another try job is needed.

Comment 4

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

Attachment: Replace imgIContainer::FrameIsOpaque with imgIContainer::IsOpaque

So I had to rebase this, and while doing so I realized that I'm pretty sure we only check whether the image is animated in nsDisplayImage::GetOpaqueRegion because we previously had to pessimistically assume that there could be some transparent frame. Now that we're using IsOpaque instead of FrameIsOpaque, we actually don't need to worry about that, so I removed the check.

(To be sure I'm right about this, I'll push another try job.)

Comment 5

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

New try job:

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

Comment 6

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

Bug 1103328 fixes the failures that occurred as a result of the change I mentioned in comment 4. Here's a new try job, which should now be green:

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

Comment 7

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

Thanks for the review! Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff89c5a8393

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ff89c5a8393