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

win-capture: Fix leaking framebuffers data #9313

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

sumydi
Copy link

@sumydi sumydi commented Jul 25, 2023

Description

The wrong linked list was used when removing framebuffer data. It generated memory leak, the linked list grew more and more involving performance degradation each time an application called the vkCmdBeginRenderPass.
Furthermore, few pointers referenced objects on the stack that were no more valid.

Motivation and Context

It solves a memory leak and performance problems.

How Has This Been Tested?

Tested on Windows 10.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

Copy link
Member

@derrod derrod left a comment

Choose a reason for hiding this comment

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

Please adjust the commit message according to our contributing guidelines.

We also have been a bit inconsistent with the prefix for the hook, but it should either be win-capture: (the plugin) or graphics-hook: (for the sub project).

@gxalpha
Copy link
Member

gxalpha commented Jul 25, 2023

You probably want to target the master branch instead of the release/29.1 branch, which is unlikely to get any more releases anyways.
I’ll change the base branch of the PR to master, please make sure to rebase your changes on master (or reset it to the state of master and cherry-pick your commit) and force-push that to the branch of this PR (otherwise the cherry-picked commits on 29.1 will also show up on the PR).

@gxalpha gxalpha changed the base branch from release/29.1 to master July 25, 2023 08:51
@sumydi
Copy link
Author

sumydi commented Jul 25, 2023

Yes, you're right, my bad, I was not aware that the 29.1 branch was "dead".
I rebased my changes to master and force-pushed the change to the PR's branch.
I hope that the commit message is now compliant with OBS guidelines.

@sumydi sumydi requested a review from derrod July 25, 2023 10:22
Copy link
Member

@derrod derrod left a comment

Choose a reason for hiding this comment

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

Commit message is fine now, I'll leave the functional review to somebody whose more familiar with Vulkan stuff.

Copy link
Collaborator

@jpark37 jpark37 left a comment

Choose a reason for hiding this comment

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

Fixes look good. @Lain-B might want to bump the hook version though.

@gxalpha gxalpha changed the title vulkan-capture: Fix leaking framebuffers data win-capture: Fix leaking framebuffers data Jul 25, 2023
@Lain-B
Copy link
Collaborator

Lain-B commented Jul 25, 2023

Fixes look good. @Lain-B might want to bump the hook version though.

Will do.

The wrong linked list was used when removing framebuffer data.
Furthermore, a pointer referenced an object on the stack that
was no more valid.
@Lain-B
Copy link
Collaborator

Lain-B commented Jul 25, 2023

Thank you very much for the fix.

@Lain-B Lain-B merged commit b12ed30 into obsproject:master Jul 25, 2023
14 checks passed
@sumydi
Copy link
Author

sumydi commented Jul 26, 2023

You're welcome, I'm happy to have helped.

I don't know if the following information can be useful for you, but:

  • the bug can degrades the performances of any application that is using Vulkan each time it calls vkCmdBeginRenderPass()
  • the severity of the issue depends on how the application manages the vkFrameBuffer
  • one potential workaround for the application is to use the VK_KHR_dynamic_rendering extension instead of vkRenderPass,

@RytoEX RytoEX added this to the OBS Studio (Next Release) milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants