UPBGE: Fix screenshot of invalid framebuffer.

Previously the screenshot called from bge.render.makeScreenshoot() were
able to copy the framebuffer just after the buffer swap, this behviour
was very hazardous. During the swap the driver could do a real swap of
back buffer to front buffer or proceed a copy. In the case of copy all
is fine, but in the case of the swap the back buffer will be the front
buffer of the last frame which will create a one frame delay.
The workaround of these issue was to always read from the front buffer
but it didn't work with all driver excepted with the configuration of
the buildbot.

The final fix found is to delay screenshot. Instead doing the screenshot
at the exact moment the user called, we push the screenshot data in a
stack where all the screen shot are proeceeded at the end of the frame
where the back buffer is always valid.
The two advantages of doing this are: Always using a valid buffer, have
a non-black screenshot for the first frame.

The function managing the stack are RAS_ICanvas::AddScreenshot(...) and
RAS_ICanvas::FlushScreenshots().

doc/python_api/rst/bge.render.rst

@@ -169,7 +169,7 @@ Functions
 
 .. function:: makeScreenshot(filename)
 
-   Writes an image file with the current displayed frame.
+   Writes an image file with the displayed image at the frame end.
 
    The image is written to *'filename'*.
    The path may be absolute (eg. ``/home/foo/image``) or relative when started with

source/gameengine/BlenderRoutines/KX_BlenderCanvas.cpp

@@ -47,9 +47,6 @@
 
 #include "GHOST_IWindow.h"
 
-#include <assert.h>
-#include <iostream>
-
 extern "C" {
 #  include "WM_api.h"
 #  include "wm_cursors.h"
@@ -249,20 +246,13 @@ void KX_BlenderCanvas::SetMousePosition(int x, int y)
 
 void KX_BlenderCanvas::MakeScreenShot(const std::string& filename)
 {
-	unsigned int *pixeldata;
 	bScreen *screen = m_win->screen;
 
 	int x = m_area_rect.GetLeft();
 	int y = m_area_rect.GetBottom();
 	int width = m_area_rect.GetWidth();
 	int height = m_area_rect.GetHeight();
 
-	pixeldata = m_rasterizer->MakeScreenshot(x, y, width, height);
-	if (!pixeldata) {
-		std::cerr << "KX_BlenderCanvas: Unable to take screenshot!" << std::endl;
-		return;
-	}
-
 	/* initialize image file format data */
 	Scene *scene = (screen) ? screen->scene : nullptr;
 	ImageFormatData *im_format = (ImageFormatData *)MEM_mallocN(sizeof(ImageFormatData), "im_format");
@@ -279,6 +269,5 @@ void KX_BlenderCanvas::MakeScreenShot(const std::string& filename)
 	BLI_strncpy(path, filename.c_str(), FILE_MAX);
 	BLI_path_abs(path, KX_GetMainPath().c_str());
 
-	/* save_screenshot() frees dumprect and im_format */
-	save_screenshot(path, width, height, pixeldata, im_format);
+	AddScreenshot(path, x, y, width, height, im_format);
 }

source/gameengine/GamePlayer/GPG_Canvas.cpp

@@ -44,8 +44,6 @@
 #include "MEM_guardedalloc.h"
 #include "DNA_space_types.h"
 
-#include "CM_Message.h"
-
 GPG_Canvas::GPG_Canvas(RAS_Rasterizer *rasty, GHOST_IWindow *window)
 	: RAS_ICanvas(rasty),
 	m_window(window),
@@ -141,13 +139,6 @@ void GPG_Canvas::MakeScreenShot(const std::string& filename)
 	unsigned int dumpsx = GetWidth();
 	unsigned int dumpsy = GetHeight();
 
-	unsigned int *pixels = m_rasterizer->MakeScreenshot(0, 0, dumpsx, dumpsy);
-
-	if (!pixels) {
-		CM_Error("cannot allocate pixels array");
-		return;
-	}
-
 	// initialize image file format data
 	ImageFormatData *im_format = (ImageFormatData *)MEM_mallocN(sizeof(ImageFormatData), "im_format");
 	BKE_imformat_defaults(im_format);
@@ -157,8 +148,7 @@ void GPG_Canvas::MakeScreenShot(const std::string& filename)
 	BLI_strncpy(path, filename.c_str(), FILE_MAX);
 	BLI_path_abs(path, KX_GetMainPath().c_str());
 
-	/* save_screenshot() frees dumprect and im_format */
-	save_screenshot(path, dumpsx, dumpsy, pixels, im_format);
+	AddScreenshot(path, 0, 0, dumpsx, dumpsy, im_format);
 }
 
 void GPG_Canvas::Init()

