Closed Bug 304561 Opened 19 years ago Closed 19 years ago

Firefox unsuitable for browsing high-res image galleries due to preview on tab icon

Categories

(Firefox :: Tabbed Browser, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: slaughter, Assigned: vlad)

References

(Depends on 1 open bug, )

Details

(Keywords: fixed1.8, memory-leak, perf, Whiteboard: [testing needed - may not be a blocker now])

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050812 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050812 Firefox/1.0+

Firefox does not appear to cache the resized image used as the favicon displayed
in the address bar and tab of tabs showing high-resolution images. The CPU
overhead of performing the scaling on every redraw is very noticeable, and makes
Firefox impractical for viewing galleries of photographs taken from digital
cameras by opening them in tabs, for example.

Reproducible: Always

Steps to Reproduce:
1. Open the image from the URL on multiple tabs, say about 10.
2. Switch between the tabs, open and close menus, resize the browser window, etc.
3.

Actual Results:  
Browser practically unusable on my dual processor Athlon MP 2800+ with 1GB RAM
and a Radeon 9800XT running latest Catalyst drivers under Windows XP SP2. It's
no longer a top-of-the-range system, but it's faster than most users' machines.

Expected Results:  
No browser slowdown. The 16x16 image could be cached. Experiments show that this
seems to occur for some images in different formats or with different colour depths.


I understand that the resized-image-caching thing is a core issue rather than a
Firefox problem per se, as it also affects scrolling on pages that dynamically
resize images. However, using images as thumbnail icons on tabs and in the
address bar is a change for Firefox 1.5 which makes the browser unusable for
viewing high-res images from a Photo CD, digital camera or online gallery, and
there's currently no way to control it. I don't believe those situations are
atypical.

In raising the bug on this component I wish to encourage consideration of
whether a general fix is possible for Fx1.5, whether a work-around for the
specific case of favicons is conceivable, or whether it's worth disabling the
preview icons by default until the problems can be addressed (by the Cairo
stuff?) given that the feature wasn't present in 1.0.
This one is evil evil evil.   Looks like we need to find the magic size cap for
this feature.  Normal images don't leak, these do, even one at a time.

http://www.nasa.gov/images/content/125061main_image_feature_385_ys_full.jpg
http://www.nasa.gov/images/content/124923main_image_feature_383_ys_full.jpg
http://www.nasa.gov/images/content/123817main_image_feature_376_ys_full.jpg
http://www.nasa.gov/images/content/124995main_image_feature_384_ys_full.jpg
http://www.nasa.gov/images/content/123688main_image_feature_375_ys_full.jpg

Vlad says this is known, we really just need to clamp the size of the image (to
what, we haven't tracked down yet.  My guess is that its only on images that are
resized in content as well, but that's anecdotally based.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b4+
related to or dupe of bug 302858 ?
Keywords: mlk, perf
Assignee: nobody → mconnor
I can't reproduce this, but it may be something particular to ATI's GDI
implementation.  With a nvidia 6800GT, on a 2GHz Sempron, 1GB RAM, I can load up
all of the above images in tabs, and quickly switch between them.  Interesting
to note is that when I first switch to another tab, memory usage jumps
drastically, then falls back down (e.g. 23mb -> 50+mb on a switch, then falls
back down to 23mb).

Something strange is going on, though.. I opened up the images in tabs, and
clicked on the 375 image's tab (it's one of the bigger ones).
- Memory usage is 23.5MB.  I just let firefox sit there, not touching the mouse
or anything.
- At exactly the 1 minute mark, memory usage started climbing rapidly to 101MB,
and it's staying there now.  An additional minute or two didn't cause an
increase in memory usage.
- If I move the mouse at this point, memory usage immediately drops to 62MB.
- If I just let it sit, again at the 1 minute mark, memory usage jumps to 101MB.
- Move the mouse again, drops down to 44mb (I moused over some of the other
image tabs).
- If I click on another tab, especially an about:blank one, memory usage drops
back down to 23.5MB.

So, there's no memory leak here per se, but we do have some very bizzare
allocations.  While loading I can understand the memory spike; we have to keep
the entire image in RAM until we can convert it to a windows device bitmap
(which won't show up in our memory usage, I don't think).  However, the 60
second memory usage spikes are very strange.

We may want a pref to disable image favicon-ing, if we don't have one already.
Note that something happens at the 60 second mark even if the large-image tab
isn't at the front.  Is windows evicting the image data back into application
space after 60 seconds?
Note that it's entirely possible that ATI's drivers aren't loading these images
into video ram, so that drawing them involves transferring the image across the
bus every single time.. which is why it could be sloooooooow.
mconnor: were you able to reproduce this yourself? if so on which machine and
which build?

I'm not having any luck, the most I see is what Vlad sees, but I never hit a
slowdown.

