Handle VR submit depth and pose in the windows/unix marshalling layer

[twhitehead]

Add support for the flags Submit_TextureWithPose and/or Submit_TextureWithDepth. This is required for LibreVR Revive to work and it should also close issue #4121, which apparently affects at least GTA V in VR and the Red Dead Redemption 2 VR mod.

On the actual patch, I could push the arrayLayers and TransitionSurfaceLayout bits into vrclient_translate_surface_dxvk too as these operations are common to all calls to vrclient_translate_surface_dxvk. It would be a bit more of a rewrite though, is I didn't want to go ahead and do it unless you actually wanted it. Maybe best to do that as a followup commit if you want it.

As a side note, it would be great if you could accept my wine pull request too to solve the other missing piece for LibreVRRevive. It has already been accepted into upstream and GE Proton.

[twhitehead]

@rbernon occurred to me that my pull requests might just be sitting because they aren't on the correct radars... 😄

[rbernon]

What about that comment now? Is this something that was missing from the DXVK unwrap, or is it still garbage? If this is still garbage we can't safely access it.

[twhitehead]

That is a good question. I don't have No Man Sky (No Man's Sky?) to test if it works or not. I did build build a redist proton with these commits though that someone could test.

I can see a couple of options

1. No Man Sky works, so we are good as is.
2. Let the developers of No Man Sky know they are incorrectly using Submit_TextureWithDepth as they are not submitting with a depth texture so they fix it.
3. If it is possible to determine if the application is No Man Sky (an if on the SteamAppId or something?), make the unpack conditional on that.
4. Don't unpack the just depth case.

This last one would still unpack Just pose or pose and depth (the two cases I am actually interested in), so, from my perspective, it would be okay. Although it does seem kind of wrong as it would then be broken for any other application that actually does use Submit_TextureWithDepth properly.

[rbernon]

Okay, thanks for the clarifications. I'm not the original author here, so I'd rather keep it like it is unless it's fixed. I can probably test whether it is or not, just need to find time for that.

Thanks for the PR, it looks okay from a first quick glance. Though it might take a moment until it lands.

[twhitehead]

No problem. Thanks very much for looking at it. 👍

[gofman]

What about that comment now? Is this something that was missing from the DXVK unwrap, or is it still garbage? If this is still garbage we can't safely access it.

No Man's Sky is a Vulkan game, no dxvk unwrap is used. I think it still passes broken data here (at least we had seen that a few weeks ago I guess), and it is still not used by the compositor. I suppose only the first patch is needed for the games to work? I think we can leave that without unwrapping (with some warn maybe), but we can't unconditionally unwrap.

[gofman]

What I am trying to say that you probably don't really need to pass depth texture anyhow. What I suppose is actually fixing the game is passing pose and handling the flag combination. Can you please check if the game works if instead of unwrapping depth texture you just pass the null handle it still works? I'd expect it so, and then we could get rid of most of the code in those patches, also avoiding breaking NMS on the way.

[gofman]

On a separate note, are you checking this with Steam VR or some custom compositor?

[gofman]

To give a bit more context, ValveSoftware/openvr#825 (depth texture used to be plainly ignored on SteamVR). That's why passing invalid data there doesn't break things, and why I am curious how actual texture unwrapping can change anything WRT the mentioned games.

[twhitehead]

The particular application I was looking at was Revive. It is a dll that you inject into an Oculus application. It hooks the Oculus API calls and translates them to OpenVR calls instead. There is a fairly extensive list of titles that it works for.

Right now it just does Submit_TextureWithPose. The author (@CrossVR) said

The flag is necessary to ensure stable reprojection. To closely align with the Oculus API Revive does its own pose prediction for each frame. The pose that was used to render the frame is then sent to the compositor to use for reprojection.

There is also a ENABLE_DEPTH_SUBMIT define in the code that will cause it to do Submit_TextureWithDepth too . Currently this isn't enabled by default. I don't know if it will be at some point or not. I figured I might as well do them all while I was at it though as the hard part was getting up to speed on the code, getting it accepted, etc., not doing the actual patch.

With regard to other possible titles, googling for fixme:vrclient:ivrcompositor_submit_dxvk Unhandled flags 0x8 (Submit_TextureWithPose) gives reports that mention L.A. Noire: The VR Case Files and various VR mods (GTA5 VR, Red Dead Redemption 2 VR, CyberPunk 2077). Doing the same for flags 0x10 (Submit_TextureWithDepth) and 0x18 (Submit_TextureWithPose | Submit_TextureWithDepth) doesn't return any hits.

I am just using the Steam VR compositor. From what you are saying though, it even if Revive did do Submit_TextureWithDepth at some point, unpacking wouldn't matter as it isn't used.

[gofman]

Thanks!

So if I understand correctly, from what you are saying nothing really depends on actually unwrapping the textures. Also, as I noted, SteamVR seems to ignore depth texture anyway, doesn't matter whether the application passes that or not.

So could you please remove all the code related to actual texture unwrapping from PR and just handle flags and their combination, along with correctly passing the pose? Submit_TextureWithDepth and both pose in depth can be handled, you just don't need to actually mind the texture. That will remove the majority of most complicated and regression prone code from the MR. Then I'd look at the final version in more details for nits and we could get that in.

If SteamVR gets actual support for depth texture we might get back to correctly delivering the data, before that, even regardless of NMS, it doesn't make much sense to add effectively dead code which we can't even test.

[twhitehead]

On a slightly different note, would you happen to know if there is someone in particular I should CC on my ValveSoftware wine pull request.

It just adds a missing case to the certificate handling code that is required for the Oculus runtime dll to authenticate. It has already been accepted in upstream wine and GE Proton, so it should be pretty non-controversial.

[rbernon]

