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

Wolfenstein: The Old Blood tracing broken due to abuse of GL_MAP_PERSISTENT_BIT mappings #900

Closed
zmike opened this issue Oct 3, 2023 · 19 comments

Comments

@zmike
Copy link
Contributor

zmike commented Oct 3, 2023

When replaying, the intro move plays fine, the title/menu screen is black, and gameplay looks like this:
image

@jrfonseca
Copy link
Member

It's impossible to say what could be going on.

Are there any apitrace warnings when tracing/replaying?

@zmike
Copy link
Contributor Author

zmike commented Oct 4, 2023

Sorry, I was working on getting a trace uploaded but I got distracted. Here's a trace.

When replaying I get a ton of errors like

Mesa: error: GL_INVALID_OPERATION in glBindVertexArray(non-gen name)

@jrfonseca
Copy link
Member

glBindVertexArray from call no 1652114 succeeds, while 1652185 fails, the only thing changed being that's its on a different context:

1652114 @1 glBindVertexArray(array = 2)
1652185 @2 glBindVertexArray(array = 2)

VAOs are per context objects, however VAO 2 is never created on the context used by thread @2:

424866 @1 glXMakeCurrent(dpy = 0x7f3a25a0, drawable = 65012140, ctx = 0x7f00640b1c50) = True
424869 @0 glXMakeCurrent(dpy = 0x7f3a25a0, drawable = 65012140, ctx = 0x7f0098bd9aa0) = True
426387 @0 glGenVertexArrays(n = 1, arrays = &1)
427496 @1 glGenVertexArrays(n = 1, arrays = &1)
427517 @1 glGenVertexArrays(n = 1, arrays = &2)

591379 @2 glXMakeCurrent(dpy = 0x7f3a25a0, drawable = 65012222, ctx = 0x7f001c0b1cf0) = True
...


So this looks like a WINE error.

I presumed that the screenshot you showed does not happen when running WINE directly.  Is that truly the case?

If this indeed renders fine in WINE, then the problem is not these `glBindVertexArray`...

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

Indeed it renders okay in the game.

@jrfonseca
Copy link
Member

jrfonseca commented Oct 5, 2023

I think I know the rendering is not correct. WINE (or the app, if it uses GL API directly?) is using GL_MAP_PERSISTENT_BIT, but the writes are not being trapped:

426111 glNamedBufferStorageEXT(buffer = 3, size = 10485760, data = NULL, flags = GL_MAP_READ_BIT | GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT)
426112 glMapNamedBufferRangeEXT(buffer = 3, offset = 0, length = 10485760, access = GL_MAP_READ_BIT | GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT) = 0x7f004dc16000
[...]

426339 glBindBuffer(target = GL_PIXEL_UNPACK_BUFFER, buffer = 3)
426340 glCompressedTextureSubImage2DEXT(texture = 50, target = GL_TEXTURE_2D, level = 0, xoffset = 0, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT, imageSize = 8192, bits = 0x8000)
426341 glCompressedTextureSubImage2DEXT(texture = 49, target = GL_TEXTURE_2D, level = 0, xoffset = 0, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = NULL)
426342 glCompressedTextureSubImage2DEXT(texture = 51, target = GL_TEXTURE_2D, level = 0, xoffset = 0, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0x4000)
426343 glCompressedTextureSubImage2DEXT(texture = 50, target = GL_TEXTURE_2D, level = 0, xoffset = 128, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT, imageSize = 8192, bits = 0x12000)
426344 glCompressedTextureSubImage2DEXT(texture = 49, target = GL_TEXTURE_2D, level = 0, xoffset = 128, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0xa000)
426345 glCompressedTextureSubImage2DEXT(texture = 51, target = GL_TEXTURE_2D, level = 0, xoffset = 128, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0xe000)
426346 glCompressedTextureSubImage2DEXT(texture = 50, target = GL_TEXTURE_2D, level = 0, xoffset = 256, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT, imageSize = 8192, bits = 0x1c000)
426347 glCompressedTextureSubImage2DEXT(texture = 49, target = GL_TEXTURE_2D, level = 0, xoffset = 256, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0x14000)
426348 glCompressedTextureSubImage2DEXT(texture = 51, target = GL_TEXTURE_2D, level = 0, xoffset = 256, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0x18000)
426349 glCompressedTextureSubImage2DEXT(texture = 50, target = GL_TEXTURE_2D, level = 0, xoffset = 384, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT1_EXT, imageSize = 8192, bits = 0x26000)
426350 glCompressedTextureSubImage2DEXT(texture = 49, target = GL_TEXTURE_2D, level = 0, xoffset = 384, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0x1e000)
426351 glCompressedTextureSubImage2DEXT(texture = 51, target = GL_TEXTURE_2D, level = 0, xoffset = 384, yoffset = 0, width = 128, height = 128, format = GL_COMPRESSED_RGBA_S3TC_DXT5_EXT, imageSize = 16384, bits = 0x22000)

One would normally expect a few fake memcpy calls to the 0x7f004dc16000 - ... range, but none can be seen. So all these texture uploads using UBOs are using zeroed/uninitialized memory.

Apitrace has code to trap writes to persistent memory, but IIUC, the problem here is that apitrace only does that with GL_MAP_COHERENT_BIT is set, but WINE is not setting GL_MAP_COHERENT_BIT.

https://registry.khronos.org/OpenGL-Refpages/gl4/html/glMapBufferRange.xhtml states:

