widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

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

Commit message:
- Write screenshot before swapping data into the backbuffer. Before this change, we used glReadPixels() which always reads the backbuffer, i.e. the last frame. Now we force a redraw when a screenshot is requested and save the screenshot from the freshly drawn backbuffer before it is swapped to the front.
- m_blub -> blub_ in class Graphic.
- removed Graphic::save_png, since it was a stateless wrapper for save_to_png().

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1302565 in widelands: "Screenshot shows wrong image when a game is chosen with the mouse pointer"
  https://bugs.launchpad.net/widelands/+bug/1302565

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

See commit message above.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/screenshot_from_backbuffer into lp:widelands.

=== modified file 'src/graphic/CMakeLists.txt'
--- src/graphic/CMakeLists.txt	2016-01-16 15:19:32 +0000
+++ src/graphic/CMakeLists.txt	2016-01-19 07:44:26 +0000
@@ -204,6 +204,7 @@
     base_macros
     economy
     graphic
+    graphic_image_io
     graphic_surface
     logic
     wui_mapview_pixelfunctions

=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2016-01-16 15:19:32 +0000
+++ src/graphic/graphic.cc	2016-01-19 07:44:26 +0000
@@ -70,9 +70,9 @@
  * Initialize the SDL video mode.
  */
 void Graphic::initialize(int window_mode_w, int window_mode_h, bool init_fullscreen) {
-	m_window_mode_width = window_mode_w;
-	m_window_mode_height = window_mode_h;
-	m_requires_update = true;
+	window_mode_width_ = window_mode_w;
+	window_mode_height_ = window_mode_h;
+	requires_update_ = true;
 
 	// Request an OpenGL 2 context with double buffering.
 	SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1);
@@ -80,19 +80,19 @@
 	SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 1);
 	SDL_GL_SetAttribute(SDL_GL_ACCELERATED_VISUAL, 1);
 
-	log("Graphics: Try to set Videomode %ux%u\n", m_window_mode_width, m_window_mode_height);
-	m_sdl_window =
+	log("Graphics: Try to set Videomode %ux%u\n", window_mode_width_, window_mode_height_);
+	sdl_window_ =
 	   SDL_CreateWindow("Widelands Window", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED,
-	                    m_window_mode_width, m_window_mode_height, SDL_WINDOW_OPENGL);
+	                    window_mode_width_, window_mode_height_, SDL_WINDOW_OPENGL);
 
 	resolution_changed();
 	set_fullscreen(init_fullscreen);
 
-	SDL_SetWindowTitle(m_sdl_window, ("Widelands " + build_id() + '(' + build_type() + ')').c_str());
-	set_icon(m_sdl_window);
+	SDL_SetWindowTitle(sdl_window_, ("Widelands " + build_id() + '(' + build_type() + ')').c_str());
+	set_icon(sdl_window_);
 
-	m_glcontext = SDL_GL_CreateContext(m_sdl_window);
-	SDL_GL_MakeCurrent(m_sdl_window, m_glcontext);
+	gl_context_ = SDL_GL_CreateContext(sdl_window_);
+	SDL_GL_MakeCurrent(sdl_window_, gl_context_);
 
 #ifdef USE_GLBINDING
 	glbinding::Binding::initialize();
@@ -128,12 +128,12 @@
 
 	glClear(GL_COLOR_BUFFER_BIT);
 