Yeah, I can probably do that especially as it's upstream already. Sorry for the delay, I don't monitor PR here very closely.

[twhitehead]

I believe that should be much more to your liking. If you want, I also have these commits that I could add

• make set_skybox_d3d11_init only perform a conversion if all the textures are TextureType_DirectX as this is assumed by the conversion code, and the deinit part only runs if they all are
• remove 54 lines of duplicated translation code by pushing it into the common vrclient_translate_surface_dxvk rountine

If you do, just let me know, and I will push again.

[gofman]

If some apps are using (or might start using soon) Submit_TextureWithDepth, (Submit_TextureWithDepth | Submit_TextureWithPose) I think it is cleaner to do something like this in load_compositor_texture_dxvk() to clear the flag to Vulkan backend instead of passing undefined data (even though it currently ignores it):

    if (*flags & Submit_TextureWithDepth)
    {
        WARN( "Ignoring depth.\n" );
       *flags &= ~Submit_TextureWithDepth;
    }

    if ( *flags & Submit_TextureWithPose )
        state->texture.pose.mDeviceToAbsoluteTracking = ((w_VRTextureWithPose_t*)texture)->mDeviceToAbsoluteTracking;

Since VRTextureWithPoseAndDepth_t is inherited from VRTextureWithPose_t we don't have to mind input VRTextureWithPoseAndDepth_t layout.

A couple of nits about the rest:

 >   union
 >   {
 >       w_Texture_t texture;
 >       w_VRTextureWithPose_t pose;
>    } texture;

I'd rename pose to texture_with_pose.

...

>    state->texture.texture = vrclient_translate_texture_dxvk( texture, &vkdata, state->dxvk_surface,
>            &state->dxvk_device, &state->image_layout, &image_info );

Please don't change formatting here (before the patch the second line is aligned differently.

...
>    state->texture.texture.handle = &state->vkdata;
>
>    if ( *flags & Submit_TextureWithPose )

Spurious spaces here (after '(' and before ')').

>        state->texture.pose.mDeviceToAbsoluteTracking = ((w_VRTextureWithPose_t*)texture)->mDeviceToAbsoluteTracking;

You are assuming here that &state->texture.texture == &state->texture.pose.[texture] (i. e., that pose is inherited from texture). Which is technically correct but hard to read and a bit fragile. I know it is inconvenient to do strtaightforward with how it is currently done with returning texture by value from vrclient_translate_texture_dxvk. But I'd at least make it explicit, e. g., like *(w_VRTextureWithPose_t *)&state->texture.texture) = ... Or otherwise each time I look at such code I have to think of how that assignment to pose gets passed anywhere at all from here.

[twhitehead]

Thanks very much for the feedback. I have made all the changes.

One last thing I was wondering is, given your comment about preferring the cast for being more explicit, would you prefer me to replace

struct submit_state
{
    union
    {
        w_Texture_t texture;
        w_VRTextureWithPose_t texture_with_pose;
    } texture;
    ...
};

static const w_Texture_t *load_compositor_texture_dxvk( uint32_t eye, const w_Texture_t *texture, uint32_t *flags,
                                                        struct submit_state *state )

with just the larger pose types

{
    w_VRTextureWithPose_t texture_with_pose;
    ...
};

static const w_Texture_t *load_compositor_texture_dxvk( uint32_t eye, const w_VRTextureWithPose_t Texture_t *texture_with_pose, uint32_t *flags,
                                                        struct submit_state *state )
...

and do explicit casts to the smaller types everywhere they are used?

Basically, what I am thinking, is like the way things are done in unix_vrcompositor_manual.c. For example

static u_Texture_t *unwrap_submit_texture_data( const w_VRTextureWithPoseAndDepth_t *w_texture, uint32_t flags, u_VRTextureWithPoseAndDepth_t *u_texture,
                                                u_VRVulkanTextureArrayData_t *u_vkdata, u_VRVulkanTextureData_t *u_depth_vkdata )
{
    switch (flags & (Submit_TextureWithPose | Submit_TextureWithDepth))
    {
    default:
        *(u_Texture_t *)u_texture = unwrap_texture_data( (const w_Texture_t *)w_texture, flags, u_vkdata );
        break;
    case Submit_TextureWithPose:
        *(u_VRTextureWithPose_t *)u_texture = unwrap_texture_with_pose( (const w_VRTextureWithPose_t *)w_texture, flags, u_vkdata );
        break;
    case Submit_TextureWithDepth:
        *(u_VRTextureWithDepth_t *)u_texture = unwrap_texture_with_depth( (const w_VRTextureWithDepth_t *)w_texture, flags, u_vkdata, u_depth_vkdata );
        break;
    case Submit_TextureWithPose | Submit_TextureWithDepth:
        *u_texture = unwrap_texture_with_pose_and_depth( w_texture, flags, u_vkdata, u_depth_vkdata );
        break;
    }

    return (u_Texture_t *)u_texture;
}

[gofman]

Thanks a lot! I've pushed that to Proton, it is now in Experimental [bleeding-edge] branch: https://github.com/ValveSoftware/Proton/commits/experimental-bleeding-edge-8.0-66940-20231204-p831858-w8b5ab8-d1b31aa-v04cad9 . From there it should trip to regular Experimental on the next Experimental release (unless some regression spotted) and then eventually to stable.

If you think it worth cleaning up some stuff around shuffling those textures and are interested to proceed you are wellcome but I think in any case it is unrelated to the present PR and would be better to do and discuss in a separate. But one thing I'd start from is to stop returning texture by value from vrclient_translate_texture_dxvk(). If you open something like that, would you please ping me there?

[twhitehead]

Thank you very much! I can open a new pull request for the cleanup.

I'll start with that return and just do one thing at a time.