Adjust size of surface cache

Bug #1121944 reported by Nicolai Hähnle
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
widelands
Fix Released
Medium
Unassigned

Bug Description

The surface cache introduced with bzr6501 is fixed to 30MB. This is not enough to hold all the graphics used in a typical game, even for a single player. As a result, the game sometimes stalls briefly while image data is reloaded into the cache, especially when jumping around on the map.

To help with this issue, the game should at least let users change the size of the surface cache manually via a command line option and perhaps an advanced configuration option.

In addition, the game should probably attempt to determine a reasonable size for the surface cache automatically, based on the available video memory. This may require some non-portable, operating system dependent code.

Tags: graphics

Related branches

Revision history for this message
Shevonar (shevonar) wrote :

We can let SDL determine how much video memory is available [1] so we don't need platform dependent code. I prefer the automatic adjustment of cache size over a manual configuration, which won't be used by most people I guess.

[1] http://www.libsdl.org/cgi/docwiki.cgi/SDL_VideoInfo

Revision history for this message
Nicolai Hähnle (nha) wrote :

Thanks for the info, that's good to know! I definitely prefer automatic adjustment as well, I just believe in manual overrides for the case when the automatic adjustment guesses wrong for whatever reasons. It's always good to have a fallback for workarounds.

SirVer (sirver)
Changed in widelands:
status: New → Confirmed
assignee: nobody → SirVer (sirver)
Revision history for this message
SirVer (sirver) wrote :

I have a bunch of thoughts on this issue: firstly, if you look at the (plentyfully available) debug output you see that the cache is only dropping really old stuff (a few seconds old). So it is not too small in the sense that it is not able to keep everything needed for the current frame in memory. It is just that we only load stuff when we really want to render it - which is too late and leads to the stuttering. I have no real idea how to improve this situation besides force loading 'related animations' which could mean 'stuff I might need soon' or 'stuff that is not on the screen but nearby'.

But the real idea behind the cache was to limit memory growth (which I did not fully succeed at: Widelands memory still grows linearly and I have no clue way. But it grows much slower now) and not to reduce overall memory footprint. So making the cache larger seems like an easy fix.

Video memory might not be the correct benchmark though. afaik opengl cares for getting stuff from system memory to video memory, so it will only crash when there is no system memory left (can someone verify this?). If so, we can determine total RAM (how) and use a fraction of it or just set the fixed size to something that will hold most graphic files (maybe 100MB or 150MB, I do not know). The stutter at the beginning of a (loaded) game will not away with this though (it is there from the on-demand loading patch and has nothing to do with the big merge). Occasional cache misses will still stutter even with a bigger cache - sometimes we just have to go to the disk.

Comments or ideas?

Changed in widelands:
status: Confirmed → Incomplete
Revision history for this message
Nicolai Hähnle (nha) wrote :

I don't think you really can improve much. Users can always just jump directly to a different point on the map, and there's no way to predict that. It seems much better to just let the cache grow a bit larger. If the system has enough memory, it should be possible to have a cache that only ever evicts font stuff during play (so that no disk access is necessary at all).

You are correct that the graphics driver will take care of moving texture between system and video memory. On older systems there are subtleties related to AGP as well, and on top of that, integrated graphics are always much more "fun".

I honestly don't know what the best guess would be (which is also a reason to have a manual override for whatever the automatic detection does). Something like max(SOMETHING_SMALL, min(SOMETHING_LARGE, (video_mem - ballpark over-estimate of framebuffer size and swap/compositing buffers) + max(0, (system_mem - safety) / x)))?

Revision history for this message
SirVer (sirver) wrote :

As a clutch fix I increased the cache size to be 150 MB for now.

Revision history for this message
SirVer (sirver) wrote :

I made a test just now and realized that all animation graphics need ~380MB in the surface cache right now.

Revision history for this message
SirVer (sirver) wrote :

Fwiw, I just implemented a version of Widelands that will parse all animations immediately, but will defer loading images to a background thread which will just load them and put them into the SurfaceCache. I have a SSD, so I feel very little difference, could somebody else test this please?

some caveats:
- This is proof of concept - it contains a lot of NOCOMMIT fixmes and is a hacky design.
- It only works with the SDL renderer right now - the problem is that creating surfaces needs gl calls and you are only allowed to call gl from one thread only. So surface generation must be limited to one thread alone. I would be very open to design suggestions how to circumvent this problem.
- needs boost_thread.

generally speaking hitting the disk to read animations/images on the fly seems a nogo. And increases the surfaceCache will not help either: every rendered text is accessed at least once - If you fill the cache with all animation frames, but then never meet an atlantean for hours, all the atlantean graphics will have been pushed out of the cache again by text surfaces and you will hit the disk which leads to stutter. Generating a sprite map of the animations and therefore reducing total size of animation and number of image files to load is the way to go here. Animations should then be loaded at the start of the game and kept in memory forever. I might take a stab at this when I find time.

Comments?

Revision history for this message
SirVer (sirver) wrote :

This will need to be looked at again as soon as the spritemap branch is merged.

Changed in widelands:
milestone: build18-rc1 → build19-rc1
Revision history for this message
SirVer (sirver) wrote :

Setting to incomplete for bug sweeping.

Revision history for this message
SirVer (sirver) wrote :

We only use the texture cache for transient surfaces these days, i.e. rendered text. It is fine to go back to 30MB for now.

SirVer (sirver)
Changed in widelands:
status: Incomplete → Fix Committed
assignee: SirVer (sirver) → nobody
GunChleoc (gunchleoc)
tags: added: graphics
removed: graphic
GunChleoc (gunchleoc)
Changed in widelands:
status: Fix Committed → Fix Released
Revision history for this message
GunChleoc (gunchleoc) wrote :

Fixed in build19-rc1.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.