Discard decoded image surfaces more intelligently

NEW
Unassigned

Status

()

defect
P3
normal
5 years ago
5 months ago

People

(Reporter: mccr8, Unassigned, Mentored, NeedInfo)

Tracking

(Depends on 1 bug, Blocks 4 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2][gfx-noted])

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.)
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.
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.
Mentor: seth
Sounds reasonable to not conflate what we already know about these images and be able to separately query for these.
With bug 1125490 fixed we should discard when a document is destroyed. So this bug is basically fixed then, no?
Whiteboard: [MemShrink] → [MemShrink][gfx-noted]
(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.
Summary: Discard decoded image surfaces more aggressively for closed tabs → Discard decoded image surfaces more intelligently
Is this just desktop, or does it affect B2G and Fennec as well?
Whiteboard: [MemShrink][gfx-noted] → [MemShrink:P1][gfx-noted]
(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.
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.
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.
Blocks: 1163367
Blocks: 1213245
Depends on: 1259281
Whiteboard: [MemShrink:P1][gfx-noted] → [MemShrink:P1][gfx-noted] [platform-rel-Intel]
platform-rel: --- → ?
platform-rel: ? → ---
Whiteboard: [MemShrink:P1][gfx-noted] [platform-rel-Intel] → [MemShrink:P1][gfx-noted]
Timothy, do you think this is good enough now, or should we keep it around?
Flags: needinfo?(tnikkel)
Whiteboard: [MemShrink:P1][gfx-noted] → [MemShrink:P2][gfx-noted]
You need to log in before you can comment on or make changes to this bug.