source/gameengine/Ketsji/KX_KetsjiEngine.cpp

@@ -274,9 +274,13 @@ void KX_KetsjiEngine::EndFrame()
 
 	m_logger.StartLog(tc_rasterizer, m_kxsystem->GetTimeInSeconds());
 	m_rasterizer->EndFrame();
+
+	m_logger.StartLog(tc_logic, m_kxsystem->GetTimeInSeconds());
+	m_canvas->FlushScreenshots();
+
 	// swap backbuffer (drawing into this buffer) <-> front/visible buffer
 	m_logger.StartLog(tc_latency, m_kxsystem->GetTimeInSeconds());
-	m_rasterizer->SwapBuffers(m_canvas);
+	m_canvas->SwapBuffers();
 	m_logger.StartLog(tc_rasterizer, m_kxsystem->GetTimeInSeconds());
 
 	m_canvas->EndDraw();

source/gameengine/Rasterizer/RAS_ICanvas.cpp

@@ -40,6 +40,8 @@ extern "C" {
 #  include "IMB_imbuf_types.h"
 }
 
+#include "CM_Message.h"
+
 #include <stdlib.h> // for free()
 
 // Task data for saving screenshots in a different thread.
@@ -104,6 +106,28 @@ RAS_Rasterizer::HdrType RAS_ICanvas::GetHdrType() const
 	return m_hdrType;
 }
 