-	SDL_GL_SwapWindow(m_sdl_window);
+	SDL_GL_SwapWindow(sdl_window_);
 
 	/* Information about the video capabilities. */
 	{
 		SDL_DisplayMode disp_mode;
-		SDL_GetWindowDisplayMode(m_sdl_window, &disp_mode);
+		SDL_GetWindowDisplayMode(sdl_window_, &disp_mode);
 		log("**** GRAPHICS REPORT ****\n"
 		    " VIDEO DRIVER %s\n"
 		    " pixel fmt %u\n"
@@ -156,13 +156,13 @@
 	if (UI::g_fh)
 		UI::g_fh->flush();
 
-	if (m_sdl_window) {
-		SDL_DestroyWindow(m_sdl_window);
-		m_sdl_window = nullptr;
+	if (sdl_window_) {
+		SDL_DestroyWindow(sdl_window_);
+		sdl_window_ = nullptr;
 	}
-	if (m_glcontext) {
-		SDL_GL_DeleteContext(m_glcontext);
-		m_glcontext = nullptr;
+	if (gl_context_) {
+		SDL_GL_DeleteContext(gl_context_);
+		gl_context_ = nullptr;
 	}
 }
 
@@ -183,21 +183,21 @@
 }
 
 void Graphic::change_resolution(int w, int h) {
-	m_window_mode_width = w;
-	m_window_mode_height = h;
+	window_mode_width_ = w;
+	window_mode_height_ = h;
 
 	if (!fullscreen()) {
-		SDL_SetWindowSize(m_sdl_window, w, h);
+		SDL_SetWindowSize(sdl_window_, w, h);
 		resolution_changed();
 	}
 }
 
 void Graphic::resolution_changed() {
 	int new_w, new_h;
-	SDL_GetWindowSize(m_sdl_window, &new_w, &new_h);
+	SDL_GetWindowSize(sdl_window_, &new_w, &new_h);
 
 	screen_.reset(new Screen(new_w, new_h));
-	m_rendertarget.reset(new RenderTarget(screen_.get()));
+	render_target_.reset(new RenderTarget(screen_.get()));
 
 	Notifications::publish(GraphicResolutionChanged{new_w, new_h});
 
@@ -209,13 +209,13 @@
 */
 RenderTarget * Graphic::get_render_target()
 {
-	m_rendertarget->reset();
-	return m_rendertarget.get();
+	render_target_->reset();
+	return render_target_.get();
 }
 
 bool Graphic::fullscreen()
 {
-	uint32_t flags = SDL_GetWindowFlags(m_sdl_window);
+	uint32_t flags = SDL_GetWindowFlags(sdl_window_);
 	return (flags & SDL_WINDOW_FULLSCREEN) || (flags & SDL_WINDOW_FULLSCREEN_DESKTOP);
 }
 
@@ -232,35 +232,33 @@
 	// whet fullscreen, we do it when in windowed mode.
 	if (value) {
 		SDL_DisplayMode display_mode;
-		SDL_GetDesktopDisplayMode(SDL_GetWindowDisplayIndex(m_sdl_window), &display_mode);
-		SDL_SetWindowSize(m_sdl_window, display_mode.w, display_mode.h);
+		SDL_GetDesktopDisplayMode(SDL_GetWindowDisplayIndex(sdl_window_), &display_mode);
+		SDL_SetWindowSize(sdl_window_, display_mode.w, display_mode.h);
 
-		SDL_SetWindowFullscreen(m_sdl_window, SDL_WINDOW_FULLSCREEN_DESKTOP);
+		SDL_SetWindowFullscreen(sdl_window_, SDL_WINDOW_FULLSCREEN_DESKTOP);
 	} else {
-		SDL_SetWindowFullscreen(m_sdl_window, 0);
+		SDL_SetWindowFullscreen(sdl_window_, 0);
 
 		// Next line does not work. See comment in refresh().
-		SDL_SetWindowSize(m_sdl_window, m_window_mode_width, m_window_mode_height);
+		SDL_SetWindowSize(sdl_window_, window_mode_width_, window_mode_height_);
 	}
 	resolution_changed();
 }
 
 
 void Graphic::update() {
-	m_requires_update = true;
+	requires_update_ = true;
 }
 
 /**
  * Returns true if parts of the screen have been marked for refreshing.
 */
 bool Graphic::need_update() const {
-	return m_requires_update;
+	return requires_update_;
 }
 
 /**
  * Bring the screen uptodate.
- *
- * \param force update whole screen
 */
 void Graphic::refresh()
 {
@@ -271,33 +269,33 @@
 	// refresh() when in window mode.
 	if (!fullscreen()) {
 		int true_width, true_height;
-		SDL_GetWindowSize(m_sdl_window, &true_width, &true_height);
-		if (true_width != m_window_mode_width || true_height != m_window_mode_height) {
-			SDL_SetWindowSize(m_sdl_window, m_window_mode_width, m_window_mode_height);
+		SDL_GetWindowSize(sdl_window_, &true_width, &true_height);
+		if (true_width != window_mode_width_ || true_height != window_mode_height_) {
+			SDL_SetWindowSize(sdl_window_, window_mode_width_, window_mode_height_);
 		}
 	}
 
-	SDL_GL_SwapWindow(m_sdl_window);
-	m_requires_update = false;
-}
-
-
-/**
- * Saves a pixel region to a png. This can be a file or part of a stream.
- *
- * @param surf The Surface to save
- * @param sw a StreamWrite where the png is written to
- */
-void Graphic::save_png(Texture* texture, StreamWrite * sw) const {
-	save_to_png(texture, sw, ColorType::RGBA);
+	// The backbuffer now contains the current frame. If we want a screenshot,
+	// we should better take it now, before this is swapped out to the
+	// frontbuffer and becomes inaccessible to us.
+	if (!screenshot_filename_.empty()) {
+		log("Save screenshot to %s\n", screenshot_filename_.c_str());
+		std::unique_ptr<StreamWrite> sw(g_fs->open_stream_write(screenshot_filename_));
+		save_to_png(screen_->to_texture().get(), sw.get(), ColorType::RGB);
+		screenshot_filename_.clear();
+	}
+
+	SDL_GL_SwapWindow(sdl_window_);
+	requires_update_ = false;
 }
 
 /**
  * Save a screenshot to the given file.
 */
-void Graphic::screenshot(const string& fname) const
+void Graphic::screenshot(const string& fname)
 {
-	log("Save screenshot to %s\n", fname.c_str());
-	std::unique_ptr<StreamWrite> sw(g_fs->open_stream_write(fname));
-	save_to_png(screen_->to_texture().get(), sw.get(), ColorType::RGB);
+	screenshot_filename_ = fname;
+
+	// Force a redraw of the screen soon.
+	update();
 }

=== modified file 'src/graphic/graphic.h'
--- src/graphic/graphic.h	2016-01-09 15:27:05 +0000
+++ src/graphic/graphic.h	2016-01-19 07:44:26 +0000
@@ -71,41 +71,48 @@
 	void update();
 	bool need_update() const;
 	void refresh();
-	SDL_Window* get_sdlwindow() {return m_sdl_window;}
+	SDL_Window* get_sdlwindow() {return sdl_window_;}
 
 	ImageCache& images() const {return *image_cache_.get();}
 	AnimationManager& animations() const {return *animation_manager_.get();}
 
-	void save_png(Texture*, StreamWrite*) const;
-
-	void screenshot(const std::string& fname) const;
+	// Requests a screenshot being taken on the next frame.
+	void screenshot(const std::string& fname);
 
 private:
 	// Called when the resolution (might) have changed.
 	void resolution_changed();
 
 	// The height & width of the window should we be in window mode.
-	int m_window_mode_width;
-	int m_window_mode_height;
+	int window_mode_width_;
+	int window_mode_height_;
 
 	/// This is the main screen Surface.
 	/// A RenderTarget for this can be retrieved with get_render_target()
 	std::unique_ptr<Screen> screen_;
+
 	/// This saves a copy of the screen SDL_Surface. This is needed for
 	/// opengl rendering as the SurfaceOpenGL does not use it. It allows
 	/// manipulation the screen context.
-	SDL_Window * m_sdl_window;
-	SDL_GLContext m_glcontext;
+	SDL_Window* sdl_window_;
+	SDL_GLContext gl_context_;
+
 	/// A RenderTarget for screen_. This is initialized during init()
-	std::unique_ptr<RenderTarget> m_rendertarget;
+	std::unique_ptr<RenderTarget> render_target_;
+
 	/// This marks the complete screen for updating.
-	bool m_requires_update;
+	bool requires_update_;
 
 	/// Non-volatile cache of independent images.
 	std::unique_ptr<ImageCache> image_cache_;
 
 	/// This holds all animations.
 	std::unique_ptr<AnimationManager> animation_manager_;
+
+	/// Screenshot filename. If a screenshot is requested, this will be set to
+	/// the requested filename. On the next frame the screenshot will be written
+	/// out and this will be clear()ed again.
+	std::string screenshot_filename_;
 };
 
 extern Graphic * g_gr;

=== modified file 'src/graphic/minimap_renderer.cc'
--- src/graphic/minimap_renderer.cc	2016-01-08 21:00:39 +0000
+++ src/graphic/minimap_renderer.cc	2016-01-19 07:44:26 +0000
@@ -25,6 +25,7 @@
 #include "economy/flag.h"
 #include "economy/road.h"
 #include "graphic/graphic.h"
+#include "graphic/image_io.h"
 #include "graphic/texture.h"
 #include "logic/field.h"
 #include "logic/map.h"
@@ -263,5 +264,5 @@
 
 	// Render minimap
 	std::unique_ptr<Texture> texture(draw_minimap(egbase, player, viewpoint, layers));
-	g_gr->save_png(texture.get(), streamwrite);
+	save_to_png(texture.get(), streamwrite, ColorType::RGBA);
 }