Closed Bug 952354 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8350420 - Flags: review?(jmuizelaar)
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.
Attachment #8350420 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8358848 - Flags: review?(seth)
Comment on attachment 8358848 [details] [diff] [review]
Same idea for GetFrame and CopyFrame

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

Even better. Thanks, Timothy.
Attachment #8358848 - Flags: review?(seth) → review+
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.)
Flags: needinfo?(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.
Flags: needinfo?(tnikkel)
I guess we've waited enough. Shall we land this?
Flags: needinfo?(seth)
How's this for an answer?

https://hg.mozilla.org/integration/mozilla-inbound/rev/bac8b8ba264e
Flags: needinfo?(seth)
https://hg.mozilla.org/mozilla-central/rev/bac8b8ba264e
https://hg.mozilla.org/mozilla-central/rev/11f1efb21790
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.