952354 - Draw raster image frames even if they were decoded with nonpremultiplied alpha if the frame is opaque

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now we can hit some horrible cases where the same image is used by both WebGL and an <img> tag or similar. WebGL often requests that we decode the image with non-premultiplied alpha, while when displaying content images in HTML and CSS we decode with premultiplied alpha. This can result in us discarding and redecoding the image twice *every time we redraw the page*, since we can only store one decoded version at a time and each time a different version is requested we throw the other one away.

The real fix here is to make it possible to store multiple decoded versions of the same image. (We need that for so many reasons!) However, we can handle many instances of mixed WebGL/HTML content inadvertently hitting this path by being willing to draw image frames with non-premultiplied alpha if the frame is opaque anyway. In this case premultiplied/non-premultiplied should make no difference.

We hit this issue in the real world with Goo Create, so it's not a theoretical concern.

Comment 1

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

Attachment: Draw non-premultiplied alpha raster image frames if the frame is opaque.

This patch makes us just go ahead and draw in the cases where we have a non-premultipled alpha but opaque frame.

This gets rid of the endless redecoding visible on Goo Create.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8350420 [details] [diff] [review]
Draw non-premultiplied alpha raster image frames if the frame is opaque.

Ok. We really should make sure we fix this properly though. Someone else will undoubtedly hit the same cliff.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Same idea for GetFrame and CopyFrame

If the decode with default flags is the first one to get started then we should do the same in GetFrame/CopyFrame so we don't have to do an extra decode.

Comment 4

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

Comment on attachment 8358848 [details] [diff] [review]
Same idea for GetFrame and CopyFrame

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

Even better. Thanks, Timothy.

Comment 5

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

I'd like to land these together. Timothy, have you run a try job on yours? (I have run one on my patch, which is posted in bug 943803.)

Comment 6

[Timothy Nikkel (:tnikkel)]

I have run try job, it was good.

I was actually going to suggest holding off on landing these because it seems like we have some bugs when webgl tries to use images as textures and the resultant decode/discard stuff because of premultiplied alpha. There is also bug 958758. Landing the patches in this bug would make those happen much more rarely, so we could have an initial period where we shake out any bugs and then land these.

Comment 7

[Timothy Nikkel (:tnikkel)]

I guess we've waited enough. Shall we land this?

Comment 8

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

How's this for an answer?

https://hg.mozilla.org/integration/mozilla-inbound/rev/bac8b8ba264e

Comment 9

[Timothy Nikkel (:tnikkel)]

And pushed my patch too
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f1efb21790

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bac8b8ba264e
https://hg.mozilla.org/mozilla-central/rev/11f1efb21790