GL_MAP_COHERENT_BIT indicates that a persistent mapping is also to be coherent. Coherent maps guarantee that the effect of writes to a buffer's data store by either the client or server will eventually become visible to the other without further intervention from the application. In the absence of this bit, persistent mappings are not coherent and modified ranges of the buffer store must be explicitly communicated to the GL, either by unmapping the buffer, or through a call to glFlushMappedBufferRange or glMemoryBarrier.

It's true that Apitrace actually doesn't handle the glMemoryBarrier case, however WINE is neither calling glFlushMappedBufferRange nor glMemoryBarrier.

Therefore this is a WINE bug. It should be setting GL_MAP_COHERENT_BIT.

You should be able to hack Apitrace source to treat GL_MAP_PERSISTENT_BIT the same as GL_MAP_COHERENT_BIT and verify it indeed leads to textures uploads to be correctly recorded.

WINE (or the app) might also have settings to avoid persistent/coherent mappings?

@jrfonseca jrfonseca changed the title Wolfenstein: The Old Blood tracing broken Wolfenstein: The Old Blood tracing broken due to abuse of GL_MAP_PERSISTENT_BIT mappings Oct 5, 2023
@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

Interesting analysis. According to pcgamingwiki the game is natively using GL, so this would be an application bug. I'll test and report back.

@jrfonseca
Copy link
Member

Yeah, from what I can tell from WINE's source code, it always uses GL_MAP_PERSISTENT_BIT | GL_MAP_COHERENT_BIT, so it's consistent with an app bug.

You can try to modify the application source code to treat GL_MAP_PERSISTENT_BIT as if GL_MAP_COHERENT_BIT was also set, to force a working trace. (We might even have this as a environment or compile define setting in case other apps have the same bug.)

But this is illegal/incorrect use of OpenGL API, and might cause certain drivers which treat coherent memory differently to misrender.

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

I can't modify the application since it's a proprietary game, but I can hack up apitrace for testing.

There's a lot of references to GL_MAP_PERSISTENT_BIT in wrappers/: should I be changing all of them or just some?

@jrfonseca
Copy link
Member

jrfonseca commented Oct 5, 2023

This is a 8 year old game. Perhaps ID's engine has been fixed since (or abandon GL altogether?)

Search for GL_MAP_COHERENT_BIT instead:

$ git grep GL_MAP_COHERENT_BIT wrappers/
wrappers/gltrace.py:            print('    if ((access_flags & GL_MAP_COHERENT_BIT) && (access_flags & GL_MAP_WRITE_BIT)) {')
wrappers/gltrace.py:            print('    if ((access_flags & GL_MAP_COHERENT_BIT) && (access_flags & GL_MAP_WRITE_BIT)) {')
wrappers/gltrace.py:            print('    if ((access_flags & GL_MAP_COHERENT_BIT) && (access_flags & GL_MAP_WRITE_BIT)) {')
wrappers/gltrace.py:            print(r'    if ((flags & GL_MAP_COHERENT_BIT) && (flags & GL_MAP_WRITE_BIT)) {')
wrappers/gltrace.py:            print(r'    if ((access & GL_MAP_COHERENT_BIT) && (access & GL_MAP_WRITE_BIT)) {')

You want to replace every instance of GL_MAP_COHERENT_BIT with GL_MAP_PERSISTENT_BIT. Or at least that's the general idea.

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

Oh okay, I misunderstood.

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

Yep, that fixed it!

@jrfonseca
Copy link
Member

Cool. Glad to hear it.

If this happens again with another app I might make Apitrace's default behavior to treat GL_MAP_PERSISTENT_BIT without GL_MAP_FLUSH_EXPLICIT_BIT the same way as if GL_MAP_COHERENT_BIT, as I have to admit that's more likely to be intended...

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

I'm guessing this will also happen with Wolfenstein: The New Order since it's using the same engine. Installing now to test.

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

Nope, TNO seems to run fine without those changes. Will test DOOM2016 next since that had a similar sort of issue.

@zmike
Copy link
Contributor Author

zmike commented Oct 5, 2023

Oh ho ho it fixes DOOM2016 too!

@zmike
Copy link
Contributor Author

zmike commented Oct 12, 2023

@jrfonseca What's going to be the resolution here?

@jrfonseca
Copy link
Member

jrfonseca commented Oct 16, 2023

To summarize, so far the apps affected are:

  • Wolfenstein: The Old Blood
  • Doom 2016

Right?

I'm not sure it's worth doing changing default behavior just for two apps.

3264525 is the best that can be done for now. If you want, we could also control the behavior via env var as opposed to source code modification. If it's worth it, we could even automatically detect based on the process name...

Once you verify that replacing #if 1 with #if 0 works for the games above after 3264525 I'll get it committed.

@zmike
Copy link
Contributor Author

zmike commented Oct 16, 2023

It's indeed just those two apps (that I've found so far).

I agree that changing default behavior may be overkill. I think automatically detecting it by process name would be great if it isn't too challenging. I can get the app names when I test your patch.

@zmike
Copy link
Contributor Author

zmike commented Oct 18, 2023

I can confirm that 3264525 with the #if 1 change resolves the issue.

  • Z:\home\zmike\.local\share\Steam\steamapps\common\Wolfenstein The Old Blood\WolfOldBlood_x64.exe
  • Z:\home\zmike\.local\share\Steam\steamapps\common\DOOM\DOOMx64.exe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants