Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla8

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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...
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 1

6 years ago
Created attachment 546904 [details] [diff] [review]
Patch v1
Attachment #546904 - Flags: review?(joe)
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.
Attachment #546904 - Flags: review?(joe) → review-
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 547418 [details] [diff] [review]
Patch v2
Attachment #547418 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Attachment #546904 - Attachment is obsolete: true
Can you separate that into two patches?
(Assignee)

Comment 7

6 years ago
Created attachment 547422 [details] [diff] [review]
Part 1, v3 - Don't trigger a decode when FrameUpdate is called.
(Assignee)

Comment 8

6 years ago
Created attachment 547424 [details] [diff] [review]
Part 2, v3 - Discard a previously-decoded image when it's unlocked.
Attachment #547424 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Attachment #547418 - Attachment is obsolete: true
Attachment #547418 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Attachment #547422 - Flags: review?(joe)
Attachment #547422 - Flags: review?(joe) → review+
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."
Attachment #547424 - Flags: review?(joe) → review+
(Assignee)

Comment 10

6 years ago
Inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/dc14633c3bf1
http://hg.mozilla.org/integration/mozilla-inbound/rev/c27db897f085
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/dc14633c3bf1
http://hg.mozilla.org/mozilla-central/rev/c27db897f085
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.