Open Bug 1579173 Opened 5 years ago Updated 2 years ago

Image Cache: Images Sometimes Get Lost If Made Visible With Opacity (hitting surface cache limit)

Categories

(Core :: Graphics: ImageLib, defect, P2)

All
Unspecified
defect

Tracking

()

Tracking Status
firefox71 --- affected

People

(Reporter: Gankra, Unassigned)

Details

STR:

  • (For quick reproduction) Set image.cache.size to a very small value (so things fall out of it quickly)
  • Load up http://stuff.veekun.com/volatile/firefox-eviction-bug-demo/
  • Hold "right" on the keyboard
  • Find a permanent pink spot (can go back with left, but if you go below image 1 it will break so don't do that)

This is a page with hundreds of images, and we're selecting which one is visible with the arrow keys. Pink is the background-color behind the numbered images, so seeing pink means that we're trying to draw an image but the data isn't there.

Pink flickering is fine, that's just us reloading the image into the cache slower than we drew a frame. But if the pink color stays forever, that means we've "lost" the image and our caching is busted.

On my mac, non-wr, no changes to image cache size, I can also see this happen a lot if I browse some other tabs for a bit and come back. Usually if I press "right" after that I get a perma-pink (until I look at another tab and come back).

The original reporters saw this with and without webrender on linux.

The images are being inserted into the DOM dynamically based on loading events, but are otherwise "boring" from the DOM perspective. There's just a ton of opacity: 0 images on the page, and pressing right/left changes which one has a css class making it opacity: 1 -- that "should" work fine.

The original reporter also says this doesn't happen with visibility:hidden. Not knowing that part of the code well, I'm guessing that's because we do lots of async animation shenanigans with opacity, but not visibility? Presumably the async opacity machinery doesn't properly manage the image cache.

Happens on Nightly and Stable.

Possibly doesn't happen on windows (reporter had trouble reproducing there).

Has STR: --- → yes
Priority: -- → P2

Original Discussion can be found here (but my report should cover everything): https://twitter.com/Gankra_/status/1169676326086893570?s=20

Thanks for this report!

What happens here is that when the images are inserted into the dom they are considered visible (more on this later) and we decode them because of that. Because they are considered visible in an active tab they are "locked" which means we can't discard them. There are 800 images, each has ~2MB decoded data. The decoded image surface cache is usually 1GB, so after we reach the limit we refuse to decode any more images. The images that don't get decoded tend to be those with higher numbers because they get loaded later. We don't unlock any of these images.

Not looking at opacity when deciding image visibility was a decision I think I made a very long time ago, based on my intuition and not too much data. My thinking was that if an image was opacity 0 then the page was likely going to fade the image in, and decoding the image after the fade in started would look bad. Whereas for visibility hidden they would be toggled visible and it wouldn't look that bad if the decoded image didn't show up right away.

We could switch to consider opacity 0 images as not-visible. Not sure if/how many negative effects would come from that.

The page could use decoding=sync on the images (to avoid the pink flash but potentially introduce some jank) and visibility: hidden.

I'm open to other ideas.

I posted a patch to bug 1284651 to increase the surface cache limit from 1gb to 2gb on machines that can handle it. Of course it's a bandaid fix, you could always double the number of images to hit this bug again.

Summary: Image Cache: Images Sometimes Get Lost If Made Visible With Opacity → Image Cache: Images Sometimes Get Lost If Made Visible With Opacity (hitting surface cache limit)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.