Last Comment Bug 672578 - Stop decoding previously-discarded images when the containing tab loses focus
: Stop decoding previously-discarded images when the containing tab loses focus
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-19 11:25 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-03-30 00:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.59 KB, patch)
2011-07-19 14:16 PDT, Justin Lebar (not reading bugmail)
joe: review-
Details | Diff | Splinter Review
Patch v2 (5.55 KB, patch)
2011-07-21 09:39 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Part 1, v3 - Don't trigger a decode when FrameUpdate is called. (2.98 KB, patch)
2011-07-21 09:52 PDT, Justin Lebar (not reading bugmail)
joe: review+
Details | Diff | Splinter Review
Part 2, v3 - Discard a previously-decoded image when it's unlocked. (3.01 KB, patch)
2011-07-21 09:52 PDT, Justin Lebar (not reading bugmail)
joe: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-07-19 11:25:50 PDT
If you're decoding an image which has already been decoded, we should stop decoding it (and presumably throw it away) as soon as the tab which contains that image is not in focus.  This keeps us from spinning our wheels unnecessarily when you cycle through multiple tabs with large images which have been discarded.

I don't know how hard this would be, but I'll look into it...
Comment 1 Justin Lebar (not reading bugmail) 2011-07-19 14:16:25 PDT
Created attachment 546904 [details] [diff] [review]
Patch v1
Comment 2 Joe Drew (not getting mail) 2011-07-20 14:59:52 PDT
Comment on attachment 546904 [details] [diff] [review]
Patch v1

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

::: modules/libpr0n/src/RasterImage.cpp
@@ +2045,5 @@
> +RasterImage::TryCancelDecode()
> +{
> +  // Can't discard if CanForciblyDiscard() would return false (ignoring the
> +  // mDecoded check) or if we're animated.
> +  if (!mDiscardable || !mHasSourceData || mAnim)

I thought mAnim implies !mDiscardable; did this actually catch something in your tests?

@@ +2053,5 @@
> +         ("RasterImage[0x%p] canceling decode", this));
> +  ShutdownDecoder(eShutdownIntent_Interrupted);
> +
> +  // ForceDiscard will abort unless we claim that we've decoded.
> +  mDecoded = PR_TRUE;

I'd rather we changed ForciblyDiscard to not require that we're decoded (so it operates like rm -f), then use CanForciblyDiscard above and not have to set this here.

@@ +2678,5 @@
>      return NS_OK;
>  
> +  // If the image is unlocked and has been decoded before, try to cancel this
> +  // decode.  If that succeeds, then we're done here.
> +  if (image->mLockCount == 0 && image->mHasBeenDecoded &&

This really feels like the wrong place to put this check.

I'm willing to reconsider, but it'll need at the very least a comment saying "we can kick off a decode, then be unlocked (allowed to discard) because the user switched tabs. Early-exit the decode in that case."

Still, I think I'd rather this be in UnlockImage() when mLockCount gets to 0.
Comment 3 Justin Lebar (not reading bugmail) 2011-07-21 08:39:59 PDT
Doing this in UnlockImage() seems pretty reasonable to me.  But here's an unreasonable backtrace:

> RasterImage::RequestDecode (this=0x222b6f0)
> RasterImage::WantDecodedFrames (this=0x222b6f0)
> RasterImage::GetImgFrame (this=0x222b6f0, framenum=0)
> RasterImage::FrameUpdated (this=0x222b6f0, aFrameNum=0, aUpdatedRect=...)
> Decoder::FlushInvalidations (this=0x6306b30)
> Decoder::PostFrameStop (this=0x6306b30)
> Decoder::Finish (this=0x6306b30)
> RasterImage::ShutdownDecoder (this=0x222b6f0, aIntent=mozilla::imagelib::RasterImage::eShutdownIntent_Interrupted)
> RasterImage::UnlockImage (this=0x222b6f0)

UnlockImage calls ShutdownDecoder which calls FlushInvalidations which calls...RequestDecode.  I'll see how hard this is to fix.
Comment 4 Justin Lebar (not reading bugmail) 2011-07-21 09:38:29 PDT
The patch I'm about to upload changes RasterImage::FrameUpdated so it doesn't
trigger a decode of the given frame.  I'm not sure this is safe, but here's my
reasoning.

RasterImage::FrameUpdated is called only from Decoder::FlushInvalidations.
Decoder::FlushInvalidations is called from three places:

 * Decoder::PostFrameStop() -- I don't think we care to ensure that the frame
   gets decoded here, since we're indicating we're done with the frame.

 * RasterImage::SyncDecode() -- We call WriteToDecoder() immediately before
   FlushInvalidations(), so the whole thing should already be decoded.

 * imgDecodeWorker::Run() -- FlushInvalidations is called after the worker
   decodes a chunk.  If there's more to decode, the worker will re-post itself
   to the event loop after the FlushInvalidations call.
Comment 5 Justin Lebar (not reading bugmail) 2011-07-21 09:39:39 PDT
Created attachment 547418 [details] [diff] [review]
Patch v2
Comment 6 Joe Drew (not getting mail) 2011-07-21 09:41:32 PDT
Can you separate that into two patches?
Comment 7 Justin Lebar (not reading bugmail) 2011-07-21 09:52:18 PDT
Created attachment 547422 [details] [diff] [review]
Part 1, v3 - Don't trigger a decode when FrameUpdate is called.
Comment 8 Justin Lebar (not reading bugmail) 2011-07-21 09:52:55 PDT
Created attachment 547424 [details] [diff] [review]
Part 2, v3 - Discard a previously-decoded image when it's unlocked.
Comment 9 Joe Drew (not getting mail) 2011-07-21 10:19:35 PDT
Comment on attachment 547424 [details] [diff] [review]
Part 2, v3 - Discard a previously-decoded image when it's unlocked.

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

::: modules/libpr0n/src/RasterImage.cpp
@@ +2567,5 @@
>    // Decrement our lock count
>    mLockCount--;
>  
> +  // If we've decoded this image once before, we're currently decoding again,
> +  // and our lock count is now zero, try to cancel the decode.

Add a parenthetical:
// and our lock count is now zero (so nothing is forcing us to stay decoded), ...

Also, "try to cancel the decode, and throw away whatever we already had."

Note You need to log in before you can comment on or make changes to this bug.