Can we get more exact details about how the browser is unusable?  Does it hang
before it resizes? Do you hit a CPU spike, or experience a memory leak?
What I see is consistent with Vlad's hypothesis in comment 5: the full image
being transferred to the video card each time it's rendered (only if it's being
scaled?). Nearly 50% of the CPU time (a single processor in the dual-CPU system)
is spent in kernel time for over a second (with six or more large image tabs),
even when doing something as trivial as causing a tab to be redrawn (by hovering
over it with the mouse to get the orange highlight, for example). As for memory
usage, loading the image causes a jump from 43 to 58MB, but most of that
disappears afterwards. It's not the memory usage that bothers me.

I can confirm that I don't have a problem on some nVidia-based systems that I
tried: a dual Xeon at work, or even a laptop with a Geforce mobile chipset. Due
to the bitmap cache, it doesn't even affect terminal services. I've been running
Catalyst 5.7 on the ATI machine (and the problem has existed in previous
versions), so I'll upgrade now that 5.8 is out.

As I mentioned, the slow resizing problem also affects images that are resized
in-page, and Internet Explorer also suffers from that problem, but favicons make
the problem worse on Firefox. A pref would help me, but it's not an ideal
solution for other ATI users, if that's the root cause.

Of course, I'm willing to provide any specific information or perform any
tests/experiments that you ask of me. For what it's worth, there's some evidence
that other users are having this problem, notably on other platforms (Mac):
http://forums.mozillazine.org/viewtopic.php?p=1565017
Odd, I can't reproduce this now, at least not the massive leaks I was seeing.

There's some spiking when switching tabs, but I'm not sure whether that's only
the tab icon or showing the image itself.  Its intermittent and subtle, but this
is a dual Xeon system.
I can no longer reproduce anything except a bit of background CPU stuff, but I
don't have a good machine to test perf stuff with.
Whiteboard: [testing needed - may not be a blocker now]
James - so you are saying you don't have a memory leak, but the browser is
considerably slower? These would be two separate issues.

Can you give us specific steps to reproduce, like which images you have open, if
they're scaled or full size, how many of them are open when you see this, etc. 
Basically give us the step by step so that we can do exactly what you're doing.

Also, what version of ff are you using?  I couldn't reproduce any slowdown on
1.0.6 or on the latest nightly.
majken, Peter's bug 302858 covers the memory leak, I think. This bug relates to
performance, and seems to only affect machines with ATI graphics cards. My
problems are described in the opening comment.

To reproduce, open the link from the URL in ten background tabs. After the tabs
have loaded, move the mouse cursor horizontally from left to right over the tab
bar. Simply redrawing the tabs to show the orange mouseover highlight causes
the CPU utilisation shown in the attached picture. If the browser is minimised
and restored, otherwise resized, or a window is dragged over the top, etc,
similar CPU usage occurs and the window will take about five seconds to redraw.
The tabs themselves aren't even showing and there is only one image involved.

This has been problematic since the images-as-favicon change was made, but I'm
currently using a build taken from the latest-mozilla1.8 directory earlier
today: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050821
Firefox/1.0+

Since Vlad's suggestion that it could be ATI drivers to blame, I have tested a
number of machines, including a Pentium M laptop, a dual P4 Xeon, an Athlon 64
X2 (dual core), an Athlon XP, and a dual Athlon MP. The X2 is PCI Express and
has been tested with an ATI X700 and an nVidia 7800GTX. The others were tested
with whatever they had: a 420 Go, some low-end Quadro, an ATI 8500 and an ATI
9800XT. With ATI cards the problem was evident, and with nVidia it wasn't.
Conspicuously absent from this matrix is an Intel+ATI combination, or any
platform other than Windows XP SP2, but I may be able to them later, and see
the thread I referenced earlier.

I'll see what else I can do to diagnose what's going on. Guidance would be
welcome. It seems that there's a problem with the ATI drivers and I'm bothered
that drawing an unscaled image is an order of magnitude faster than drawing a
scaled image. I'm equally bothered that some large greyscale images aren't
affected, but colour images are, even if I change the colour depth of the
display.
sysKin on IRC broke into firefox during the period of 100% CPU activity, and got
a stack trace.. bryner was nice enough to do the munging necessary to convert
the manual stacktrace into symbols.. the partial trace is:


nsImageWin::Draw()
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/gfx/src/windows/nsImageWin.cpp:635]
nsRenderingContextImpl::DrawImage()
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/gfx/src/shared/nsRenderingContextImpl.cpp:378]
nsImageBoxFrame::PaintImage()
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsImageBoxFrame.cpp:465]
nsImageBoxFrame::Paint()
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsImageBoxFrame.cpp:406]

Let me know if you need more... it looks like the GDI function being
called in that trace is SelectObject().

=====

Not sure what SelectObject does; pav?  (It's also the call right after the
actual rendering operation, so it could be that he cought the stack right after
rendering finished or something.)  But this is certainly something that windows
is doing and not us, directly.
This is probably our bug that's being made worse by drivers that aren't
optimizing things as much as they could be.  There's no reason we should be
drawing the full image each time for a thumbnail; unfortunately we don't have
any services in place for caching a thumbnail.

