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

Fix to reduce the amount of allocations required when using MSVO and Dynamic Resolution #3442

Merged
merged 3 commits into from
Feb 25, 2021

Conversation

robinb-u3d
Copy link
Contributor

Purpose of this PR

This is a port of from the original Post Processing repo Unity-Technologies/PostProcessing#901 now that Unity has been fixed so that we can make these changes. See the CHANGELOG.md file changes for a description of the version requirements to make use of this bugfix. Fixes case 1285605.

The existing dynamic resolution strategy manually scales the temporary intermediate textures to the correct size required for the current dynamic resolution scale. This allows the effect to run correctly and for a given fixed scaling factor will keep the memory usage constant however if dynamic resolution is being scaled over time then every time it changes scaling factor the temporary intermediate textures will be reallocated causing an increase in memory pressure while this happens and possibly generating more VRAM fragmentation (depending on the platform).

This PR make the temporary intermediate textures be full size dynamic resolution textures themselves. The behavior of this PR and the existing behavior should match when dynamic resolution is not being used. However when dynamic resolution is being used this PR changes the following:

We now allocate the temporary intermediate textures at full size all the time so for a fixed dynamic resolution scaling of 0.5 this PR will increase VRAM usage as we will now allocate all textures at 1.0 size and then make sure of dynamic resolution to access a 0.5 scaled version using the same memory.
The temporary intermediate textures are now dynamic, this means they may have less chance of matching other temporary textures used by other systems and so increase the memory usage of this project.
Changing the dynamic scaling factor will no longer cause these textures to be reallocated thus VRAM usage should be consistent no matter what level of dynamic scaling factor is used and reduce memory fragmentation and allocation overheads.
Fixing this behavior showed up a bug in how Unity handles viewport setting when using dynamic resolution on some render targets on dynamic resolution platforms. This PR requires a fixed Unity version to work correctly otherwise the temporary intermediate textures are not rendered to correctly when the scale is not 1.0. These bugfixes have now landed into Unity see the CHANGELOG.md file.


Testing status

Testing with the reproduction project from case 1285605 on the original platform and monitoring the VRAM memory usage shows that the fixed version has a constant VRAM usage with the same visual effect compared against Post Processing V2 3.0.1.

Copy link
Contributor

@Chman Chman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll add the post-processing gfx tests back next week to make sure everything is alright but it's looking good to me.

@robinb-u3d
Copy link
Contributor Author

@Chman do you want me to wait for you to add the tests and have them pass before merging this in?

@Chman
Copy link
Contributor

Chman commented Feb 16, 2021

I would prefer we wait for the tests, yes. We'll try to get them up and running sometimes this week.

@robinb-u3d
Copy link
Contributor Author

@Chman do you have any updates on the progress of getting the tests up and running so we can move forward with this PR?

@Chman
Copy link
Contributor

Chman commented Feb 24, 2021

Yes, tests are in a branch that we're currently testing but we ran into some issues with Yamato etc so it's taking longer than it should :(

@sebastienlagarde sebastienlagarde marked this pull request as ready for review February 25, 2021 09:07
@sebastienlagarde sebastienlagarde merged commit 3806572 into master Feb 25, 2021
@sebastienlagarde sebastienlagarde deleted the ppv2/v2-msvo-dynamic-res-fix branch February 25, 2021 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants