fix: Revert: Vulkan CPU texture read performance, improve Unity pipeline state integration

[karasusan]

related PR #975.

This PR is reverted this #967. I'm sorry to revert this which is efforted by @aet.

We are currently in the QA phase of package development and have tested and confirmed the issue under the following conditions:

• Use the Vulkan Graphic API
• Set Graphics Job enabled in Player Settings Window
• Build the native plugin for Release, not Debug
• Turn off Development Build in Build Settings Window

When testing video streaming with the PeerConnection sample, no video is received.

This issue is occurring since the https://github.com/Unity-Technologies/com.unity.webrtc/commit/665ad1ff2a9d6f113fda95d630249e29cae0970e. We have also confirmed that enabling VULKAN_USE_CRS resolves the issue. However, there is another problem when VULKAN_USE_CRS is enabled: performance may be degraded when using VSync. This performance issue is discussed #967.

[aet]

@karasusan I'm traveling until next week so might have limited resources available to check this, though bisecting process should be easy

[karasusan]

@aet After revert that PR, we will split the code that is no problem into multiple PRs and merging them. However, we want to release a new version this week, so these will not be included there.

Have a nice trip! 😃

[BrianHarrisonUnity]

@karasusan just to confirm, are we going back to the CRS implementation?

I remember the CRS version had an issue with VSync which is why #966 was originally reverted, but is it alright now in this version?

I can't verify since I was never able to replicate the vsync issue on my devices.

[karasusan]

@BrianHarrisonUnity No, we aren't going to back to the CRS version. We are going to back to #967 after fixing the Vulkan issue.

967 defines CRS but this is disabled by the VULKAN_USE_CRS.

[BrianHarrisonUnity]

@karasusan this PR appears to be reverting #967, which I think will bring us back to only using CRS (#966). When I look through this commit changeset I am seeing VULKAN_USE_CRS removed, and I believe we are back to the first CRS implementation that had the VSync issue.

Since we wanted to maintain both options, I believe #967 was essentially a revert of #966, but it includes the optional VULKAN_USE_CRS preprocessor flag to swap between the two.

[karasusan]

@BrianHarrisonUnity I understood you are saying. As you said, commandRecordingState is used in this PR. I tested this PR on Ubuntu and fixed the issue by increasing wait time in this commit 458c95a.

I think we should find a more fundamental solution to fix this, but I want to release this version first.

[BrianHarrisonUnity]

@karasusan Oh sorry, I misunderstood. I thought you meant we were going back to #967 in this PR because the graphics testing issue was fixed.

[karasusan]

@BrianHarrisonUnity Yes, #966 and #967 are causing problems respectively.

[BrianHarrisonUnity]

@karasusan with the graphics test fix, does #967 still have issues?

I think it's fine if we want to move forward with this version, but I thought the graphics test fix resolved the issue that was introduced with #967.

[karasusan]

@BrianHarrisonUnity

Is this? 67c631c

Let me check, just in case.

[BrianHarrisonUnity]

@karasusan Sorry I mixed that up, that fix was only for the graphics tests. I think you are right we should move forward with this PR.

fix/revert-vulkan is working correctly for me, but I still never figured out what the difference is between that version & #967.

[BrianHarrisonUnity]

Looks good to me!