widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/graphic_resetting into lp:widelands.

Requested reviews:
  Tino (tino79)
Related bugs:
  Bug #1159968 in widelands: "Crash in opengl fonthandler_cc:99"
  https://bugs.launchpad.net/widelands/+bug/1159968

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/graphic_resetting/+merge/159985

There is at least one critical bug lurking in this code - but I wasn't able to produce a reliable crash scenario, not did I see the bug. Reaching out for a bunch more eyes :)
-- 
https://code.launchpad.net/~widelands-dev/widelands/graphic_resetting/+merge/159985
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/graphic_resetting.

=== modified file 'src/graphic/font_handler.cc'
--- src/graphic/font_handler.cc	2013-06-15 13:38:19 +0000
+++ src/graphic/font_handler.cc	2013-07-16 20:53:40 +0000
@@ -111,9 +111,13 @@
 
 
 Font_Handler::~Font_Handler() {
+	flush();
 	Font::shutdown();
 }
 
+void Font_Handler::flush() {
+	d.reset(new Data);
+}
 
 /*
  * Returns the height of the font, in pixels.

=== modified file 'src/graphic/font_handler.h'
--- src/graphic/font_handler.h	2013-06-15 13:38:19 +0000
+++ src/graphic/font_handler.h	2013-07-16 20:53:40 +0000
@@ -20,6 +20,7 @@
 #ifndef FONT_HANDLER_H
 #define FONT_HANDLER_H
 
+#include <string>
 #include <boost/scoped_ptr.hpp>
 
 #include "point.h"
@@ -59,6 +60,9 @@
 		 uint32_t wrap = std::numeric_limits<uint32_t>::max());
 	uint32_t get_fontheight(const std::string & name, int32_t size);
 
+	// Delete the whole cache.
+	void flush();
+
 private:
 	struct Data;
 	boost::scoped_ptr<Data> d;

=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2013-07-14 17:41:02 +0000
+++ src/graphic/graphic.cc	2013-07-16 20:53:40 +0000
@@ -44,6 +44,7 @@
 
 #include "animation.h"
 #include "animation_gfx.h"
+#include "font_handler.h"
 #include "image.h"
 #include "image_loader_impl.h"
 #include "image_transformations.h"
@@ -62,14 +63,9 @@
 /**
  * Initialize the SDL video mode.
 */
-Graphic::Graphic
-	(int32_t w, int32_t h,
-	 int32_t bpp,
-	 bool    fullscreen,
-	 bool    opengl)
+Graphic::Graphic()
 	:
 	m_fallback_settings_in_effect (false),
-	m_rendertarget     (0),
 	m_nr_update_rects  (0),
 	m_update_fullscreen(true),
 	image_loader_(new ImageLoaderImpl()),
@@ -88,9 +84,12 @@
 	SDL_Surface * s = IMG_Load_RW(SDL_RWFromMem(fr.Data(0), fr.GetSize()), 1);
 	SDL_WM_SetIcon(s, 0);
 	SDL_FreeSurface(s);
+}
+
+void Graphic::initialize(int32_t w, int32_t h, int32_t bpp, bool fullscreen, bool opengl) {
+	cleanup();
 
 	// Set video mode using SDL. First collect the flags
-
 	int32_t flags = 0;
 	g_opengl = false;
 	SDL_Surface * sdlsurface = 0;
@@ -306,7 +305,6 @@
 		glEnable(GL_TEXTURE_2D);
 
 		GLSurfaceTexture::Initialize(use_arb);
-
 	}
 
 	if (g_opengl)
@@ -315,11 +313,11 @@
 	}
 	else
 	{
-		screen_.reset(new SDLSurface(sdlsurface));
+		screen_.reset(new SDLSurface(sdlsurface, false));
 	}
 
 	m_sdl_screen = sdlsurface;
-	m_rendertarget = new RenderTarget(screen_.get());
+	m_rendertarget.reset(new RenderTarget(screen_.get()));
 }
 
 bool Graphic::check_fallback_settings_in_effect()
