Open Bug 1113855 Opened 9 years ago Updated 1 year ago

Reduce image flashing when switch tabs

Categories

(Core :: Graphics: ImageLib, defect)

defect

Tracking

()

Tracking Status
firefox35 --- unaffected
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 - affected

People

(Reporter: seth, Unassigned)

References

(Depends on 2 open bugs)

Details

Recently the amount of image flashing when switching tabs seems to have substantially increased. Although I haven't run mozregression to narrow it down, I'm pretty confident that the culprit is bug 1060869.

We used to synchronously decode images for up to 5ms on first paint, which would allow most reasonably sized images to decode completely. I suspect that bug 1060869 broke that feature, so all we get now are asynchronous decodes, causing flashing.

This problem is apparently particularly bad on http://wiki.mozilla.org - where it's visible on the tan around the top and left - and http://kuklaskorner.com - where it's visible on the side gradients.

We should get this fixed ASAP as it's a bad user experience.
Looks like you landed the patch in bug 1060869 are you familiar enough with this to take it and work on it?
Flags: needinfo?(seth)
(In reply to Benjamin Kerensa [:bkerensa] from comment #1)
> Looks like you landed the patch in bug 1060869 are you familiar enough with
> this to take it and work on it?

Yes. I'll take a look at this after the holidays.
Flags: needinfo?(seth)
So the cause in reality is almost certainly bug 845147, which made us not sync decode small images if they've been decoded before. Bug 1060869 exposed this because it made us store more images in the SurfaceCache, which is more aggressive about discarding images than the old DiscardTracker. (In particular the DiscardTracker would let a certain amount of images stick around even if they were eligible for discarding, while the SurfaceCache does not allow that.)

The problem here is that the heuristic in bug 845147 just isn't good enough to tell when we really want to sync decode small images and when we don't. Timothy had a very good proposal for fixing this more cleanly on IRC:

[17:47:59] tn:	we really need to be able to distinguish between tab switches and scrolling into view in imagelib
[17:48:15] tn:	it shouldnt be too hard, just need to plumb it through
[17:48:36] seth:	tn: i agree, that would be much better than any kind of heuristic here
[17:50:14] tn:	seth, maybe we could even just change the code in nsDocument.cpp that locks the images when the image locking state of the document changes (ie a tab switch)
[17:50:41] tn:	seth, make that call something else that indicates "yes i really do want some decoding even if its been decoded before"
[17:50:59] tn:	plumbing the scrolling stuff from layout is a longer path of code
Blocks: 845147
I'll be looking into getting some sort of improvement related to this into 37. It's hard to define what "fixed" means in this case, since this isn't really something that either works or doesn't - it's a matter of tweaking our policies to get better behavior in common situations. But we will try to improve this in 37, and hopefully continue to do better as we move forward.
After more discussion about this one, I think we should aim to fix this on trunk and then see what the patch looks like and whether we feel comfortable uplifting it. At this point I'm pretty confident that the right fix for this looks a lot like bug 1123976 (or at least a subset of bug 1123976) and I'm not totally sure how large the patch is going to be.
Depends on: 1123976
Given the last comment, I'd suggest we stop tracking for 37, and perhaps even 38.
Flags: needinfo?(lmandel)
(In reply to Milan Sreckovic [:milan] from comment #6)
> Given the last comment, I'd suggest we stop tracking for 37, and perhaps
> even 38.

Yep, that's what I think as well. I'll go ahead and update the tracking flags to suggest tracking start at 39. Lawrence, does that work for you?
Yes. wfm. Thanks for the assessment and setting a realistic timeline for a fix.
Flags: needinfo?(lmandel)
Seth, is this still something that will be a priority for 39? If so we would want to land changes in the next three weeks (early in aurora).   We may not need to track this and if so we might wontfix this for now.
Flags: needinfo?(seth)
(In reply to Liz Henry (:lizzard) from comment #10)
> Seth, is this still something that will be a priority for 39? If so we would
> want to land changes in the next three weeks (early in aurora).   We may not
> need to track this and if so we might wontfix this for now.

I don't think this is going to make it into 39. I'm still hoping to get it in 40.
Flags: needinfo?(seth)
OK I'll go ahead and wontfix it for 37 and 38 but will leave it tracked for now for 39. Thanks!
It's probably worth posting an update: this continues to be one of my top priorities, but I've been bogged down with regressions and working out some B2G-related issues. My goal is to address this in some fashion during this quarter, but at this point I don't expect it to make Firefox 40.
Depends on: 1259281
This seems to have gotten worse with version 45. When switching back to tabs I've been away for awhile, can see lot of black flashing. Here's what my website will look like for split second: http://i.imgur.com/6RmK4tW.png

Something changed between 44 and 45 to make the behavior worse.
Something definitely changed with Firefox 45.

With my Windows 10 PC (Radeon HD 7500, Core i7-3770 3.4 GHz, Firefox 45 64-bit) I notice very little, but there is one specific site where I do see what appears to be the entire page draw black (hard to be sure) for a few milliseconds before Firefox regurgitates the background image. Just as with Mark's example, the background draws black even though the background's solid colour is not actually black.

With my Windows XP PC (an older Radeon card, Core 2 Duo, Firefox 45 32-bit) it's very noticeable: after switching back to a tab that has not been seen for a few minutes (or any other tab from that site) there's a noticeable delay of maybe 100 ms where all the background images are missing, and they all reappear. There's no black flash in most cases: I see whatever solid fallback colour is defined to exist behind the background image.

I don't know whether the change is that tab switches are now fully asynchronous, or that there's more aggressive memory management that is causing Firefox to forget decoded images. My perception was that Mozilla were trying to combat memory bloat by discarding smaller decoded images where previously they had been held decoded in RAM (for a year or more, Firefox will forget really large decoded images once they scroll out of view and/or the tab is backgrounded), but I don't honestly know -- whatever it was, the change occurred in 45, and it can be quite noticeable and distracting.
This bug started a long time before 45, so there may be multiple issues, but it would be good to run mozregression between, say, October 1, 2015 and December 31, 2015 and see if there is a particular change that made this get worse.  We did limit the number of threads we use for decoding (bug 1219501) in 45, so perhaps that's where this would lead, but getting us a regression range would really help.
I noticed this bug recently as well (filed info here: https://bugzilla.mozilla.org/show_bug.cgi?id=1259498). This was not the case with 43 or 44, at least in situation I describe here. It is very annoying and it is not observed on other browsers (Safari, IE, Opera, Chrome).

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: seth.bugzilla → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.