+void RAS_ICanvas::FlushScreenshots()
+{
+	for (const Screenshot& screenshot : m_screenshots) {
+		SaveScreeshot(screenshot);
+	}
+
+	m_screenshots.clear();
+}
+
+void RAS_ICanvas::AddScreenshot(const std::string& path, int x, int y, int width, int height, ImageFormatData *format)
+{
+	Screenshot screenshot;
+	screenshot.path = path;
+	screenshot.x = x;
+	screenshot.y = y;
+	screenshot.width = width;
+	screenshot.height = height;
+	screenshot.format = format;
+
+	m_screenshots.push_back(screenshot);
+}
+
 void save_screenshot_thread_func(TaskPool *__restrict UNUSED(pool), void *taskdata, int UNUSED(threadid))
 {
 	ScreenshotTaskData *task = static_cast<ScreenshotTaskData *>(taskdata);
@@ -122,21 +146,26 @@ void save_screenshot_thread_func(TaskPool *__restrict UNUSED(pool), void *taskda
 }
 
 
-void RAS_ICanvas::save_screenshot(const char *path, int dumpsx, int dumpsy, unsigned int *dumprect,
-                                  ImageFormatData *im_format)
+void RAS_ICanvas::SaveScreeshot(const Screenshot& screenshot)
 {
+	unsigned int *pixels = m_rasterizer->MakeScreenshot(screenshot.x, screenshot.y, screenshot.width, screenshot.height);
+	if (!pixels) {
+		CM_Error("cannot allocate pixels array");
+		return;
+	}
+
 	/* Save the actual file in a different thread, so that the
 	 * game engine can keep running at full speed. */
 	ScreenshotTaskData *task = (ScreenshotTaskData *)MEM_mallocN(sizeof(ScreenshotTaskData), "screenshot-data");
-	task->dumprect = dumprect;
-	task->dumpsx = dumpsx;
-	task->dumpsy = dumpsy;
-	task->im_format = im_format;
+	task->dumprect = pixels;
+	task->dumpsx = screenshot.width;
+	task->dumpsy = screenshot.height;
+	task->im_format = screenshot.format;
 
-	BLI_strncpy(task->path, path, FILE_MAX);
+	BLI_strncpy(task->path, screenshot.path.c_str(), FILE_MAX);
 	BLI_path_frame(task->path, m_frame, 0);
 	m_frame++;
-	BKE_image_path_ensure_ext_from_imtype(task->path, im_format->imtype);
+	BKE_image_path_ensure_ext_from_imtype(task->path, task->im_format->imtype);
 
 	BLI_task_pool_push(m_taskpool,
 	                   save_screenshot_thread_func,

source/gameengine/Rasterizer/RAS_ICanvas.h

@@ -131,6 +131,8 @@ class RAS_ICanvas
 	}
 
 	virtual void MakeScreenShot(const std::string& filename) = 0;
+	/// Proceed the actual screenshot at the frame end.
+	void FlushScreenshots();
 
 	virtual void GetDisplayDimensions(int &width, int &height) = 0;
 
@@ -148,6 +150,18 @@ class RAS_ICanvas
 	}
 
 protected:
+	struct Screenshot
+	{
+		std::string path;
+		int x;
+		int y;
+		int width;
+		int height;
+		ImageFormatData *format;
+	};
+
+	std::vector<Screenshot> m_screenshots;
+
 	int m_samples;
 	RAS_Rasterizer::HdrType m_hdrType;
 
@@ -158,19 +172,16 @@ class RAS_ICanvas
 	TaskPool *m_taskpool;
 	RAS_Rasterizer *m_rasterizer;
 
+	/** Delay the screenshot to the frame end to use a valid buffer and avoid copy from an invalid buffer
+	 * at the frame begin after the buffer swap. The screenshot are proceeded in \see FlushScreenshots.
+	 */
+	void AddScreenshot(const std::string& path, int x, int y, int width, int height, ImageFormatData *format);
+
 	/**
 	 * Saves screenshot data to a file. The actual compression and disk I/O is performed in
 	 * a separate thread.
-	 *
-	 * @param path name of the file, can contain "###" for sequential numbering. A copy of the string
-	 *                 is made, so the pointer can be freed by the caller.
-	 * @param dumpsx width in pixels.
-	 * @param dumpsy height in pixels.
-	 * @param dumprect pixel data; ownership is passed to this function, which also frees the data.
-	 * @param im_format image format for the file; ownership is passed to this function, which also frees the data.
 	 */
-	void save_screenshot(const char *path, int dumpsx, int dumpsy, unsigned int *dumprect,
-	                     ImageFormatData *im_format);
+	void SaveScreeshot(const Screenshot& screenshot);
 };
 
 #endif  // __RAS_ICANVAS_H__

source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLRasterizer.cpp

@@ -261,11 +261,7 @@ unsigned int *RAS_OpenGLRasterizer::MakeScreenshot(int x, int y, int width, int
 	unsigned int *pixeldata = nullptr;
 
 	if (width && height) {
-		pixeldata = (unsigned int*) malloc(sizeof(unsigned int) * width * height);
-		glReadBuffer(GL_FRONT);
 		glReadPixels(x, y, width, height, GL_RGBA, GL_UNSIGNED_BYTE, pixeldata);
-		glFinish();
-		glReadBuffer(GL_BACK);
 	}
 
 	return pixeldata;

source/gameengine/Rasterizer/RAS_Rasterizer.cpp

@@ -711,10 +711,6 @@ RAS_ISync *RAS_Rasterizer::CreateSync(int type)
 	}
 	return sync;
 }
-void RAS_Rasterizer::SwapBuffers(RAS_ICanvas *canvas)
-{
-	canvas->SwapBuffers();
-}
 
 const MT_Matrix4x4& RAS_Rasterizer::GetViewMatrix() const
 {

source/gameengine/Rasterizer/RAS_Rasterizer.h

@@ -532,11 +532,6 @@ class RAS_Rasterizer
 	 */
 	RAS_ISync *CreateSync(int type);
 
-	/**
-	 * SwapBuffers swaps the back buffer with the front buffer.
-	 */
-	void SwapBuffers(RAS_ICanvas *canvas);
-
 	/** Create display array storage info for drawing (mainly VBO).
 	 * \param array The display array to use vertex and index data from.
 	 * \param instancing True if the storage is used for instancing draw.