1131446 - mFrameHasNoAlpha is mostly pointless

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: mFrameHasNoAlpha is mostly pointless

Starting with bug 391583 we detect whether a png image has any transparency during decode. I don't think this is worth doing anymore. We choose the format of image based on the png header, that should be sufficient for the performance improvement we desire from opaque images. This gets us faster image decoding.

Comment 1

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

Comment on attachment 8561890 [details] [diff] [review]
mFrameHasNoAlpha is mostly pointless

Review of attachment 8561890 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

The GIF decoder does this as well. Can we get rid of it there too?

Comment 2

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

As an aside, I will note that it's not unusual for crummy encoders to give PNGs an alpha channel even though they have no transparency. (Or possibly this is a poor choice on the part of the person doing the encoding; not sure where the process is breaking down.)

But I'm not sure that's an argument for keeping mFrameHasNoAlpha around. We already decide whether to treat the image as opaque in the layer system based upon the information we collect from the header - i.e., whether PostHasTransparency() has been called. So mFrameHasNoAlpha only affects the blending we do when we draw the frame.

It's unclear to me whether the cheaper blending alone justifies keeping it, especially since content authors can avoid any penalty by simply using a sane PNG encoder and sane encoding settings.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Seth Fowler [:seth] from comment #1)
> Comment on attachment 8561890 [details] [diff] [review]
> mFrameHasNoAlpha is mostly pointless
> 
> Review of attachment 8561890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> The GIF decoder does this as well. Can we get rid of it there too?

Getting rid of the GIF decoder one looks like it will require a lot more nuance, but should be possible.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b24f9a3b47a6