fix: Use IUnityGraphicsVulkan::CommandRecordingState for commandbuffer

[karasusan]

This PR is reopening the previous one #888 . I'm checking platforms (Windows, Ubuntu, Android) on my devices.

• ✔️ Windows10 2023.1.8f1
• ✔️ Ubuntu 20.04 2022.3.6f1

@aet, @BrianHarrisonUnity I will be glad if you check it again.

[BrianHarrisonUnity]

Working good for me in 4k!

[aet]

Previously there was a framerate issue with 1080p60 on Linux, has it changed?

I'm not sure when it's suitable to try pushing forward my changes to the UnityGraphicsVulkan, there's been almost time to consider it in the past, but usually the timing ends up bad by absolutely everything changing in upstream and I'll need to start over and let the changes mature again. :)

I suspect the WaitSync part could still be blocking submissions longer than it needs to, my branch handles this a bit differently but I don't have time to PR all graphics changes yet as some changes for DX12/OpenGL are missing.

I could try to gather WaitSync specific parts into a changeset to try them out. Or I'll just rebase and try contributing them later

[aet]

@karasusan try cherry-picking https://github.com/aet/com.unity.webrtc/commit/efbcdb06e4f458b07f0a1fbe21263054027a1bcf and see how it works out for you :)

It's from a branch where the video frame scheduling/submission is changed, video uploading with sw/hw decoder optimizations, Unity render plugin state improvements with Vulkan and Vulkan specific cpu encoder optimizations. The cherry-picked changes should still work without them, but I didn't test it at all other than seeing it compile. I have some ideas how to improve the WaitSync implementation further, but hopefully this is enough.

[karasusan]

@aet OK, let me check.

[karasusan]

@aet I understood the main change in your commit is that you use condition_variable to call CommandRecordingState on the render thread. I'm not sure we should use CommandRecordingState like this, but it looks no problem for me.

[karasusan]

I am testing this change on Android and some crashes are occurred. I'll investigating the cause of issue.

[aet]

Ok! I've been using it on PC/Linux but it's definitely possible some of the behavior was finetuned to depend on changes not in the changeset.

When I was originally testing the original PR iteration, anything calling WaitSync is waiting up to 30ms by default and the way Vulkan IsFinished/WaitSync calling CommandRecordingState outside of the render thread, the call was likely to wait a long time before succeeding and eventually dropping frames with 60fps target. I'm not seeing frame drops with the condition_variable approach though it's far from optimal, but it's possible it's working for me because I've alterered the frame scheduling/submission as well. Does it work for you on PC/Linux?

[BrianHarrisonUnity]

Were you seeing the 30ms frame delay with the changes in this PR @aet? Or were you seeing 30ms frame delay from the original? I am surprised there is a substantial change between the two.

I like the changes @aet is proposing as the frame check is now happening in the rendering thread context, and we avoid sleeps, but I think we might want to issue a different event to do this check.

Here is the current event config:

            UnityVulkanPluginEventConfig batchUpdateEventConfig;
            batchUpdateEventConfig.graphicsQueueAccess = kUnityVulkanGraphicsQueueAccess_DontCare;
            batchUpdateEventConfig.renderPassPrecondition = kUnityVulkanRenderPass_EnsureOutside;
            batchUpdateEventConfig.flags = kUnityVulkanEventConfigFlag_EnsurePreviousFrameSubmission |
                kUnityVulkanEventConfigFlag_ModifiesCommandBuffersState;
            vulkan->ConfigureEvent(s_batchUpdateEventID, &batchUpdateEventConfig);

For the frame check, it's not touching the command buffer state. If we had a separate event to check firing constantly, then we could configure it like the following:

            UnityVulkanPluginEventConfig waitSyncEventConfig;  
            waitSyncEventConfig.graphicsQueueAccess = kUnityVulkanGraphicsQueueAccess_DontCare;  
            waitSyncEventConfig.renderPassPrecondition = kUnityVulkanRenderPass_DontCare;  
            waitSyncEventConfig.flags = 0;  
            vulkan->ConfigureEvent(s_waitSyncEventID, &waitSyncEventConfig);

Edit: Come to think of it, you would still need to update the state each encoding phase to have the correct frame number. You could issue additional waitSyncEvents, but not sure it would add much value. With the current changset it will update foreach track though.

Surprising that the proposed changes have an issue on Android only. I am very curious why! I tested the changes on Windows, and it's working fine for me. Ill set up a test on Linux as well to check.

[aet]

@BrianHarrisonUnity haven't had time unfortunately to test this PR, but I thought it was a rebased version of where we got off the last time 🙂

[karasusan]

@aet It works well on PC too, I will test on Linux tomorrow.