Firefox is discarding images from background tabs too aggressively, including images that are scrolled into view. When I switch from one tab to another and wait a few seconds, switching back to the original tab the images are blank for a moment and then reappear. There is no memory pressure happening in the system, this happens even right after firefox startup with two tabs open (firefox.exe taking 240mb and there's 3gb free) Direct2D Enabled true DirectWrite Enabled true (6.1.7601.17789) GPU Accelerated Windows 1/1 AzureCanvasBackend direct2d AzureContentBackend none AzureFallbackCanvasBackend cairo

This is almost certainly a regression from bug 792199. Jeff, I guess we're not calling Draw() on those images synchronously. We should make that happen.

> Jeff, I guess we're not calling Draw() on those images synchronously. We should make that happen. Well, that's a snappiness trade-off, right?

We didn't make that decision consciously, though. Our intention was that visible images would be Draw()n synchronously, do their "up to" 5ms of decoding, and then go back to the event loop. Right now what seems to be happening is that we don't draw the images synchronously, because we're definitely flashing background colour first.

This issue is particularly dramatic for me on Facebook, where there are a bunch of small images all over the page that decode at various times.

The patch that landed on bug 792199 was broken. It seems like we're currently not decoding on tab switch at all.

There are two parts. 1. Always decode during StartDecoding even if we already have a Decoder 2. Have PrepareImage continue even if we don't have flags SYNC_DECODE_IMAGES. I'm not sure either of these is exactly what we want to do, but each seem reasonable.

This completely redoes the change to PrepareImage(). Now after calling StartDecoding() we recheck if we're complete and abort if we're not doing a SYNC_DECODE. Before this regression we would usually be done decoding the image because of the decoding we did when we Locked all of the images. dbaron, do the changes to nsCSSRendering seem sane?

Comment on attachment 670534 [details] [diff] [review] Fix things properly Review of attachment 670534 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +4019,4 @@ > // Make sure the image is actually decoding > mImage->StartDecoding(); > > + // check again to see if we finished Capitalize and say what and when we would have finished.

Now that RequestDecode is calling StartDecoding it makes more sense for it's name to StartDecoding.

Comment on attachment 670534 [details] [diff] [review] Fix things properly Not sure why this review request was to me, but handing it off to dholbert.

Comment on attachment 670547 [details] [diff] [review] Rename RequestDecode to StartDecoding >+++ b/layout/style/nsStyleStruct.h >@@ -227,7 +227,7 @@ struct nsStyleImage { > /** > * Requests a decode on the image. > */ >- nsresult RequestDecode() const; >+ nsresult StartDecoding() const; Looks like we should update the comment there, too. r=me

Comment on attachment 670534 [details] [diff] [review] Fix things properly >--- a/image/src/RasterImage.cpp >+++ b/image/src/RasterImage.cpp >@@ -2554,8 +2554,13 @@ RasterImage::RequestDecodeCore(RequestDecodeType aDecodeType) > // If we've already got a full decoder running, we have nothing to do >- if (mDecoder && !mDecoder->IsSizeDecode()) >+ if (mDecoder && !mDecoder->IsSizeDecode()) { >+ if (!mDecoded && !mInDecoder && mHasSourceData && aDecodeType == SOMEWHAT_SYNCHRONOUS) { >+ SAMPLE_LABEL_PRINTF("RasterImage", "DecodeABitOf", "%s", GetURIString()); >+ DecodeWorker::Singleton()->DecodeABitOf(this); >+ } > return NS_OK; Looks like the comment here might need an update, too? "we have nothing to do" used to be true (we used to just early-return), but apparently we now sometimes have things to do there, under some conditions? Aside from that: I'm assuming the r?d[baron|holbert] was for the /layout chunk -- I'll assume that joe sanity-checked code part of the RasterImage chunk above. For the /layout chunk: that looks like we'll now be making PrepareImage a no-op for empty images (makes sense), and we'll be allowing for the fact that our StartDecoding() call might fully decode the image (makes sense). So: r=me, modulo the "we have nothing to do" comment-tweak.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea416c21d17 https://hg.mozilla.org/integration/mozilla-inbound/rev/b8970531e93e

https://hg.mozilla.org/mozilla-central/rev/b8970531e93e https://hg.mozilla.org/mozilla-central/rev/0ea416c21d17 I'm going to go ahead and ask - is it possible to test this?

(In reply to Ryan VanderMeulen from comment #14) > https://hg.mozilla.org/mozilla-central/rev/b8970531e93e > https://hg.mozilla.org/mozilla-central/rev/0ea416c21d17 > > I'm going to go ahead and ask - is it possible to test this? Not very easily. There's still the possibility of intentional flashes, they're just less likely now.

I figured it was unlikely, just wanted to ask :)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #15) > (In reply to Ryan VanderMeulen from comment #14) > > https://hg.mozilla.org/mozilla-central/rev/b8970531e93e > > https://hg.mozilla.org/mozilla-central/rev/0ea416c21d17 > > > > I'm going to go ahead and ask - is it possible to test this? > > Not very easily. There's still the possibility of intentional flashes, > they're just less likely now. Can we track these via telemetry?

I believe that this should be tracked by IMAGE_DECODE_ON_DRAW_LATENCY

(In reply to Jeff Muizelaar [:jrmuizel] from comment #18) > I believe that this should be tracked by IMAGE_DECODE_ON_DRAW_LATENCY Thanks, 18/19 trend in http://is.gd/GB4tF3 seems to confirm that.