If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Only optimize imgFrame objects when unlocking them

RESOLVED FIXED in mozilla35

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
It's not safe to call imgFrame::Optimize() while the imgFrame is locked, because there may be multiple RawAccessRef's owned by different code out there. If the owner of one RawAccessRef calls Optimize(), it will cause the data surface for that imgFrame to disappear, which is problematic for the owners of other RawAccessRef's which still need to access that data.

This seems to be causing several intermittent oranges right now. We need to fix this by only calling Optimize() when the imgFrame transitions to the unlocked state. An additional advantage of this is that the notions of imgFrame discarding and imgFrame optimization can be unified.
(Assignee)

Updated

3 years ago
Blocks: 1067288
(Assignee)

Updated

3 years ago
Blocks: 1068409
(Assignee)

Comment 1

3 years ago
Created attachment 8491875 [details] [diff] [review]
Optimize imgFrames only when unlocking

Here's the patch. This patch does two things:

(1) It makes imgFrame::Optimize() private. Callers who would've previously used Optimize() should now call imgFrame::SetOptimizable() to request optimization when the imgFrame gets unlocked.

(2) It gets rid of imgFrame::SetDiscardable() and the notion of imgFrame discardability. This is because we already set all non-animated imgFrame objects as discardable, and we also already called Optimize() on them, so the set of discardable imgFrame's and the set of optimizable imgFrame's were the same. Optimize() does strictly more than imgFrame discarding did. Both imgFrame discarding and Optimize() release mImageSurface, but Optimize() also does things like collapse single-color images to a single color value.

Callers are updated appropriately. Note that we don't check RasterImage::CanForceDiscardAndRedecode() before calling imgFrame::SetOptimizable(). This essentially keeps the status quo, because we didn't check that before calling imgFrame::Optimize(). This *does* mean that occasionally we need to read back from the GPU when creating a RawAccessRef for an imgFrame that already got optimized. We already do that, unfortunately. That will go away very soon once we do scaling via downscale-during-decode, though.
Attachment #8491875 - Flags: review?(mwu)
(Assignee)

Comment 2

