672578 - Stop decoding previously-discarded images when the containing tab loses focus

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Justin Lebar (not reading bugmail)]

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)]

Attachment: Patch v1

Comment 2

[Joe Drew (not getting mail)]

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)]

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)]

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)]

Attachment: Patch v2

Comment 6

[Joe Drew (not getting mail)]

Can you separate that into two patches?

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v3 - Don't trigger a decode when FrameUpdate is called.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v3 - Discard a previously-decoded image when it's unlocked.

Comment 9

[Joe Drew (not getting mail)]

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."

Comment 10

[Justin Lebar (not reading bugmail)]

Inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/dc14633c3bf1
http://hg.mozilla.org/integration/mozilla-inbound/rev/c27db897f085

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/dc14633c3bf1
http://hg.mozilla.org/mozilla-central/rev/c27db897f085