Closed Bug 1069652 Opened 10 years ago Closed 10 years ago

Only optimize imgFrame objects when unlocking them

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 1067288
Blocks: 1068409
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)
(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".
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.
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)
Attachment #8491875 - Attachment is obsolete: true
Attachment #8491875 - Flags: review?(mwu)
Trivial comment fix.
Attachment #8491904 - Flags: review?(mwu)
Attachment #8491902 - Attachment is obsolete: true
Attachment #8491902 - Flags: review?(mwu)
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+
(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.)
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)
Attachment #8491904 - Attachment is obsolete: true
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+
(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.
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blocks: 1070340
Blocks: 1070426
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
You need to log in before you can comment on or make changes to this bug.