Closed Bug 1137058 Opened 5 years ago Closed 5 years ago

image.mem.discardable broken in firefox 36

Categories

(Core :: ImageLib, defect)

36 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: her34her34, Assigned: seth)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

about:config set
image.mem.discardable;false
image.mem.decodeondraw;false

with this setting, firefox 35 and earlier do not have image flashing when switching between fully loaded tabs. after upgrading to firefox 36, images flash/redraw when switching tabs





Actual results:

switching between fully loaded tabs shows image flash, images pop in late. image place holders are visible before images fill place holder.


Expected results:

images should show instantly without delay
Is this similar to this bug I submitted about sites flickering that have background images when going back to their tabs: https://bugzilla.mozilla.org/show_bug.cgi?id=1137082
I'm not shure about the cause but can confirm that images redraw behaviour is annoying in Firefox 36 as it always flicker after 2 minutes idle when switching between tabs.
I can also confirm this issue which is really annoying and only started happening in Firefox 36

Some people call it image flashing or image flickering or pop in.

To be specific for what I see, I have multiple tabs that are all finished loading, so the issue isn't images downloading. When I switch tabs, the text for the tab appears first without any images and then a half second later I see the images appear. It really ruins browser experience.

If the issue is that firefox 36 is throwing away decoded images background tabs, then there should be an option to tell firefox don't throw away decoded images unless tab is closed.
Component: Untriaged → ImageLib
Product: Firefox → Core
(In reply to her34her34 from comment #0)
> image.mem.discardable;false
> with this setting, firefox 35 and earlier do not have image flashing when
> switching between fully loaded tabs. after upgrading to firefox 36, images
> flash/redraw when switching tabs

That's definitely a bug. This should still work.

> image.mem.decodeondraw;false

This pref is unrelated to this bug. Its name is actually pretty misleading; it will get renamed in an upcoming release.
(In reply to johnl15278 from comment #3)
> If the issue is that firefox 36 is throwing away decoded images background
> tabs, then there should be an option to tell firefox don't throw away
> decoded images unless tab is closed.

That is exactly what's happening. Actually, Firefox always did this, but we've become more aggressive about it recently. While this has significant benefits in terms of memory usage, it can lead to image flashing depending on how you tend to use the browser.

We're already aware of this, and we consider fixing it to be a high priority. (I had hoped to get it fixed in time for 36, but unfortunately there were some more serious issues that had to be solved first.) The work is being tracked in bug 1113855.

image.mem.discardable *should* be the option you want, but it sounds like it's broken in 36. We'll fix that in this bug.
Summary: image.mem.discardable/image.mem.decodeondraw broken in firefox 36 → image.mem.discardable broken in firefox 36
Actually, the severe image flashing that some users are experiencing is probably related to bug 1122643. If you download Firefox 37 (on Beta right now) and no longer see the problem, then bug 1122643 is the culprit.
her34her34, can you reproduce this on Firefox Beta? image.mem.discardable seems to work just fine here. It may be that this has nothing to do with image discarding, and is really just a symptom of bug 1122643.
Flags: needinfo?(her34her34)
I've checked 37b2 with same profile as 36 and can confirm that images flickering when redraw inactive tab issue isn't occuring @win7x64.
The problem still happens in Firefox 37.0b2. 

Observations:
*The problem is not as severe as happens in firefox 36. If you switch to a tab with 10 images, in firefox 36 you will see image 2-10 pop in which is very noticeable. In firefox 37.0b2 you will see image 9-10 pop in which some people might not notice.  
*the other bug 1122643 sounds unrelated to this issue. bug 1122643 mentions that images not yet loaded are black, in this issue the missing area shows the background of the website, meaning you see what is behind the image. bug 1122643 mentions that a reload of the tab is necessary to fix the problem, in this issue the image will always appear after a second or two. bug 1122643 mentions they see it on android version of firefox, I am using windows version
*In firefox 37.0b2 you will see image pop in while scrolling down page. I manually scrolled down each tab after first load to make sure pop in wasn't caused by lazy loading script from page, waited few minutes, then tested tabs. So pop in happens from switching tabs and from scrolling
*There is a slight delay sometimes when switching tabs in firefox 37.0b2. I wonder if this delay is masking image decoding, which makes the problem seem not as severe as firefox 36.
*Based on memory consumption, decoded images are being discarded. With firefox 35 set to image.mem.discardable;false, I loaded enough tabs to reach 2.0 gb ram after tabs finished loading and firefox cpu settle to 0%. Loading same tabs with firefox 35 set to image.mem.discardable;true and ram reaches 1.3gb. In firefox 37.0b2 ram reaches 1.3gb regardless of setting true or false.

*My conclusion from testing is that firefox 37.0b2 is still discarding decoded images but does a better job of masking it. image.mem.discardable;false doesn't work as it has in previous firefox versions <36.
Flags: needinfo?(her34her34)
Whiteboard: gfx-noted
(In reply to her34her34 from comment #9)
> *the other bug 1122643 sounds unrelated to this issue. bug 1122643 mentions
> that images not yet loaded are black, in this issue the missing area shows
> the background of the website, meaning you see what is behind the image. bug
> 1122643 mentions that a reload of the tab is necessary to fix the problem,
> in this issue the image will always appear after a second or two. bug
> 1122643 mentions they see it on android version of firefox, I am using
> windows version

We're pretty sure that bug does happen on Windows as well, and it may not necessarily require a reload for the images to reappear. (We added code that should make the images reappear automatically.) But if you see the background of the website and not a black area, then you're definitely not seeing bug 1122643. So it sounds like we can remove that from the list of possibilities.

> *In firefox 37.0b2 you will see image pop in while scrolling down page. I
> manually scrolled down each tab after first load to make sure pop in wasn't
> caused by lazy loading script from page, waited few minutes, then tested
> tabs. So pop in happens from switching tabs and from scrolling

That is very strange behavior! When you first load the page, we should decode all the images on it. They're decoded asynchronously, so it's possible to scroll the page faster than the images on it can be decoded, but that's nothing new, so if you're seeing a change that's definitely interesting.

We do throw away images that aren't visible after a while. We should still redecode them before you scroll them back onscreen, though. (And of course, image.mem.discardable should stop us from doing that.)

> *There is a slight delay sometimes when switching tabs in firefox 37.0b2. I
> wonder if this delay is masking image decoding, which makes the problem seem
> not as severe as firefox 36.

It's possible.

> *Based on memory consumption, decoded images are being discarded. With
> firefox 35 set to image.mem.discardable;false, I loaded enough tabs to reach
> 2.0 gb ram after tabs finished loading and firefox cpu settle to 0%. Loading
> same tabs with firefox 35 set to image.mem.discardable;true and ram reaches
> 1.3gb. In firefox 37.0b2 ram reaches 1.3gb regardless of setting true or
> false.

Yes, that is really strong evidence that the images are being discarded!

I'm not sure why you're seeing different behavior than I am here. I'll try again to reproduce the issue.
(In reply to Seth Fowler [:seth] from comment #10)
> I'm not sure why you're seeing different behavior than I am here. I'll try
> again to reproduce the issue.

OK, I did try again, and I was able to reproduce the issue. I must not have waited long enough for the images to be discarded last time. Sorry about that.

I've also found the problem, so we'll get this fixed ASAP.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here's the fix. (Note that we can't just call RasterImage::LockImage(), because
RasterImage::Init() runs off-main-thread sometimes.)
Attachment #8575727 - Flags: review?(tnikkel)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Comment on attachment 8575727 [details] [diff] [review]
Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

Refresh my memory, how did this work before?
(In reply to Timothy Nikkel (:tn) from comment #13)
> Comment on attachment 8575727 [details] [diff] [review]
> Increment RasterImage::mLockCount to ensure that non-discardable images
> don't eventually become unlocked
> 
> Refresh my memory, how did this work before?

In the past we wouldn't add ourselves to the list of discardable images that the DiscardTracker tracked if mDiscardable was false. Since the SurfaceCache owns all surfaces now, we can't use a similar approach, but it's enough to call SurfaceCache::LockImage() to get the same effect. The only problem is that the locking state of an image in the SurfaceCache is a boolean, not a counter, so if we end up calling SurfaceCache::UnlockImage() later (say because the image isn't visible anymore), the image will get unlocked anyway. Incrementing the lock count in RasterImage::Init() prevents us from ever doing that.

(In the long term it would be better to make the SurfaceCache's image locking state use a counter and not a boolean, but it wouldn't be upliftable and there are some other issues blocking that change.)
Attachment #8575727 - Flags: review?(tnikkel) → review+
Comment on attachment 8575727 [details] [diff] [review]
Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

Approval Request Comment
[Feature/regressing bug #]: Let's call it bug 1104622.
[User impact if declined]: The image.mem.discardable pref won't work.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: There is virtually zero risk here - this is a trivial one line patch that only affects a non-default configuration. It's a non-default configuration that is important to some users, though.
[String/UUID change made/needed]: None.
Attachment #8575727 - Flags: approval-mozilla-beta?
Attachment #8575727 - Flags: approval-mozilla-aurora?
(s/central/inbound, of course)
https://hg.mozilla.org/mozilla-central/rev/66a2384dde3c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Can we expect that this fix will be included in version 36.0.x or at least 37?
(In reply to JustOff from comment #19)
> Can we expect that this fix will be included in version 36.0.x

Definitely not, no.

> or at least 37?

I requested it in comment 15, but the feedback I'm getting so far leads me to think that it won't make it into 37, as we only have one beta left to go and the bar for entry is pretty high.

Based on what I know at this point, I'd expect the fix to end up in 38.
Comment on attachment 8575727 [details] [diff] [review]
Increment RasterImage::mLockCount to ensure that non-discardable images don't eventually become unlocked

I reviewed this change with Seth. There is very little risk of regression and the regression should be very visible. This fix also has potential benefit for the default case. I'm OK taking this in Beta 7. Beta+ Aurora+
Attachment #8575727 - Flags: approval-mozilla-beta?
Attachment #8575727 - Flags: approval-mozilla-beta+
Attachment #8575727 - Flags: approval-mozilla-aurora?
Attachment #8575727 - Flags: approval-mozilla-aurora+
(In reply to JustOff from comment #19)
> Can we expect that this fix will be included in version 36.0.x or at least
> 37?

So an update here - thanks to the kind approval from Lawrence in comment 21, this will make it into 37.
Thank you so much, Seth Fowler!
You need to log in before you can comment on or make changes to this bug.