Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DISTS: Fix OpenGL renderer for builds with MSYS2 #1782

Conversation

@antoniou79
Copy link
Contributor

commented Aug 1, 2019

DISTS: Fix OpenGL renderer for MSYS2/Mingw(64 or 32)

The issue pertains to MSYS2 adding a default manifest file (default-manifest.o) to the executable

The bug is for PC systems with GPU drivers that were not properly supported for Windows 10 systems, like Intel HD Graphics series 1rst and 2nd generations. In those systems, launching a game in ScummVM (built with MSYS2/Mingw) with the OpenGL renderer would cauase the game screen to be a white blank image, and various warnings would be output to the console eg.
"WARNING: GL ERROR: GL_INVALID_ENUM on glTexSubImage2D(0x0DE1, 0, 0, area.top, src.w, area.height(), _glFormat, _glType, src.gere.cpp:167)!"

This was due to MSYS2/Mingw builds trying to load the (poorly supported) GPU driver, while "advertising" support for Windows 10 in their embedded default Manifest file. Hence, the GPU driver dll (eg ig4icd64.dll for Intel HD graphic series) would be unloaded, causing the bug.

More information is available in the following links:

DISTS: Fix OpenGL renderer issue for builds with MSYS2/Mingw64 or MSY…
…S2/Mingw32

The issue pertains to MSYS2 adding a default manifest file (default-manifest.o) to the executable

The bug is for PC systems with GPU drivers that were not properly supported for Windows 10 systems, like
Intel HD Graphics series 1rst and 2nd generations. In those systems, launching a game in ScummVM (built with MSYS2/Mingw)
with the OpenGL renderer would cauase the game screen to be a white blank image, and various warnings would be output to the console:
eg.
"WARNING: GL ERROR: GL_INVALID_ENUM on glTexSubImage2D(0x0DE1, 0, 0, area.top, src.w, area.height(), _glFormat, _glType, src.gere.cpp:167)!"
This was due to MSYS2/Mingw builds trying to load the (poorly supported) GPU driver while advertising support for Windows 10 in their
embedded default Manifest file. Hence, the GPU driver dll (eg ig4icd64.dll) would be unloaded, causing the bug.
More information is available in the following links:
https://github.com/pal1000/save-legacy-intel-graphics
LWJGL/lwjgl#119
msys2/MSYS2-packages#454
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69880
@lotharsm

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Can you verify that the change doesn't break compilation with Visual Studio? IIRC @SupSuper had a problem compiling ScummVM with Visual Studio when I simply added a manifest file the last time.

@antoniou79

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Can you verify that the change doesn't break compilation with Visual Studio? IIRC @SupSuper had a problem compiling ScummVM with Visual Studio when I simply added a manifest file the last time.

Not soon. I don't use Visual Studio. Could someone else try and verify this?

@sluicebox

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

It was me! #1541 (comment)

The same applies here, create_project needs to be updated to set the visual studio project setting to not generate a manifest, which it does by default, otherwise the linker will bomb because one already exists. We didn't get that far on the other PR, but I can take a look later today to see what to change and test that.

The resource script should use the ID CREATEPROCESS_MANIFEST_RESOURCE_ID like in the other PR. (it's in WinUser.h)

That bare-bones manifest file is good and exactly what Visual Studio defaults to generating. Having this in place will make it easy to play around and test other manifest settings that come up in things like the DPI Wars, etc.

@sluicebox

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

We can also get rid of DISCARDABLE in the resource script, that hasn't had any effect since Win16.
https://docs.microsoft.com/en-us/windows/win32/menurc/common-resource-attributes

@antoniou79

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

We can also get rid of DISCARDABLE in the resource script, that hasn't had any affect since Win16.
https://docs.microsoft.com/en-us/windows/win32/menurc/common-resource-attributes

Should I remove all the DISCARDABLE instances there?

@sluicebox

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Let's leave those, I'll zap them in a later commit with an explanation and keep this one free of distractions.

The ID change looks good, I'll get create_project figured out later and tested against a few visual studios.

When this is done and the commits are squashed, I think it should be renamed to something like WIN32: Add Application Manifest and then mention in the commit message that it has the side effect of preventing the obscure bug in the msys2 build.

@sluicebox

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Here's the changes to create_project needed for visual studio compatibility:

https://gist.github.com/sluicebox/3e406fdac61abc5cdf95aab97f6c7897

I'm sure there are countless better ways contribute these three lines than a diff paste, sorry!

@SupSuper

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

Looks good! Every modern Windows application should have a manifest (MSVC adds one by default) as it makes them Vista-aware and gets rid of backwards compatibility hacks.

Tested it on MSVC and old Windows and there don't seem to be any breaking changes.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Great work! Creating a default (and custom, later on) manifest is a good way of controlling the behavior of the final executable.

Since this has been tested on different compilers and several versions of Windows without any adverse effects, it is good to be merged.

Merging

@bluegr bluegr merged commit e962374 into scummvm:master Aug 4, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.