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

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

Trunk
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8350420 [details] [diff] [review]
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.
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+
Created attachment 8358848 [details] [diff] [review]
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.
Attachment #8358848 - Flags: review?(seth)
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 5

5 years ago
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)
(Assignee)

Comment 8

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.