3 years ago
(In reply to Seth Fowler [:seth] from comment #1)
> Callers are updated appropriately. Note that we don't check
> RasterImage::CanForceDiscardAndRedecode() before calling
> imgFrame::SetOptimizable(). This essentially keeps the status quo, because
> we didn't check that before calling imgFrame::Optimize(). This *does* mean
> that occasionally we need to read back from the GPU when creating a
> RawAccessRef for an imgFrame that already got optimized. We already do that,
> unfortunately. That will go away very soon once we do scaling via
> downscale-during-decode, though.

I should clarify that further: if we want to support both optimizing imgFrame's and HQ scaling them, we can really only get the image data for HQ scaling one of two ways: readback from the GPU, or redecoding the image. Currently we're taking the readback route; that's the status quo. (And we virtually always avoid it in practice for images that are only displayed in a single size on the screen.) Once we have downscale-during-decode, we'll take the route of redecoding the image. We'll need to check whether we can redecode, of course, but we'd do the checks elsewhere.

Hopefully my rambling has not made this more confusing than necessary! I should've just stuck with the simpler message of "we're maintaining the status quo here".
(Assignee)

Comment 3

3 years ago
Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=c4e8dc4f5857

Comment 4

3 years ago
Comment on attachment 8491875 [details] [diff] [review]
Optimize imgFrames only when unlocking

Review of attachment 8491875 [details] [diff] [review]:
-----------------------------------------------------------------

Making Optimize() private is nice - I was thinking about doing something similar a while ago.

::: image/src/imgFrame.cpp
@@ +730,5 @@
> +  // which don't have surfaces.)
> +  if (mLockCount == 1 && !mPalettedImageData) {
> +    // Convert the data surface to a GPU surface or a single color if possible.
> +    // If successful, this will also release mImageSurface.
> +    Optimize();

Optimize() isn't a complete replacement for the old discarding code. The reason is that there's a chance that mImageSurface isn't released, and mImageSurface holds a reference to the volatile buffer.
(Assignee)

Comment 5

3 years ago
Created attachment 8491902 [details] [diff] [review]
Optimize imgFrames only when unlocking

That's a good point; we may not free mImageSurface in Optimize(), especially on Android. This new version of the patch unconditionally frees mImageSurface at the end of Optimize() if we're on Android. It'd be nice if we could eventually do this on all platforms.
Attachment #8491902 - Flags: review?(mwu)
(Assignee)

Updated

3 years ago
Attachment #8491875 - Attachment is obsolete: true
Attachment #8491875 - Flags: review?(mwu)
(Assignee)

Comment 6

3 years ago
Created attachment 8491904 [details] [diff] [review]
Optimize imgFrames only when unlocking

Trivial comment fix.
Attachment #8491904 - Flags: review?(mwu)
(Assignee)

Updated

3 years ago
Attachment #8491902 - Attachment is obsolete: true
Attachment #8491902 - Flags: review?(mwu)

Comment 7

3 years ago
Comment on attachment 8491904 [details] [diff] [review]
Optimize imgFrames only when unlocking

Review of attachment 8491904 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: image/src/RasterImage.cpp
@@ -252,5 @@
>        image->UnlockImage();
>      }
>  
> -    if (DiscardingEnabled() && dstFrame) {
> -      dstFrame->SetDiscardable();

Hmm this reminds me.. this means we may let the OS discard images that aren't initialized with INIT_FLAG_DISCARDABLE. That might not be so bad since they're just resource:// and chrome:// URIs which we can reload. We also ignore the global discarding pref, which doesn't bother me too much either.

::: image/src/imgFrame.cpp
@@ +738,5 @@
> +  if (mLockCount == 1 && !mPalettedImageData) {
> +    // Convert the data surface to a GPU surface or a single color if possible.
> +    // This will also release mImageSurface if possible.
> +    Optimize();
> +    

nit: trailing whitespace.
Attachment #8491904 - Flags: review?(mwu) → review+
(Assignee)

Comment 8

3 years ago
(In reply to Michael Wu [:mwu] from comment #7)
> Hmm this reminds me.. this means we may let the OS discard images that
> aren't initialized with INIT_FLAG_DISCARDABLE. That might not be so bad
> since they're just resource:// and chrome:// URIs which we can reload. We
> also ignore the global discarding pref, which doesn't bother me too much
> either.

Hmm.. I'm having second thoughts about this. I agree it's not such a problem, but maybe it's better to not make that change in this bug. We prevent that by other means in bug 1060869, but I can't guarantee that'll land immediately.

I think what I may do is restore imgFrame::SetDiscardable() for now, just for that case. I'll remove it in bug 1060869 instead.

(I'll keep the other changes, though. We'll just check that flag at the end of Optimize() before actually discarding mImageFrame.)
(Assignee)

Comment 9

3 years ago
Created attachment 8491927 [details] [diff] [review]
Optimize imgFrames only when unlocking

Alright, this implements the change I mentioned in comment 8.

I'll rip SetDiscardable() right back out in bug 1060869, because in that bug SurfaceCache gains the ability to prevent imgFrame's from being freed by the OS if the image they're associated with is locked. We shouldn't need SetDiscardable() after that.

Sorry for so many revisions so quickly; this is a top crash on Nightly, so I want to get it fixed ASAP. (But still fixed correctly!)
Attachment #8491927 - Flags: review?(mwu)
(Assignee)

Updated

3 years ago
Attachment #8491904 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=190fd8fac499

Comment 11

3 years ago
Comment on attachment 8491927 [details] [diff] [review]
Optimize imgFrames only when unlocking

Review of attachment 8491927 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +257,1 @@
>        dstFrame->SetDiscardable();

Not going to check to see if global discarding is enabled? (fine with me - just making sure this is intentional)
Attachment #8491927 - Flags: review?(mwu) → review+
(Assignee)

Comment 12

3 years ago
(In reply to Michael Wu [:mwu] from comment #11)
> Not going to check to see if global discarding is enabled? (fine with me -
> just making sure this is intentional)

Yup, it's intentional. It doesn't make sense to disable discarding for HQ scaled frames; they're inherently short-lived. In bug 1060200, which will land as soon as I fix these regressions, we start storing them in the surface cache also, which can discard them at any time.
(Assignee)

Comment 13

3 years ago
Try looks green, so I went ahead and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b14b05cdb75
https://hg.mozilla.org/mozilla-central/rev/4b14b05cdb75
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Updated

3 years ago
Blocks: 1070340
(Assignee)

Updated

3 years ago
Blocks: 1070426
(Assignee)

Comment 15

3 years ago
I pushed a quick followup to fix the comment added in this bug to imgFrame::Optimize(), which was totally wrong, as I realized when discussing this stuff with Timothy.

https://hg.mozilla.org/integration/mozilla-inbound/rev/caeb5176aeca
https://hg.mozilla.org/mozilla-central/rev/caeb5176aeca
You need to log in before you can comment on or make changes to this bug.