Page MenuHomeWildfire Games

Alt + Tab in fullscreen mode crash
ClosedPublic

Authored by Angen on Jan 9 2018, 6:56 PM.

Details

Summary

Added detection for switching alt + tab while in fullscreen mode while in game.
Plus I had got error on minimize, because need_render did not catch g_app_minimized change, so I moved it to the condition too.

Test Plan

A bit hard, if you had not this kind of problem.
At least look if something is not broken with this patch :)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Angen created this revision.Jan 9 2018, 6:56 PM
Angen updated the Trac tickets for this revision.
Vulcan added a subscriber: Vulcan.Jan 9 2018, 7:55 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Does it solve the problem for you?

elexis added a subscriber: elexis.Jan 9 2018, 8:04 PM
elexis added inline comments.
source/main.cpp
374 ↗(On Diff #5195)

(x && y) || !y is the same as x || !y

Angen added a comment.Jan 9 2018, 8:11 PM

Does it solve the problem for you?

for me, yes.

Angen updated this revision to Diff 5215.Jan 10 2018, 3:29 PM

simplifying by elexis and variable cleanup

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Stan added a subscriber: Stan.Jan 11 2018, 8:52 AM
Stan added inline comments.
source/main.cpp
307 ↗(On Diff #5215)

Comments usually start with caps. I'm not sure that comment is really necessary though.

Could you say, why it's crashed for you?

source/main.cpp
372 ↗(On Diff #5215)

Why exactly this?

source/ps/VideoMode.cpp
403 ↗(On Diff #5215)

Methods starts with a capital letter too.

Angen marked an inline comment as done.Jan 21 2018, 10:58 AM

Could you say, why it's crashed for you?

!g_app_minimised:
In some way, I cant say why exactly, when I minimised my window, g_app_minimised was seted to true, but need_render already used old value of g_app_minimised. I guess that maybe window event runs in different thread as game itself, so when I minimised the window probably need_render was computed before the g_app_minimised changed in the memory, then window was minimised so there was not area where to render and it tries to do it. There is also chance that it is something wrong with specific graphic cards family.

To the alt + tab
(g_app_has_focus || !g_VideoMode.isInFullscreen()):
Maybe it is only windows thing, but when I debugged alt+tab, minimised window event was not used, so after alt+tab window is not minimised and therefore kept rendering, but since game was in fullscreen and not displayed, graphic card maybe could not find rendering area correctly, so it crashed.
The point of this solution is that if you are in fullscreen mode and game is not focused, you are not playing game but doing something else, so you do not see it, no point to render.

Imarok added a subscriber: Imarok.Jan 25 2018, 6:47 PM

Got someone to test it on a Intel HD530 and he confirmed, that this patch fixes the problem.

In D1212#50205, @Angen wrote:

Could you say, why it's crashed for you?

!g_app_minimised:
In some way, I cant say why exactly, when I minimised my window, g_app_minimised was seted to true, but need_render already used old value of g_app_minimised. I guess that maybe window event runs in different thread as game itself, so when I minimised the window probably need_render was computed before the g_app_minimised changed in the memory, then window was minimised so there was not area where to render and it tries to do it. There is also chance that it is something wrong with specific graphic cards family.
To the alt + tab
(g_app_has_focus || !g_VideoMode.isInFullscreen()):
Maybe it is only windows thing, but when I debugged alt+tab, minimised window event was not used, so after alt+tab window is not minimised and therefore kept rendering, but since game was in fullscreen and not displayed, graphic card maybe could not find rendering area correctly, so it crashed.
The point of this solution is that if you are in fullscreen mode and game is not focused, you are not playing game but doing something else, so you do not see it, no point to render.

So you can reproduce the crash? Could you get a stack trace of the crash?

Angen added a comment.Jan 27 2018, 6:22 PM

So you can reproduce the crash? Could you get a stack trace of the crash?

anything helpfull from stack:
pyrogenesis.exe!CGame::Update(const double deltaRealTime, bool doInterpolate) Line 422 C++

and this from output:
First-chance exception at 0x528D8120 (ig75icd32.dll) in pyrogenesis.exe: 0xC0000005: Access violation reading location 0x00000018.

In D1212#51129, @Angen wrote:

So you can reproduce the crash? Could you get a stack trace of the crash?

anything helpfull from stack:
pyrogenesis.exe!CGame::Update(const double deltaRealTime, bool doInterpolate) Line 422 C++
and this from output:
First-chance exception at 0x528D8120 (ig75icd32.dll) in pyrogenesis.exe: 0xC0000005: Access violation reading location 0x00000018.

Could you compile in the debug mode and get a full callstack?

Angen added a comment.Jan 28 2018, 6:16 PM

CallStack of alt+Tab error:

       pyrogenesis_dbg.exe!CRenderer::RenderSubmissions(const CBoundingBoxAligned & waterScissor) Line 1596	C++
	pyrogenesis_dbg.exe!CRenderer::RenderScene(Scene & scene) Line 1898	C++
	pyrogenesis_dbg.exe!CGameView::Render() Line 490	C++
	pyrogenesis_dbg.exe!Render() Line 219	C++
	pyrogenesis_dbg.exe!Frame() Line 378	C++
	pyrogenesis_dbg.exe!RunGameOrAtlas(int argc, const char * * argv) Line 590	C++
	pyrogenesis_dbg.exe!SDL_main(int argc, char * * argv) Line 632	C++
In D1212#51301, @Angen wrote:

CallStack of alt+Tab error:

pyrogenesis_dbg.exe!CRenderer::RenderSubmissions(const CBoundingBoxAligned & waterScissor) Line 1596	C++

L1596 is this part of CRenderer::RenderSubmissions

	if (m_Options.m_Silhouettes)
	{
		RenderSilhouettes(context);
	}

Can we find out which line therein is the one crashing?
Either by adding a debug_printf("hello world\n") before the statements, or by using the debugger differently.

Also can you test if it doesnt crash if silouettes are disabled?

Angen added a comment.Jan 29 2018, 4:22 PM

It crashes after this function

void CPostprocManager::ReleaseRenderOutput()

called as

	  if (m_Options.m_Postproc)
	  {
		m->postprocManager.ApplyPostproc();
		m->postprocManager.ReleaseRenderOutput();
	  }

in

void CRenderer::RenderSubmissions(const CBoundingBoxAligned& waterScissor)

Can you test if it doesnt crash if silouettes are disabled?

Angen added a comment.EditedJan 29 2018, 7:40 PM
In D1212#51519, @elexis wrote:

Can you test if it doesnt crash if silouettes are disabled?

It has nothing to do with silouettes.
Just now I enabled everything except post processing and no crash.

Ten I disabled everything and enabled postprocesing, it crashed.

Stan added a comment.Feb 11 2018, 11:21 PM

Just tested with someone called Jongleur on #0ad and the patch indeed fixes the bug. http://irclogs.wildfiregames.com/2018-02/2018-02-11-QuakeNet-%230ad.log

Since only Intel HD 4000 users experience it, it must be a driver bug.
I agree with Vladislav that we should try to get a stacktrace to see which call exactly crashes.
Perhaps we can catch it differently.

Also maybe there is a newer driver and we can just tell people to install that?
If not, then I guess we can commit this if there is a comment added that this is needed for this specific graphics card family on windows,
so that we can maybe remove it eventually.

source/main.cpp
372 ↗(On Diff #5215)

The new condition says that if fullscreen mode is enabled, that it shouldn't render if the window isn't focused.
Since not everyone with windows gets that crash, it must be a driver bug.

Also, there will be a merge conflict with D1212. It would be great if someone can test that this still doesn't crash after we have committed that.

elexis retitled this revision from Alt + Tab in fullscreen mode to Alt + Tab in fullscreen mode crash.Feb 19 2018, 5:08 PM
Angen updated this revision to Diff 5846.Feb 20 2018, 8:45 PM
Angen updated this revision to Diff 5847.Feb 20 2018, 8:56 PM

comment removed

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/51/display/redirect

What's about callstack in the debug mode?

@echotangoecho proposed to detect the graphics card family in hwdetect.js and set a flag enabling the workaround.
The detection looks easy, just a string match. But I doubt that we want to add a JS interface function for this workaround.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/52/display/redirect

vladislavbelov requested changes to this revision.Feb 21 2018, 5:50 PM

First of all we need to investigate the problem source.

This revision now requires changes to proceed.Feb 21 2018, 5:50 PM
Angen added a comment.Feb 21 2018, 6:38 PM

Call stack from build game as debug is here

In D1212#51301, @Angen wrote:

CallStack of alt+Tab error:

       pyrogenesis_dbg.exe!CRenderer::RenderSubmissions(const CBoundingBoxAligned & waterScissor) Line 1596	C++
	pyrogenesis_dbg.exe!CRenderer::RenderScene(Scene & scene) Line 1898	C++
	pyrogenesis_dbg.exe!CGameView::Render() Line 490	C++
	pyrogenesis_dbg.exe!Render() Line 219	C++
	pyrogenesis_dbg.exe!Frame() Line 378	C++
	pyrogenesis_dbg.exe!RunGameOrAtlas(int argc, const char * * argv) Line 590	C++
	pyrogenesis_dbg.exe!SDL_main(int argc, char * * argv) Line 632	C++

More when it crashes is here:

In D1212#51500, @Angen wrote:

It crashes after this function

void CPostprocManager::ReleaseRenderOutput()

called as

	  if (m_Options.m_Postproc)
	  {
		m->postprocManager.ApplyPostproc();
		m->postprocManager.ReleaseRenderOutput();
	  }

in

void CRenderer::RenderSubmissions(const CBoundingBoxAligned& waterScissor)

and insurance about postprocessing

In D1212#51520, @Angen wrote:
In D1212#51519, @elexis wrote:

Can you test if it doesnt crash if silouettes are disabled?

It has nothing to do with silouettes.
Just now I enabled everything except post processing and no crash.
Ten I disabled everything and enabled postprocesing, it crashed.

In D1212#53949, @Angen wrote:

Call stack from build game as debug is here

In D1212#51301, @Angen wrote:

in

void CRenderer::RenderSubmissions(const CBoundingBoxAligned& waterScissor)

I mean on which line inside RenderSubmissions, could you modify the code like:

debug_printf("breakpoint");
if (m_Options.m_Silhouettes)
{
    debug_printf("breakpoint");
    RenderSilhouettes(context);
}
debug_printf("breakpoint");

And run it? We need to know a line.

Angen added a comment.EditedFeb 23 2018, 3:35 PM

As I said earlier. It is postprocessing.


And to it be more specific, it breaks after, not inside. Mystery.

Angen added a comment.EditedMar 4 2018, 1:38 PM

For info:
it is this code which is case of break

    pglBlitFramebufferEXT(0, 0, m_Width, m_Height, 0, 0, m_Width, m_Height,
GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT, GL_NEAREST);
vladislavbelov added inline comments.Mar 4 2018, 8:18 PM
source/main.cpp
372 ↗(On Diff #5847)

I think, the comment isn't right, because the !g_app_minimized part is valid, so it isn't the fix. I suggest to use comment like:

// We don't have to render an inactive fullscreen frame, because it can lead to errors for some graphic card families.

Or something like that, at least it needs to check a gramma.

source/ps/VideoMode.cpp
403 ↗(On Diff #5847)

This function doesn't change the object state, so it should be const.

Angen updated this revision to Diff 6046.Mar 6 2018, 7:34 PM

const function

Vulcan added a comment.Mar 6 2018, 7:41 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/164/display/redirect

Angen updated this revision to Diff 6096.Mar 9 2018, 10:45 PM

added emty lines around function

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/202/display/redirect

vladislavbelov accepted this revision.Mar 9 2018, 11:19 PM

I compiled and tested it. The game still works fine for me.

This revision is now accepted and ready to land.Mar 9 2018, 11:19 PM
This revision was automatically updated to reflect the committed changes.

@Angen, thank you for the patch! I think it's really helpful.