1123976 - Discard decoded image surfaces more intelligently

Status: NEW

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

Description

[Andrew McCreight [:mccr8]]

From bug 1122704, it sounds like we hold alive some decoded image data even for tabs that are closed, for 60 seconds.  It would be good if we could communicate to imagelib somehow that the tabs are gone for real and so it is okay to throw out that image data immediately.

Surely there's some DOM code around somewhere that knows when this is happening, and we could stick some kind of callback into imagelib there.  Maybe this would be WindowDestroyedEvent::Run()?

(Seth also mentioned that it might be useful to know when a page enters the BF cache, to distinguish the case when an image just scrolls off the page, and I think there's also some DOM code that knows that, too.)

Comment 1

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

Thanks for filing. I think we really need to make this information accessible to ImageLib. We also want things like different synchronous decoding behavior for scrolling vs. navigating back to a page, and not having this info has bit us in that area too.

Comment 2

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

I've been toying with ideas for how to do this, and I have some rough ideas for how the API should look. 

Basically, right now we have this imperative API: DOM and layout code tells an image "lock yourself" (don't allow yourself to be discarded) and "unlock yourself" (you may now be discarded). Because of this, we can't really tell the difference between an image getting scrolled a few pixels off-screen, an image sitting in the bfcache, an image referenced only by JavaScript, and an image that is just sitting in the ImageLib network cache.

I think we need to replace the locking concept with an API that explicitly tracks all of these things:

- Is the image currently visible somewhere? (We'll consider "visible" to include being in the APZ rendered region; I don't think it's worth distinguishing those two things.)
- Is the image in a document that's currently visible somewhere?
- Is the image in a document that's in the bfcache?
- Does JavaScript have a live reference to the image?

All of these things may be true simultaneously of the same image, and indeed may be true in multiple different ways (e.g. via multiple documents) at the same time, so we actually need a counter for each of these things, replacing the single "lock count" that we have now.

With this information we should be able to develop policies to manage the image's surfaces and source data much more intelligently than we can do right now.

I don't have time to do this in the short term, unfortunately, though I definitely plan to get to this eventually. I think this might be a good candidate to be a mentored bug, though.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds reasonable to not conflate what we already know about these images and be able to separately query for these.

Comment 4

[Timothy Nikkel (:tnikkel)]

With bug 1125490 fixed we should discard when a document is destroyed. So this bug is basically fixed then, no?

Comment 5

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

(In reply to Timothy Nikkel (:tn) from comment #4)
> With bug 1125490 fixed we should discard when a document is destroyed. So
> this bug is basically fixed then, no?

Well, the problem in the title of the bug is basically fixed, but I do think we can still be smarter about this stuff. Let's rename the bug.

Comment 6

[Nicholas Nethercote [inactive]]

Is this just desktop, or does it affect B2G and Fennec as well?

Comment 7

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

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Is this just desktop, or does it affect B2G and Fennec as well?

It affects every platform in some sense, though this bug is really more about adding a mechanism than adopting policy. It's not unlikely that we'd have different policies for discarding for desktop vs. Android vs. B2G.

Comment 8

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

Adding bug 1135313 as a blocker, because it looks like it will make it simple to answer the question "is this image in a live document?", and being able to answer that is probably the first step in implementing the more detailed tracking discussed in this bug.

Comment 9

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

From bug 1164164 comment 10 (more context in that bug):

(In reply to David Flanagan [:djf] from comment #10)
> ...
> In general, Gallery depends on the ability to do image processing (thumbnail
> generation and image editing) using <canvas> and offscreen images. 
> Obviously gecko should decode images that are onscreen in preference to
> images that are in the document but offscreen.  But I think maybe it should
> give the highest priority to images that are not in the document at all.
> Those are the images where failure causes data loss rather than just visual
> issues.

In this bug's terms, what this translates to is that we should prioritize images which are only referenced by JavaScript.

Comment 10

[Eric Rahm [:erahm]]

Timothy, do you think this is good enough now, or should we keep it around?