The default bug view has changed. See this FAQ.

Images are blank momentarily on tab switch

RESOLVED FIXED in mozilla19

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Felipe, Assigned: jrmuizel)

Tracking

Trunk
mozilla19
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Assignee: nobody → jmuizelaar
Blocks: 792199
Summary: Images are being discarded too aggressively → Images are blank momentarily on tab switch
> 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.
(Assignee)

Comment 5

5 years ago
The patch that landed on bug 792199 was broken. It seems like we're currently not decoding on tab switch at all.
(Assignee)

Comment 6

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

Comment 7

5 years ago
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?
Attachment #670287 - Attachment is obsolete: true
Attachment #670534 - Flags: review?(joe)
Attachment #670534 - Flags: review?(dbaron)
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.
Attachment #670534 - Flags: review?(joe) → review+
(Assignee)

Comment 9

5 years ago
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.
Attachment #670547 - Flags: review?(dbaron)
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.
Attachment #670534 - Flags: review?(dbaron) → review?(dholbert)
Attachment #670547 - Flags: review?(dbaron) → review?(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
Attachment #670547 - Flags: review?(dholbert) → review+
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.
Attachment #670534 - Flags: review?(dholbert) → review+
(Assignee)

Comment 13

5 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 15

5 years ago
(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 :)
Flags: in-testsuite? → in-testsuite-

Comment 17

5 years ago
(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?
(Assignee)

Comment 18

5 years ago
I believe that this should be tracked by IMAGE_DECODE_ON_DRAW_LATENCY

Comment 19

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.