Closed
Bug 1298184
Opened 8 years ago
Closed 2 years ago
Clear the ImageLib surface cache, not the image cache, when a device reset occurs
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
INVALID
People
(Reporter: seth, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
10.33 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
gfxWindowsPlatform::HandleDeviceReset() may be called in a variety of situations where all surfaces may have become invalid (for example, due to a graphics driver cache) or where our graphics backend may have changed. In these situations, the surfaces we've decoded images into need to be regenerated. Current, we clear the image caches in this situation, but this does not achieve the goal we're trying to achieve, because the frame tree holds on to image objects directly. That means that for visible pages, clearing the image caches won't cause the images to be redecoded. And we pay a heavy price for this, because we're forced to load formerly cached images again *from the network*. Clearing ImageLib's surface cache is the right thing to do here. The surface cache stores, well, surfaces - the decoded versions of images. We can clear all of the surfaces without getting rid of the image data itself. This will work even for visible pages, automatically triggering a redecode when we try to draw the images again, and it won't force us to reload any data from the network. A nice side effect of making this change is that this lets us check the content backend on the main thread, before we begin decoding an image, safe in the knowledge that if there's a device reset, the resulting surface will get evicted from the surface cache and we'll never use it. This enables us to further off-main-thread all the things.
Reporter | ||
Comment 1•8 years ago
|
||
In order for this to work, DiscardAll() has to discard even locked surfaces. When the surface cache code was originally written, we discarded the original image data for some images (for example, images loaded from chrome:// URIs), so we couldn't rematerialize their surfaces. That's why there are references in the documentation to this situation, and why DiscardAll() doesn't discard surfaces for locked images. For various reasons, including downscale-during-decode support, we stopped discarding the original image data. All images now retain their image data, no matter what. That means that there are no cases where we can't rematerialize an image's surfaces. Given that change in behavior, it's reasonable to discard even locked surfaces in DiscardAll(). And we need that behavior to correctly recover from a device reset, since even locked surfaces are affected by a driver crash or backend change. (Indeed, this issue shows that we really couldn't have supported non-rematerializable surfaces in the first place.) This change also fixes an issue with the existing caller of SurfaceCacheUtils::DiscardAll(), which intends to discard all SVG-as-image surfaces when a theme change occurs. All such surfaces used to be unlocked, but at some point in the past we started to support locking them, which broke the theme change code. After this patch, it'll work correctly again.
Attachment #8785004 -
Flags: review?(dholbert)
Reporter | ||
Comment 2•8 years ago
|
||
This part switches us from clear the image cache (technically image *caches*, I suppose) to clearing the surface cache when we handle a device reset.
Attachment #8785005 -
Flags: review?(dvander)
Reporter | ||
Comment 3•8 years ago
|
||
Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59df009e96b
Reporter | ||
Comment 4•8 years ago
|
||
Whoops, a hunk from part 1 ended up in part 2. Reuploading.
Attachment #8785012 -
Flags: review?(dholbert)
Reporter | ||
Updated•8 years ago
|
Attachment #8785004 -
Attachment is obsolete: true
Attachment #8785005 -
Attachment is obsolete: true
Attachment #8785004 -
Flags: review?(dholbert)
Attachment #8785005 -
Flags: review?(dvander)
Reporter | ||
Comment 5•8 years ago
|
||
Attachment #8785013 -
Flags: review?(dvander)
Comment 6•8 years ago
|
||
Just to ease my uneasiness, what's the worst that can happen if we purge cache entries for an image that's locked, supposing we get a random DiscardAll() call? We'll just have to redecode a bunch of images the next time we paint them, right? (Whoever is keeping a cache-entry locked presumably shouldn't be holding raw pointers to its surfaces or anything like that, right? And if they are, they've got owning references, I epxect... so it's unlikely that there are any use-after-free bugs that'd be caused by disregarding cache entries' locked status here, right?)
Comment 7•8 years ago
|
||
Comment on attachment 8785012 [details] [diff] [review] (Part 1) - Make SurfaceCache::DiscardAll() discard even locked surfaces. Review of attachment 8785012 [details] [diff] [review]: ----------------------------------------------------------------- r=me, assuming this is pretty tame operation, per comment 6
Attachment #8785012 -
Flags: review?(dholbert) → review+
Attachment #8785013 -
Flags: review?(dvander) → review+
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #6) > Just to ease my uneasiness, what's the worst that can happen if we purge > cache entries for an image that's locked, supposing we get a random > DiscardAll() call? We'll just have to redecode a bunch of images the next > time we paint them, right? Yes, we'll just notice that the surfaces aren't there in RasterImage::LookupFrame() and we'll trigger a redecode. It's not a big deal. *However*: I realized after posting these patches that this will trigger an assert in debug builds for animated images, because we assert that animated images never need to be redecoded. We already detect and handle some analogous cases, so I could just post a third part to handle this in RasterImage, but I think a better idea is to just wait until I land a patch that's already in my patch queue that removes that assert. So let's not land this right away. I'll try to get that patch uploaded so I can make the bug block this one. > (Whoever is keeping a cache-entry locked presumably shouldn't be holding raw > pointers to its surfaces or anything like that, right? And if they are, > they've got owning references, I epxect... so it's unlikely that there are > any use-after-free bugs that'd be caused by disregarding cache entries' > locked status here, right?) Yeah, that's definitely true. Use-after-free is not a problem here.
Comment 9•8 years ago
|
||
Sounds good. Thanks!
Comment 10•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: seth.bugzilla → nobody
Flags: needinfo?(aosmond)
Updated•2 years ago
|
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(aosmond)
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•