Merge lp:~widelands-dev/widelands/graphic_resetting into lp:widelands

[SirVer]

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 :)

[cghislai (charlyghislain)]

I get an invalid pointer when trying to free colormap (Colormap::~Colormap at line 66)
This happens when i change to opengl rendering after having played with non-opengl graphics

Additionally, i get a crash at GLSurfaceTexture::setup_gl(), line 280. This happens when using opengl and buildings statistics. The cause seems to be the attempt to render an empty text. At SubTagRenderNode::render(SurfaceCache* surface_cache), the following code throws the exception in place of the crash:

virtual Surface* render(SurfaceCache* surface_cache) {
  if (width() == 0 && height() == 0)
   throw RT::Exception("RT render: Tried to render empty surface");

Handling special cases to prevent blitting to happen for empty text (but with rich text tags), this latter crash can be avoided.

Using trunk, i get the crash at fonthandler_1.cc:99 described in the bug report when trying to render the loading screen tip using opengl.

[Tino (tino79)]

Ok, got it to compile with mingw, but it does not work on windows:
Widelands starts fine but as soon as i click to proceed to the main menu Widelands just closes.
No output into stderr.txt and stdout.txt just ends after the graphics initialization part

review: Needs Fixing

[SirVer (sirver)]

Thanks for the testing - I'll review the branch again and try to figure out what is wrong. That it crashes this early for you is an indication that something very basic is wrong. Maybe valgrind can help me out.

[Tino (tino79)]

It seems like a SIGTRAP error as described here: http://stackoverflow.com/questions/1621059/breakpoints-out-of-nowhere-when-debugging-with-gdb-inside-ntdll

So with "handle SIGTRAP nostop" widelands continues to the main menu.

I can't find the code point in widelands where this happens, will try windbg...

[Tino (tino79)]

Ok, the cuplrit seems to be the truncating in helper.cc::random_string()

    buffer[nlen] = '\0';

Afterwards the SIGTRAP occurs at some point when boost tries to delete the buffer.
I've committed a small change which fixes the code at least for my system.
Perhaps using boost:UUID could replace the manual string generation?

Now this trunk allows me to switch the graphic mode ingame, but only before i've run a game. If I go back from a game to the main menu and switch the graphics mode widelands still quits.

[cghislai (charlyghislain)]

at r6566, I can play normally.

I get errors when graphics are reset after a game:
segfault at free(colormap); in Colormap destructor
invalid pointer at delete texutre in Graphic::cleanup()

[cghislai (charlyghislain)]

This last commit fixes the crash on my side

[cghislai (charlyghislain)]

Also it seems graphics are restted far too often. Once on game start, then once each time mainmenu is displayed, then once on game loading, at least.

[SirVer (sirver)]

Both of the bugs you guys were fixing are really embarrassing - but thanks for that :).

I will check and see if the branch is now crash free for me as well and then really propose for merging. This will likely not happen till the weekend though.

[Tino (tino79)]

Ok, i can't trigger any more crashes now, seems to work perfectly.

review: Approve

[Jens Beyer (qcumber-some)]

I also can't make it crash anymore :-)

[cghislai (charlyghislain)]

I also approve this to be merged as I need this code to be able to spot issues related to opengl rendering.

review: Approve

=== 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;