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 double free in FSR2 destructor #95682

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Aug 17, 2024

Before this change, using FSR2 resulted in the following error when the effect was destroyed:

ERROR: Attempted to free invalid ID: 662734928609453
   at: _free_internal (servers/rendering/rendering_device.cpp:4957)

This happened because ACCUMULATE and ACCUMULATE_SHARPEN passes shared the same shader_version object but had different pipeline IDs. When version_free was called for ACCUMULATE pass, it destroyed pipelines created from that version, including the pipeline for the ACCUMULATE_SHARPEN pass.

Using a unique version could work around this problem, but it's easier to rely on version_free destroying the created pipelines through the dependency mechanism.

I've verified that FSR2 works correctly after this change (on TPS demo) and that the pipelines are all destroyed; during the call of version_free for accumulate pass, both the pipelines created for accumulate and accumulate_sharpen passes are destroyed; the call in accumulate_sharpen becomes a no-op.

@zeux zeux requested a review from a team as a code owner August 17, 2024 05:25
Before this change, using FSR2 resulted in the following error when the
effect was destroyed:

	ERROR: Attempted to free invalid ID: 662734928609453
	   at: _free_internal (servers/rendering/rendering_device.cpp:4957)

This happened because ACCUMULATE and ACCUMULATE_SHARPEN passes shared
the same shader_version object but had different pipeline IDs. When
version_free was called for ACCUMULATE pass, it destroyed pipelines
created from that version, including the pipeline for the
ACCUMULATE_SHARPEN pass.

Using a unique version could work around this problem, but it's easier
to rely on version_free destroying the created pipelines through the
dependency mechanism.
@Chaosus Chaosus added this to the 4.4 milestone Aug 17, 2024
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Thanks! Indeed, pipelines are cleaned up when a shader is freed. So there is little use in freeing them manually in destructors.

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 18, 2024
@akien-mga akien-mga merged commit 9cc53a1 into godotengine:master Aug 19, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@zeux zeux deleted the fsr2-fix-free branch August 21, 2024 20:27
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

None yet

4 participants