Last Comment Bug 799335 - Images are blank momentarily on tab switch
: Images are blank momentarily on tab switch
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla19
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks: 792199
  Show dependency treegraph
 
Reported: 2012-10-08 21:29 PDT by :Felipe Gomes (needinfo me!)
Modified: 2012-10-17 11:18 PDT (History)
15 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This seems to do the trick (1.95 KB, patch)
2012-10-11 00:59 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Fix things properly (2.46 KB, patch)
2012-10-11 14:14 PDT, Jeff Muizelaar [:jrmuizel]
joe: review+
dholbert: review+
Details | Diff | Splinter Review
Rename RequestDecode to StartDecoding (1.56 KB, patch)
2012-10-11 14:29 PDT, Jeff Muizelaar [:jrmuizel]
dholbert: review+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-10-08 21:29:31 PDT
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
Comment 1 Joe Drew (not getting mail) 2012-10-09 07:54:35 PDT
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.
Comment 2 Justin Lebar (not reading bugmail) 2012-10-09 08:02:09 PDT
> 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?
Comment 3 Joe Drew (not getting mail) 2012-10-09 08:06:17 PDT
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.
Comment 4 Andrew McCreight [:mccr8] 2012-10-09 16:20:24 PDT
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.
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-10-09 16:31:31 PDT
The patch that landed on bug 792199 was broken. It seems like we're currently not decoding on tab switch at all.
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-10-11 00:59:10 PDT
Created attachment 670287 [details] [diff] [review]
This seems to do the trick

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.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-10-11 14:14:16 PDT
Created attachment 670534 [details] [diff] [review]
Fix things properly

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 8 Joe Drew (not getting mail) 2012-10-11 14:23:16 PDT
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.
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-10-11 14:29:20 PDT
Created attachment 670547 [details] [diff] [review]
Rename RequestDecode to StartDecoding

Now that RequestDecode is calling StartDecoding it makes more sense for it's name to StartDecoding.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-10-11 14:34:32 PDT
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 11 Daniel Holbert [:dholbert] 2012-10-11 14:43:03 PDT
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 12 Daniel Holbert [:dholbert] 2012-10-11 15:17:12 PDT
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.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-10-13 16:39:45 PDT
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?
Comment 15 Jeff Muizelaar [:jrmuizel] 2012-10-14 14:10:46 PDT
(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.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-10-14 14:14:53 PDT
I figured it was unlikely, just wanted to ask :)
Comment 17 (dormant account) 2012-10-17 10:37:24 PDT
(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?
Comment 18 Jeff Muizelaar [:jrmuizel] 2012-10-17 11:14:30 PDT
I believe that this should be tracked by IMAGE_DECODE_ON_DRAW_LATENCY
Comment 19 (dormant account) 2012-10-17 11:18:16 PDT
(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.

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