Closed
Bug 672578
Opened 13 years ago
Closed 13 years ago
Stop decoding previously-discarded images when the containing tab loses focus
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
Details
Attachments
(2 files, 2 obsolete files)
2.98 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
3.01 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #546904 -
Flags: review?(joe)
Comment 2•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
Attachment #547418 -
Flags: review?(joe)
Assignee | ||
Updated•13 years ago
|
Attachment #546904 -
Attachment is obsolete: true
Comment 6•13 years ago
|
||
Can you separate that into two patches?
Assignee | ||
Comment 7•13 years ago
|
||
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #547424 -
Flags: review?(joe)
Assignee | ||
Updated•13 years ago
|
Attachment #547418 -
Attachment is obsolete: true
Attachment #547418 -
Flags: review?(joe)
Assignee | ||
Updated•13 years ago
|
Attachment #547422 -
Flags: review?(joe)
Updated•13 years ago
|
Attachment #547422 -
Flags: review?(joe) → review+
Comment 9•13 years ago
|
||
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•13 years ago
|
||
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/dc14633c3bf1 http://hg.mozilla.org/integration/mozilla-inbound/rev/c27db897f085
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/dc14633c3bf1 http://hg.mozilla.org/mozilla-central/rev/c27db897f085
Status: NEW → RESOLVED
Closed: 13 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.
Description
•