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

Remove the need for a patched nvidia library for NvFBC. #2471

Open
wants to merge 2 commits into
base: nightly
Choose a base branch
from

Conversation

hgaiser
Copy link

@hgaiser hgaiser commented Apr 26, 2024

Description

This PR is based on hgaiser/nvfbc-rs#5 , which in turn is based on https://github.com/keylase/nvidia-patch/blob/master/win/nvfbcwrp/nvfbcwrp_main.cpp#L23-L25 .

With this change you don't need a patched NVIDIA library to use NvFBC. My best guess is that this is what GeForce Experience uses to allow it to work on GTX / RTX cards.

Screenshot

N/A

Issues Fixed or Closed

N/A

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@CLAassistant
Copy link

CLAassistant commented Apr 26, 2024

CLA assistant check
All committers have signed the CLA.

@ReenigneArcher
Copy link
Member

Thank you for the PR! I also didn't know we could handle it like this.

If this affects NVENC as well, I think there might need to be an additional change somewhere in https://github.com/LizardByte/Sunshine/blob/nightly/src/nvenc/nvenc_base.cpp

@hgaiser hgaiser changed the title Remove the need for a patched nvidia library for NvFBC and NVENC. Remove the need for a patched nvidia library for NvFBC. Apr 26, 2024
@hgaiser
Copy link
Author

hgaiser commented Apr 26, 2024

Yeah so I'm a bit confused by NVENC. It almost seems like enabling this patch also allows NVENC.. I never thought about it but using Moonshine I never use a patched NVIDIA library and NVENC encoding works fine there.

I feel like this might trigger something in the NVIDIA library that also allows encoding, but I'm not sure. I tried rebooting, but I still see that I can encode, even if I use nightly and not this PR.

[2024:04:27:00:38:36]: Error: Failed to create session: This hardware does not support NvFBC
[2024:04:27:00:38:37]: Info: Found H.264 encoder: h264_nvenc [nvenc]
[2024:04:27:00:38:37]: Info: Found HEVC encoder: hevc_nvenc [nvenc]

It might also mean that if you use NVENC, but not NvFBC, that you still need a patched driver 🤷 . I haven't been able to trigger this case at least.

Can you test this to see how this works for you? Whether you can capture & encode using this PR but can't capture nor encode without this PR?

@ReenigneArcher
Copy link
Member

There might be some confusion. The ./src/nvenc code is only used on Windows (as far as I know), since nvfbc is deprecated on Windows. For reference, it was introduced here: #1427

Currently I don't have a quick way to test your PR, but I could ask for testers on our Discord server?

@hgaiser
Copy link
Author

hgaiser commented Apr 26, 2024

There might be some confusion. The ./src/nvenc code is only used on Windows (as far as I know), since nvfbc is deprecated on Windows. For reference, it was introduced here: #1427

Ah I see. I'm curious, why isn't ffmpeg used on Windows? I never looked into it much but I thought it was cross platform?

Currently I don't have a quick way to test your PR, but I could ask for testers on our Discord server?

Yeah I think that would be nice 👍

@ReenigneArcher
Copy link
Member

ReenigneArcher commented Apr 27, 2024

We do use ffmpeg on Windows as well. I'm not up to speed on the standalone nvenc encoder, but I know the other capture methods do use ffmpeg.

cgutman
cgutman previously approved these changes Apr 30, 2024
Copy link
Contributor

@cgutman cgutman left a comment

Choose a reason for hiding this comment

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

NVENC is not (meaningfully) limited for our purposes on consumer GPUs. Only NvFBC has this restriction of being completely unavailable on consumer GPUs, so only making this change for NvFBC seems correct to me.

If someone can validate that this still works on cards with official NvFBC support, it seems safe to merge.

Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 6.17%. Comparing base (7fb8c76) to head (ee2b5d9).

Additional details and impacted files
@@            Coverage Diff             @@
##           nightly   #2471      +/-   ##
==========================================
- Coverage     6.17%   6.17%   -0.01%     
==========================================
  Files           86      86              
  Lines        17546   17549       +3     
  Branches      8190    8190              
==========================================
  Hits          1083    1083              
- Misses       15410   15414       +4     
+ Partials      1053    1052       -1     
Flag Coverage Δ
Linux 4.26% <0.00%> (-0.01%) ⬇️
Windows 2.04% <ø> (ø)
macOS-12 8.71% <ø> (+0.12%) ⬆️
macOS-13 ?
macOS-14 8.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/platform/linux/cuda.cpp 1.64% <0.00%> (-0.02%) ⬇️

... and 1 file with indirect coverage changes

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.

None yet

4 participants