ValveSoftware
fshack: Resolve fbo for glCopyTexSubImage2D
GL_INVALID_OPERATION is generated if: the effective value of GL_SAMPLE_BUFFERS for the read framebuffer is one.
This API works by spec with a system multisampled renderbuffer BUT returns error with user-defined multisampled renderbuffers
MESA project introduced a workaround for Penumbra game: https://gitlab.freedesktop.org/mesa/mesa/-/commit/8ced03437d62b38dc2d8c1bd43c005aedcaf7f72
But it might break other games, so it need to be resolved in Proton layer
Note: Pure WINE does not have this issue, it's only Proton wine fail
Merged, thanks!
It should show up in bleeding-edge soon and in the very next experimental release.
My understanding is that we end up with a user-defined multisampled renderbuffer because of fshack in place of what normally is a system one.
@gofman has some concerns, especially around performance, with enabling this globally for each game that may not even need the hack. I've reverted the change for now and let you two discuss it out. Thanks!
Ultimately I need a bit of time for me to come to this and understand the issue.
Do you know how exactly we end up with multisampled read buffer with Proton / fshack while the game does not end up with it in upstream Wine? I'd think that shouldn't happen, and if it does somehow, then maybe that is the part we can attempt to fix instead.
UPDATE: Why I think it needs a more thorough understanding is:
- adding an extra GL function hook to fshack diff is unfortunate by itself and complicates the thing, so we need good reason to do it;
- there is also glCopyTextureSubImage2D function which would also need that probably? Not immediately sure, but there might be something else;
- if wrongly getting multisampled read buffer ends up being unavoidable with fshack for some reason we will want to at least make sure that the given read buffer is resolved just once and not on every glTexSubImage. Maybe at the moment of such read buffer binding which would also avoid the need to override texture copy functions.
So I took a look at the game and it doesn't do anything with frame buffers having only the default multisampled one from which it blits to texture with glCopyTextureSubImage2D. Indeed the only difference which fshack is introducing is that it sets a non-default framebuffer, and default framebuffer is special in a way that autoresolves are allowed from it but not from user (that is, having the name other than 0) framebuffers.
Unfortunately I don't see so far any good way of solving that in general with fshack. Note that even performance aside the implementation in this patch won't provide a correct behaviour as it doesn't mind user framebuffers (which, if multisampled, should probably fail the same way, and may even have a different size). That part could be fixed, also framebuffer blit can maybe be performed for only a part of framebuffer.
But more important is that there is much more functions which would need such a fixup to make it correct, at least: glCopyTextureSubImage2D, glCopyTextureSubImage3D, glCopyTexSubImage3D, glReadPixels (and maybe I missed some). That is rather lot of Proton specific function overrides. Doing that just for glCopyTexSubImage2D which is known to only fix one game which already got a workaround in Mesa is not obviously useful.
Ideally we'd have some way to enable the newly added allow_multisampled_copyteximage Mesa option from Proton through environment variable or somehow easier than getting a custom copy of dri conf file. But I'd be hesitant to do even that by default as it doesn't do a fully correct thing with allowing texture copies for all the user framebuffers and not just fshack special default replacement one.
So my suggestion is to leave it until we have at least one game besides already worked around in Mesa which needs something like this.
If there are other ideas how to make fshack behaviour correct here without redefining a lot more OpenGL functions in fshack that could be interesting even without finding games which need it first.
So I came across another game which hits similar issue (Star Wars Knights of the Old Republic II: The Sith Lords). Unfortunately, as I feared in the previous comment, this PR as is doesn't help the game, it hits the same issue with different functions: glCopyTexImage2D, glReadPixels. The workaround added to Mesa doesn't fully work as well, as it doesn't consider all the related functions (glReadPixels in this case). And workaround in Mesa is not fully helping here anyway as the issue is also on Nvidia, and I'd personally wouldn't pursue perfecting it.
Ironically, there is GL extension EXT_multisampled_render_to_texture which allows to do exactly what we need in fshack, available on Mesa and Nvidia but... only for GLES / not on desktop. As I understand there are some reasons for that (even though the extension just works on desktop if enabled).
So I didn't find anything better than to proceed along the lines of this commit. I pushed an updated version of the commit from this PR to Proton Experimental (currently [bleeding-edge] branch, here is the commit: https://github.com/ValveSoftware/wine/commit/6b40f84abf032f13d3227a54095e439eb9bd1cad) followed up by doing the same for glCopyTexImage2D, glReadPixels. My changes in the patch are:
- factor out resolve_fs_hack_fbo() so it can be reused for other functions, reducing the overall boilerplate;
- also track fs_hack_needs_resolve in wgl_context to avoid getting gl drawable each time even if no resolve is needed (also avoiding leaking a reference to GL drawable present in the original patch);
- don't resolve if fs_hack is not enabled at all;
- don't resolve and don't erroneously bind resolve fbo if fs_hack fbo is not selected as read buffer.