http://www.gnome.org/bounties/Memory.html#169347

The problem is that the memory is actually freed but only after 30 seconds. See
cleanup_cb in libbackground/applier.c. So probably it's nothing to fix here :)

Well, nautilus doesn't use libbackground but creates EelBackground object for
any folder. Gnome-settings-daemon works fine.

Try setting your background image to `none' and having a color background. Look
at what g-s-m says about your memory usage. Then, add your image. You will see
(screen height) * (screen width) * sizeof (int) * 3 bytes more of memory.

With nautilus removed from session situation is a bit different - after
background  change the memory used is twice the screen bitmap size and after 30
seconds it's reduced to the bitmap image size.

According to [1], the cc timeout has been added in 2002 by Richard Hestilow and
was slightly changed in the same year by Bradford Hovinen. CCing those. Any
ideas why this was added? 

[1]
http://cvs.gnome.org/viewcvs/gnome-control-center/libbackground/applier.c?rev=1.41&view=log

Created attachment 38349 [details] [review]
Patch to nautilus

This patch reduces memory usage to 2 * image size by using the same pixmap for
root pixmap and background of root nautilus window.

Hey,

Two things

- the `gc' var becomes unused. For some reason, i have -Werror, so that messes
up the build

- nautilus-directory-background.c: In function `image_loading_done_callback':
nautilus-directory-background.c:456: warning: `background_window' might be used
uninitialized in this function

I think that is a bug.

Also, looking at the eel function, I *think* that is copying the stuff. However,
am not sure...

Created attachment 38352 [details] [review]
Patch to nautilus CVS

Fix the errors pointed above

Never mind, I was able to see the improvement now by looking at X's rss usage.
Oddly, the total memory statistic in g-s-m didn't seem to get changed. Should
look at that (I don't think nautilus's rss was any worse)

Ignore me. I can see this clear as day now.

Created attachment 38353 [details]
Stack trace of crash regression from patch in comment #10

To trigger the crash, open a window and choose "Edit" -> "Backgrounds and
Emblems". Drag one of the backgrounds onto the desktop.

If anyone can get the crasher fixed before noon tomorrow (EST), it would be
really helpful. We are going to ship this in a SUSE 9.3 beta and that is the
deadline.

The current idea is we are shipping now, regression or not, so that we can test
other code paths. But it'd be cool to fix it too...

I assume this doesn't break the case where you're not running nautilus.  I
suppose it couldn't if we don't change the code to gsd.  :)

AFAIK, here's the summary for the rough-and-tumble bounty hunters (I think Ben 
alluded to it...): 
- If you are not running Nautilus, gnome-settings-daemon holds the background 
image for 30 seconds before cleaning it up... IMHO, nothing to see there, move 
along. 
- If you are running Nautilus, gnome-settings-daemon never gets a hold of the 
background image in the first place. 

I think the panel has the background in memory too (for translucent panels). See
the code in panel-background-monitor.c.

Created attachment 38358 [details] [review]
Patch to nautilus

Fix crash (really it was just an assert) and some X server issues.
set_root_pixmap function updated from libbackground

gekker, if the patch in comment 18 works, please try to get it in to beta 3. I 
am unable to try it here, because I am at school and don't have access to a 
GNOME build. I'd do the following testing:

1) test that starting of GNOME works with no background
2) test that you can change background by selecting a new image
3) test that you can change background via DnD

If 1 and 2 work, it should be good to do (that is at least as good as what we 
had before). If three works, that's really good :-).

Sadly, I am pretty sure you won't be able to contact me again before the 
deadline. Sorry.

re comment 17:

That sounds like it might need xorg magic. I think that getting the other 
duplicated image for the non-panel case will take some too. I'd appreciate 
input from an x wiz here.

About comment 17, see what I wrote here:

http://mail.gnome.org/archives/desktop-devel-list/2005-March/msg00001.html

Created attachment 38412 [details] [review]
Patch to EEL

This patch to eel does second part of task with method similar to libbackground
one - it adds timeout after that client side background pixbuf is freed.

It can be applied, but in general I think it's not natural way of doing things.
We do need server copy of background, we do need client copy of background
since we can perform significant operations on it like resizing and scaling. So
anyhow there should be two copies of image. 

The way we are trying to solve that problem is simulation of system swap, but
that is not the task of user program but task of operation system. Without
patch, memory will also swapped after some timeout and that process will be
more clear for programmers and user. As with patch actual memory will be freed.

"It can be applied, but in general I think it's not natural way of doing things.
We do need server copy of background, we do need client copy of background
since we can perform significant operations on it like resizing and scaling. So
anyhow there should be two copies of image."

Resizing/scaling needs to be done *once*. So, we rescale it, put the image in 
the server, and are done with it.

How do desktop icons work with this background? Who is responsible for anti-
aliasing the icons with the background?

>Resizing/scaling needs to be done *once*. So, we rescale it, put the image in 
>the server, and are done with it.

The problem is that on every user "background set" operation usually there are
three or four background pixbuf modification - user can rescale it, change
placement or change background color (if background has alpha) after that
background should be redrawn on pixmap. The load of pixbuf with 1600x1200 is
long time operation, it will significant slow-down to reread it from file every
time, so you _should_ have client copy of pixbuf.

Desktop icons are completely independent from background - one you setup widget
background pixmap you can draw on it whatever you like. Widget background will
remain.

"The problem is that on every user "background set" operation usually there are
three or four background pixbuf modification - user can rescale it, change
placement or change background color (if background has alpha) after that
background should be redrawn on pixmap. The load of pixbuf with 1600x1200 is
long time operation, it will significant slow-down to reread it from file every
time, so you _should_ have client copy of pixbuf."

Yes, but for any time when the user is *not* modifying the background, we want 
one and only one copy.

"Desktop icons are completely independent from background - one you setup widget
background pixmap you can draw on it whatever you like. Widget background will
remain."

Hrm, federico told me otherwise. Comments?

"Yes, but for any time when the user is *not* modifying the background, we want 
one and only one copy."

The timeout makes sure that is the case, eventually... while the user may still
be making up their mind about background colors on translucent images, or "Fill
Screen" vs "Centered", having the the image load from file and decompress proior
to applying each of those operations may be less welcome than having ~4MB stick
around for a few seconds.

Having the timeout is fine.  What Richard means is that programs which draw the
background (g-s-d or Nautilus) really only see a bunch of changes coming from
GConf.  You can't know when the user is done fiddling with the options, so we
need to free the client-side pixbufs in a timer.

About comment #25 and icons, I meant that Nautilus draws them on its own on a
window that is as big as the root window.  They are not in the pixmap data for
the background; they just get drawn with normal draw_pixbuf() calls.

There is also a copy of the background in gnome-terminal: bug 169704 .

For comment #28, it sounds like g-t can do the same thing as pach 38358: use the
same pixmap.

However, I do not think that fixing transparency in g-t or in the panel is
required for the bounty. Both are pretty much non-standard options.

Otoh, they shouldn't be all that hard to fix.

Good news: the patch does the memory job. And it does it really well. Here are
my observations, with both patches on:

Solid color background:     732.8 MB
Switch to a picture:        740.3 MB
After 30 seconds:           736.3 MB

Bad news: There are some regressions changing backgrounds. Here are a couple of
oddities I noticed

- I tried going from solid color background to a translucent tile with a horiz
gradiant in the background. I saw the tile scaled to the size of the screen, and
then it flashed to being correct

- I tried going from solid color to a scaled picture. I saw the picture flash
and then it was resized.

Am going to double check that these are actually regressions.

Ok, these are *not* regressions. They are very ugly though.

There is some unefficiency in nautilus background code. But they are easy fix i
think, just give me the time :)

*** Bug 168675 has been marked as a duplicate of this bug. ***

*** Bug 84147 has been marked as a duplicate of this bug. ***

A related patch seems to have made it into Garnome - causing a new crash. See
bug 169980.

Created attachment 38589 [details] [review]
Patch to nautilus

Fix problem in GARNOME with g_object_unref

Also, create work around oddities with background changes. The reason is that
although wallpaper capplet use gconf_change_set those operation is not atomic
in gconf, so notification recieved first for background, then for placement.
Added small timeout on gconf notification to let gconf update all values.

Created attachment 38611 [details] [review]
Patch to eel

Updated patch. Fixed interoperability issues with other programs trying to set
desktop background.

Why are the paches marked obsolete? Did you intend to attach new ones?

Created attachment 38664 [details] [review]
Nautilus patch

Created attachment 38665 [details] [review]
Eel patch

Finally I've retch the DAO of background setting :). The result is two patches
above. Short description of changes they introduce -

1. Small timeout for handling of gconf changes - more slickness as a result,
work around atomic gconf problem.

2. Client side pixbuf swapping - after 30 seconds client side image is freed

3. Desktop pixmap creation and setting code can not be removed, since it does
work to satisfy compatibility requirements with older X application. But to 
save memory usage we need to unify this background with background of nautilus
root window (Note, that it differs from X root window). So the utility code is
moved to eel-background.

This really needs to be reviewed by nautilus maintainers by now.

I'd really like to see these patches getting into 2.10.1. They are very user
visible. I am working to get distros shipping them. SUSE already has it, and
Ubuntu should soon. So it seems silly not to get it upstream.

You can't in general have the same pixmap for the desktop and the root window.
The visual used for the desktop window might not be the same as the one for the
root window. You need to verify that this is true before reusing it.

We tried the patches for Suse; they work fine, but Nautilus crashes if you
change resolutions with resapplet.

that doesn't seem to "work fine" :)