@@ -327,25 +325,27 @@
 	return m_fallback_settings_in_effect;
 }
 
-/**
- * Free the surface
-*/
-Graphic::~Graphic()
-{
-	BOOST_FOREACH(Texture* texture, m_maptextures)
-		delete texture;
-	delete m_rendertarget;
-
+void Graphic::cleanup() {
+	flush_maptextures();
 	flush_animations();
+	surface_cache_->flush();
+	// TODO: this should really not be needed, but currently is :(
+	if (UI::g_fh)
+		UI::g_fh->flush();
 
 	if (g_opengl)
 		GLSurfaceTexture::Cleanup();
 }
 
+Graphic::~Graphic()
+{
+	cleanup();
+}
+
 /**
  * Return the screen x resolution
 */
-int32_t Graphic::get_xres() const
+int32_t Graphic::get_xres()
 {
 	return screen_->width();
 }
@@ -353,11 +353,21 @@
 /**
  * Return the screen x resolution
 */
-int32_t Graphic::get_yres() const
+int32_t Graphic::get_yres()
 {
 	return screen_->height();
 }
 
+int32_t Graphic::get_bpp()
+{
+	return m_sdl_screen->format->BitsPerPixel;
+}
+
+bool Graphic::is_fullscreen()
+{
+	return m_sdl_screen->flags & SDL_FULLSCREEN;
+}
+
 /**
  * Return a pointer to the RenderTarget representing the screen
 */
@@ -365,7 +375,7 @@
 {
 	m_rendertarget->reset();
 
-	return m_rendertarget;
+	return m_rendertarget.get();
 }
 
 /**

=== modified file 'src/graphic/graphic.h'
--- src/graphic/graphic.h	2013-03-09 08:07:22 +0000
+++ src/graphic/graphic.h	2013-07-16 20:53:40 +0000
@@ -86,13 +86,18 @@
  */
 class Graphic {
 public:
-	Graphic
-		(int32_t w, int32_t h, int32_t bpp,
-		 bool fullscreen, bool opengl);
+	Graphic();
 	~Graphic();
 
-	int32_t get_xres() const;
-	int32_t get_yres() const;
+	// Initialize or reinitialize the graphics system. Throws on error.
+	void initialize
+		(int32_t w, int32_t h, int32_t bpp, bool fullscreen, bool opengl);
+
+	int32_t get_xres();
+	int32_t get_yres();
+	int32_t get_bpp();
+	bool is_fullscreen();
+
 	RenderTarget * get_render_target();
 	void toggle_fullscreen();
 	void update_fullscreen();
@@ -133,6 +138,7 @@
 	bool check_fallback_settings_in_effect();
 
 private:
+	void cleanup();
 	void save_png_(Surface & surf, StreamWrite*) const;
 
 	bool m_fallback_settings_in_effect;
@@ -153,7 +159,7 @@
 	/// manipulation the screen context.
 	SDL_Surface * m_sdl_screen;
 	/// A RenderTarget for screen_. This is initialized during init()
-	RenderTarget * m_rendertarget;
+	boost::scoped_ptr<RenderTarget> m_rendertarget;
 	/// keeps track which screen regions needs to be redrawn during the next
 	/// update(). Only used for SDL rendering.
 	SDL_Rect m_update_rects[MAX_RECTS];

=== modified file 'src/graphic/image_cache.h'
--- src/graphic/image_cache.h	2013-02-10 16:08:37 +0000
+++ src/graphic/image_cache.h	2013-07-16 20:53:40 +0000
@@ -25,6 +25,7 @@
 #include <boost/utility.hpp>
 
 #include "image.h"
+
 class IImageLoader;
 class SurfaceCache;
 

=== modified file 'src/graphic/render/sdl_surface.cc'
--- src/graphic/render/sdl_surface.cc	2013-07-12 15:11:32 +0000
+++ src/graphic/render/sdl_surface.cc	2013-07-16 20:53:40 +0000
@@ -26,7 +26,8 @@
 SDLSurface::~SDLSurface() {
 	assert(m_surface);
 
-	SDL_FreeSurface(m_surface);
+	if (m_free_surface_on_delete)
+		SDL_FreeSurface(m_surface);
 }
 
 const SDL_PixelFormat & SDLSurface::format() const {

=== modified file 'src/graphic/render/sdl_surface.h'
--- src/graphic/render/sdl_surface.h	2013-02-10 16:41:12 +0000
+++ src/graphic/render/sdl_surface.h	2013-07-16 20:53:40 +0000
@@ -33,10 +33,13 @@
 */
 class SDLSurface : public Surface {
 public:
-	SDLSurface(SDL_Surface* surface) :
+	// The surface set by SetVideoMode must not be freed according to the SDL
+	// docs, so we need 'free_surface_on_delete'.
+	SDLSurface(SDL_Surface* surface, bool free_surface_on_delete = true) :
 		m_surface(surface),
 		m_offsx(0), m_offsy(0),
-		m_w(surface->w), m_h(surface->h)
+		m_w(surface->w), m_h(surface->h),
+		m_free_surface_on_delete(free_surface_on_delete)
 	{}
 	virtual ~SDLSurface();
 
@@ -72,6 +75,7 @@
 	int32_t m_offsx;
 	int32_t m_offsy;
 	uint16_t m_w, m_h;
+	bool m_free_surface_on_delete;
 };
 
 

=== modified file 'src/graphic/surface_cache.cc'
--- src/graphic/surface_cache.cc	2013-07-13 14:32:49 +0000
+++ src/graphic/surface_cache.cc	2013-07-16 20:53:40 +0000
@@ -39,6 +39,7 @@
 	virtual ~SurfaceCacheImpl();
 
 	// Implements SurfaceCache.
+	virtual void flush();
 	virtual Surface* get(const string& hash);
 	virtual Surface* insert(const string& hash, Surface*, bool);
 
@@ -64,9 +65,16 @@
 };
 
 SurfaceCacheImpl::~SurfaceCacheImpl() {
+	flush();
+}
+
+void SurfaceCacheImpl::flush() {
 	for (Container::iterator it = entries_.begin(); it != entries_.end(); ++it) {
 		delete it->second;
 	}
+	entries_.clear();
+	access_history_.clear();
+	used_transient_memory_ = 0;
 }
 
 Surface* SurfaceCacheImpl::get(const string& hash) {

=== modified file 'src/graphic/surface_cache.h'
--- src/graphic/surface_cache.h	2013-07-13 14:32:49 +0000
+++ src/graphic/surface_cache.h	2013-07-16 20:53:40 +0000
@@ -38,6 +38,9 @@
 	SurfaceCache() {};
 	virtual ~SurfaceCache() {};
 
+	/// Deletes all surfaces in the cache leaving it as if it were just created.
+	virtual void flush() = 0;
+
 	/// Returns an entry if it is cached, NULL otherwise.
 	virtual Surface* get(const std::string& hash) = 0;
 

=== modified file 'src/helper.cc'
--- src/helper.cc	2013-02-10 19:36:24 +0000
+++ src/helper.cc	2013-07-16 20:53:40 +0000
@@ -17,9 +17,16 @@
  *
  */
 
+#include <cstdarg>
+#include <string>
+
+#include <boost/random.hpp>
+#include <boost/scoped_array.hpp>
+
 #include "helper.h"
 
-#include <cstdarg>
+using namespace std;
+
 
 /// Split a string by separators.
 /// \note This ignores empty elements, so do not use this for example to split
@@ -71,3 +78,14 @@
 		((k.sym >= SDLK_WORLD_0) && (k.sym <= SDLK_WORLD_95)) ||
 		((k.sym >= SDLK_KP0)     && (k.sym <= SDLK_KP_EQUALS));
 }
+
+static boost::random::mt19937 random_generator;
+string random_string(const string& chars, int nlen) {
+	boost::random::uniform_int_distribution<> index_dist(0, chars.size() - 1);
+	boost::scoped_array<char> buffer(new char[nlen - 1]);
+	for (int i = 0; i < nlen; ++i) {
+		buffer[i] = chars[index_dist(random_generator)];
+	}
+	return string(buffer.get(),nlen);
+}
+

=== modified file 'src/helper.h'
--- src/helper.h	2013-02-10 19:36:24 +0000
+++ src/helper.h	2013-07-16 20:53:40 +0000
@@ -158,4 +158,7 @@
 
 bool is_printable(SDL_keysym k);
 
+/// Generate a random string of given size out of the given alphabet.
+std::string random_string(const std::string& chars, int nlen);
+
 #endif

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2013-03-02 20:35:18 +0000
+++ src/ui_basic/panel.cc	2013-07-16 20:53:40 +0000
@@ -17,15 +17,14 @@
  *
  */
 
-#include <boost/concept_check.hpp>
-
 #include "constants.h"
 #include "graphic/font_handler.h"
 #include "graphic/font_handler1.h"
 #include "graphic/graphic.h"
-#include "graphic/in_memory_image.h"
 #include "graphic/rendertarget.h"
 #include "graphic/surface.h"
+#include "graphic/surface_cache.h"
+#include "helper.h"
 #include "log.h"
 #include "profile/profile.h"
 #include "sound/sound_handler.h"
@@ -34,8 +33,37 @@
 
 #include "panel.h"
 
+using namespace std;
+
 namespace UI {
 
+namespace  {
+class CacheImage : public Image {
+public:
+	CacheImage(uint16_t w, uint16_t h) :
+		width_(w), height_(h),
+		hash_("cache_image_" + random_string("0123456789ABCDEFGH", 32)) {}
+	virtual ~CacheImage() {}
+
+	// Implements Image.
+	virtual uint16_t width() const {return width_;}
+	virtual uint16_t height() const {return height_;}
+	virtual const string& hash() const {return hash_;}
+	virtual Surface* surface() const {
+		Surface* rv = g_gr->surfaces().get(hash_);
+		if (rv)
+			return rv;
+
+		rv = g_gr->surfaces().insert(hash_, Surface::create(width_, height_), true);
+		return rv;
+	}
+
+private:
+	const int16_t width_, height_;
+	const string hash_;
+};
+
+}  // namespace
 Panel * Panel::_modal       = 0;
 Panel * Panel::_g_mousegrab = 0;
 Panel * Panel::_g_mousein   = 0;
@@ -835,12 +863,8 @@
 		uint32_t innerw = _w - (_lborder + _rborder);
 		uint32_t innerh = _h - (_tborder + _bborder);
 
-		if
-			(!_cache ||
-			 _cache.get()->width() != innerw ||
-			 _cache.get()->height() != innerh)
-		{
-			_cache.reset(new_in_memory_image("dummy_hash", Surface::create(innerw, innerh)));
+	if (!_cache || _cache->width() != innerw || _cache->height() != innerh) {
+			_cache.reset(new CacheImage(innerw, innerh));
 			_needdraw = true;
 		}
 

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2013-07-15 05:18:12 +0000
+++ src/wlapplication.cc	2013-07-16 20:53:40 +0000
@@ -263,9 +263,6 @@
 m_mouse_locked         (0),
 m_mouse_compensate_warp(0, 0),
 m_should_die           (false),
-m_gfx_w(0), m_gfx_h(0), m_gfx_bpp(0),
-m_gfx_fullscreen       (false),
-m_gfx_opengl           (true),
 m_default_datadirs     (true),
 #ifdef WIN32
 m_homedir(FileSystem::GetHomedir() + "\\.widelands"),
@@ -470,7 +467,6 @@
 			throw;
 		}
 	} else {
-
 		g_sound_handler.start_music("intro");
 
 		{
@@ -810,41 +806,31 @@
 }
 
 /**
- * Initialize the graphics subsystem (or shutdown, if system == GFXSYS_NONE)
+ * Initialize the graphics subsystem (or shutdown, if w and h are 0)
  * with the given resolution.
  * Throws an exception on failure.
- *
- * \note Because of the way pictures are handled now, this function must not be
- * called while UI elements are active.
- *
- * \todo Ensure that calling this with active UI elements does barf
- * \todo Document parameters
  */
-
 void WLApplication::init_graphics
 	(const int32_t w, const int32_t h, const int32_t bpp,
 	 const bool fullscreen, const bool opengl)
 {
-	if
-		(w == m_gfx_w && h == m_gfx_h && bpp == m_gfx_bpp &&
-		 fullscreen == m_gfx_fullscreen &&
-		 opengl == m_gfx_opengl)
+	if (!w && !h) { // shutdown.
+		delete g_gr;
+		g_gr = 0;
 		return;
-
-	delete g_gr;
-	g_gr = 0;
-
-	m_gfx_w = w;
-	m_gfx_h = h;
-	m_gfx_bpp = bpp;
-	m_gfx_fullscreen = fullscreen;
-	m_gfx_opengl = opengl;
-
-
-	// If we are not to be shut down
-	if (w && h) {
-		g_gr = new Graphic
-			(w, h, bpp, fullscreen, opengl);
+	}
+	assert(w > 0 && h > 0);
+
+	if (!g_gr) {
+		g_gr = new Graphic();
+		g_gr->initialize(w, h, bpp, fullscreen, opengl);
+	} else {
+		if
+			(g_gr->get_xres() != w || g_gr->get_yres() != h || g_gr->get_bpp() != bpp
+				|| g_gr->is_fullscreen() != fullscreen || g_opengl != opengl)
+		{
+			g_gr->initialize(w, h, bpp, fullscreen, opengl);
+		}
 	}
 }
 
@@ -856,7 +842,7 @@
 	init_graphics
 		(s.get_int("xres", XRES),
 		 s.get_int("yres", YRES),
-		 s.get_int("depth", 16),
+		 s.get_int("depth", 32),
 		 s.get_bool("fullscreen", false),
 		 s.get_bool("opengl", true));
 }
@@ -881,14 +867,12 @@
 	set_input_grab(s.get_bool("inputgrab", false));
 	set_mouse_swap(s.get_bool("swapmouse", false));
 
-	m_gfx_fullscreen = s.get_bool("fullscreen", false);
-
-	m_gfx_opengl = s.get_bool("opengl", true);
-
 	// KLUDGE!
 	// Without this the following config options get dropped by check_used().
 	// Profile needs support for a Syntax definition to solve this in a
 	// sensible way
+	s.get_bool("fullscreen");
+	s.get_bool("opengl");
 	s.get_int("xres");
 	s.get_int("yres");
 	s.get_int("border_snap_distance");
@@ -1085,12 +1069,7 @@
 	SDL_EnableUNICODE(1); //needed by helper.h:is_printable()
 	SDL_EnableKeyRepeat(SDL_DEFAULT_REPEAT_DELAY, SDL_DEFAULT_REPEAT_INTERVAL);
 
-	uint32_t xres = s.get_int("xres", XRES);
-	uint32_t yres = s.get_int("yres", YRES);
-
-	init_graphics
-		(xres, yres, s.get_int("depth", 16),
-		 m_gfx_fullscreen, m_gfx_opengl);
+	refresh_graphics();
 
 	// Start the audio subsystem
 	// must know the locale before calling this!

=== modified file 'src/wlapplication.h'
--- src/wlapplication.h	2013-05-20 19:40:13 +0000
+++ src/wlapplication.h	2013-07-16 20:53:40 +0000
@@ -287,21 +287,6 @@
 	///true if an external entity wants us to quit
 	bool   m_should_die;
 
-	///The Widelands window's width in pixels
-	int32_t    m_gfx_w;
-
-	///The Widelands window's height in pixels
-	int32_t    m_gfx_h;
-
-	///The Widelands window's bits per pixel
-	int32_t    m_gfx_bpp;
-
-	///If true Widelands is (should be, we never know ;-) running
-	///in a fullscreen window
-	bool   m_gfx_fullscreen;
-
-	bool   m_gfx_opengl;
-
 	//do we want to search the default places for widelands installs
 	bool   m_default_datadirs;
 	std::string m_homedir;