It's entirely possible that ATI's drivers (rightfully, in most cases) assume
that any images past a certain size won't be used frequently, so they don't
bother to put them into video ram.

A pref with max image size (width*height) beyond which we wouldn't set the
thumbnail should take care of this for now.  I'd reccommend a value of 768*1024..
Assignee: mconnor → vladimir
Patch that adds a pref called "browser.chrome.image_icons.max_size" with a
default of 1024; if either image dimension exceeds this value, then that image
gets no tab icon if its loaded in a tab.  I think 1024 should fix things;
however, if people still see the slowdown with this once it goes in while
viewing images that are close to that limit (say, looking at a 1024x1024
image), we should drop it lower until the problem goes away.

I'm trying to get a machine with an ATI in it here to test this locally as
well.
Attachment #193506 - Flags: superreview?(bzbarsky)
Attachment #193506 - Flags: review?(mconnor)
Comment on attachment 193506 [details] [diff] [review]
304561-limit-image-icon-size.patch

>             if (browser.contentDocument instanceof ImageDocument) {
>               if (this.mPrefs.getBoolPref("browser.chrome.site_icons"))
>-                this.setIcon(aTab, browser.currentURI.spec);
>+              {
>+                try {

nit: bracket on previous line, note that the line directly above uses different
style...

>+                  if (!req)
>+                    return;
>+                  if (req.image.width > sz ||
>+                      req.image.height > sz)
>+                    return;

if (!req || req.image.width > sz || req.image.height > sz)
  return;

r=me on the toolkit/pref bits, hopefully bz can r+sr the content bits
Attachment #193506 - Flags: review?(mconnor) → review+
Is there any reason we can't do what you suggested above, vlad, and "simply"
generate a resized version of the image and set that as the favicon instead of
resizing the big one each time? We could also use that image as the favicon when
you bookmark an image, if we did that, without putting multi-megabyte images
into the bookmarks file.
(In reply to comment #16)
> Is there any reason we can't do what you suggested above, vlad, and "simply"
> generate a resized version of the image and set that as the favicon instead of
> resizing the big one each time? We could also use that image as the favicon when
> you bookmark an image, if we did that, without putting multi-megabyte images
> into the bookmarks file.

That sounds great; let me know how I generate a resized version of the image and
set it as the src for an <img> :)
Well, one inefficient way would be to draw the image to a canvas and then grab a
data: URI out of the canvas... But I'm not suggesting doing it that way. Can't
imagelib provide some sort of convenient API to do something like this?
(In reply to comment #18)
> Well, one inefficient way would be to draw the image to a canvas and then grab a
> data: URI out of the canvas... But I'm not suggesting doing it that way. Can't
> imagelib provide some sort of convenient API to do something like this?

Actually, that would be a pretty efficient way; the problem is that we have no
way of encoding an image back into data for creating that data URI.  We'll have
something in place for gecko1.9/future, but not for ffox 1.5.
Okie dokie. Should we file a bug on this, or leave this one open on the matter?
Comment on attachment 193506 [details] [diff] [review]
304561-limit-image-icon-size.patch

Why not just expose the imgIRequest for the image instead of the
nsIImageLoadingContent?  I see no reason someone would need the DOM node
itself, and the fact that there even is one is really an implementation
detail...
Updated patch that just exposes the imgIRequest.. good idea! :)
Attachment #193506 - Attachment is obsolete: true
Attachment #193735 - Flags: superreview?(bzbarsky)
Comment on attachment 193735 [details] [diff] [review]
new patch, with bz's comments

>Index: content/html/document/src/nsImageDocument.cpp

>+nsImageDocument::GetImageRequest(imgIRequest** aImageRequest)
>+  if (mImageContent) {

do_QI is null-safe; no need for this null-check.

>Index: toolkit/content/widgets/tabbrowser.xml

>+                  if (!req ||
>+	              req.image.width > sz ||
>+                      req.image.height > sz)

Fix the indent here?

And you probably want a "!req.image ||" in there too -- req could be non-null
but could have a null image...

sr=bzbarsky with those nits.
Attachment #193735 - Flags: superreview?(bzbarsky) → superreview+
Attachment #193735 - Flags: approval1.8b4? → approval1.8b4+
Checked in on branch and trunk with nits fixed
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
can this one get "fixed1.8" ?
The problem appears to be solved
(In reply to comment #20)
> Okie dokie. Should we file a bug on this, or leave this one open on the matter?

Was there a follow-up bug filed?
Keywords: fixed1.8
I don't understand how the patch here would fix a memory leak (bug 302858). 
Doesn't it just avoid the case that causes the leak to show up?
*** Bug 325219 has been marked as a duplicate of this bug. ***
Depends on: 519824
Depends on: 1048664
You need to log in before you can comment on or make changes to this bug.