Created attachment 38724 [details] [review]
Nautilus patch

Created attachment 38725 [details] [review]
Eel patch

This patches solves problems mentioned above with different visuals and have
some
fixes of screen resolution change.

Unfortunately I was unable to reproduce crash on resize, so if you have it,
please, attach backtrace.

In general this looks good. But i have a few comments:

The callback in desktop_background_gconf_notify_cb doesn't protect against
background lifetime issues. It needs to ref the background, or make sure the
timeout is removed if the background goes away.

Why use GRADIENT_PIXMAP_TILE_SIZE*GRADIENT_PIXMAP_TILE_SIZE for a solid color?
That seems wasteful to me. We should just do a 1x1 pixmap in this case. We don't
want a dithered background anyway.

The code doesn't follow the nautilus coding standard (docs/style-guide.html).
For instance, always add braces and spaces before paranthesis in function calls.
Also, don't use if (boolean_var == 0) or if (boolean_var != 0), use if
(boolean_var) or if (!boolean_var).

Created attachment 38743 [details] [review]
Eel patch

Created attachment 38744 [details] [review]
Nautilus patch

Thanks for comments. Here are updated patches.

Commited.

I made some minor changes to the nautilus patch, to clean up the
"desktop_gconf_notification_timeout" data in the callback, and to handle it not
being set in the destructor.

There is still the report about crash on size change. If anyone can reproduce
that, please tell us.

This is a bounty, so it should be kept open.

I'll award this after it is pretty clear that the regression is not there.

I am seeing one small regression after these patches were applied.  Bug 42185
"desktop draws white at startup" is back.

I don't think that it's a regression. The problem is that 

realized_callback from fm-desktop-icon-view.c:342

is called before 

nautilus_connect_desktop_background_to_file_metadata

so desktop widget is realized without background and only then background is
attached. Just now I have no thoughts how to fix it. Can maintainers comment?

Created attachment 38820 [details] [review]
Nautilus patch

Here is patch to fm-desktop-icon-view that fixes the problem above, although I
am not sure that's the right way to fix this bug. 

Probably, there is need to fix also non-desktop background in the same way.

This means nautilus_connect_desktop_background_to_file_metadata will be called
twice for the desktop. It needs to be able to handle this without e.g.
monitoring things twice.

Also, can we really guarantee that the the file for the desktop window has been
set at the time of realization? I think its likely atm that this happens, but
weirness in timing or code changes later can change this. 

With this approach you need to change the realize code to be able to handle file
being NULL, and keep the call in fm_icon_view_begin_loading, but make sure it
handles being called twice. Also, add some comment to the code in realize to
describe why the code is there.

However, I think a more fruitfull approach is to look at why this is happening,
and how we can prevent it from happening for all windows. 

Things start in fm-directory-view:finish_loading(). This is called when we've
loaded a view for the directory and are ready to start using it. It calls:

	nautilus_window_info_report_load_underway (view->details->window,
						   NAUTILUS_VIEW (view));

	/* Tell interested parties that we've begun loading this directory now.
	 * Subclasses use this to know that the new metadata is now available.
	 */
	fm_directory_view_begin_loading (view);

Where report_load_underway switches to the new way and sets up the directory
file, and begin_loading is what sets the background.

The problem here is caused by us actually presenting the window in 
the report_load_underway handler, which is slightly to early. Now, this really
is sort of and arbitrary place to put this call. It was put here in the hopes
that most window setup is done, so we won't get much flicker at this point.
Clearly this is wrong, and we could benefit from doing it slightly later. 

So, I propose to introduce a new window callback,
nautilus_window_info_show_window() which is called from finish_loading after
begin_loading has been called.

Created attachment 38926 [details] [review]
Patch to fix last problem

Thanks, it seems to be more natural way. This patch implements proposed above.

Hmm, why did you move the nautilus_file_set_has_open_window call? Was this
required? It seems a bit strange.

If keept this call will force realize on desktop widget during emission of
"changed" signal on NautilusFile. This will call realize on some icons
(EelCanvasItem) and realize of the whole EelCanvas. 

I've thought that it can be clear that set_has_open window should be called only
when window is actually shown after gtk_widget_show.

Its sort of strange in the sense of the highlevel behaviour of the view interface.  

The beging_loading call is what initiates/sets the NautilusFile object for the
window, and it might be expected that it sets the has_open_window property on
the file. The fact that gtk_widget_show hasn't been called yet isn't really
important, because it will be called basically immediately afterwards. However,
if its needed for the flicker to go away we need to be more pragmatic.

Anyway, I'm applying this patch.

Thanks, by the way there is some old related code in
src/nautilus-desktop-window.c enclosed into #if 0/#endif. Probably it can be
removed now.

The patch is applied, so this appears to be fixed.  Ben and Alex have asked that
this bounty be awarded, so I am closing the bug.