supertuxkart / stk-code

The code base of supertuxkart
Other
4.45k stars 1.05k forks source link

Game crash after taking screenshot HD 4600 full screen #1818

Closed mathsoft-dev closed 9 years ago

mathsoft-dev commented 9 years ago

Hello, When you take a screenshot in full screen mode(doesn't occurs when windowed), the game crash with HD 4600 latest driver. With my nvidia card, no problems. The crash occurs also in latest STK official release. It enters : void IrrDriver::doScreenShot() m_video_driver->createScreenShot(); (COpenGLDriver::createScreeshot) and crashs in this function almost at the end by doing glReadPixels(0,0,ScreeSize.Width, ScreenSize.Height, fmt,type,pixels)

I debuged it and tried to fix it but wasn't able yet

auriamg commented 9 years ago

Hi, on which OS is this?

mathsoft-dev commented 9 years ago

Windows 8.1

mathsoft-dev commented 9 years ago

Hey, Here is some update, I continue investigating and found interesting things. Here is the code we are talking about in method COPenGLDriver::createScreenshot. if (pixels) { GLenum tgt=GL_FRONT; switch (target) { case video::ERT_FRAME_BUFFER: break; case video::ERT_STEREO_LEFT_BUFFER: tgt=GL_FRONT_LEFT; break; case video::ERT_STEREO_RIGHT_BUFFER: tgt=GL_FRONT_RIGHT; break; default: tgt=GL_AUX0+(target-video::ERT_AUX_BUFFER0); break; } glReadBuffer(tgt); glReadPixels(0, 0, ScreenSize.Width, ScreenSize.Height, fmt, type, pixels); testGLError(); glReadBuffer(GL_BACK); }

When we enter the switch case(target) at line 4652, tgt is set to GL_FRONT because we go in case video::ERT_FRAME_BUFFER. I found out in this website that "Intel graphics have historically always had problems with reading from, or drawing to, the front buffer and that we should use GL_BACK". http://gamedevelopment.games1234.net/view/63538897166780581733501/using-glreadbufferglreadpixels-returns-black-image-instead-of-the-actual-image-only-on-intel-cards

@vlj In fact, If we remove the line glReadBuffer(tgt); or set it to glReadBufer(GL_BACK), there is no more crash and the screenshot is taken and working. I'm not sure why this line is needed in the first place, I thought that glReadPixels(0, 0, ScreenSize.Width, ScreenSize.Height, fmt, type, pixels); was enough to take the screenshot ??

However, I'm not really familiar with OpenGL so I didn't submit a patch yet because I want to know why the switch case and GL_FRONT was needed to avoid introducing new problems ?

vlj commented 9 years ago

I didnt write the code to store the screenshot. But if it works I think it's a fine fix.

GL_FRONT is the buffer "returned" by the app to the OS and that will be presented on screen. The GL_BACK buffer is the buffer used to draw things. Each frame the front and back buffer are being swapped (that's double buffering) so that the user doesn't see "frame in progress". I don't know where the glread is taking place but if it's used before the backbuffer is cleared or after it is written to but not swapped, then it contains correct content (of course there might be a frame latency but it's unfortunatly unavoidable)

vlj commented 9 years ago

Is the bug fixed ?

mathsoft-dev commented 9 years ago

Yes! thanks for your feedback.