-
-
Notifications
You must be signed in to change notification settings - Fork 950
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
feat(win/video): support native YUV 4:4:4 encoding #2533
Conversation
b64cf43
to
2598f91
Compare
Cool, didn't realize NVENC supported that (even though AV1 is still software only right?). |
Yes, no hardware encoding for AV1 4:4:4 on current generation of gpus
I'm not against the idea of supporting lossless encoding options (NVENC can do it for H.264 and HEVC, not AV1), but current netcode imposes hard limit on maximum video packet size, and this limit is very easy to hit on lossless. So need to improve the netcode first. |
Supposedly.. it may even be possible to dynamically switch between lossy and lossless? |
You're describing near lossless encoding, or including both DCT transform and quantization bypasses into rate control assessment. As far as I know, NVENC is not capable of this (quantization can be dynamically lossless when rate control selects QP=4, but DCT transform bypass is static on/off switch). |
dd157c0
to
f2b674a
Compare
c652d57
to
e961b3c
Compare
Should be more or less done. |
Ah, and I still have no idea if Intel encoder works correctly since I don't have supported hardware at hand right now. |
Resolved the CUDA bug, only minor stuff is left. |
We have some new patterns for docs which help produce cleaner doxygen docs. https://docs.lizardbyte.dev/projects/sunshine/en/master/source_code/source_code.html |
Alright, I will update the comments to what the codebase will be using at the time of merge. Currently this PR is held back at moonlight side, and merging one without the other is pointless. |
I will mark this as draft. Please mark it ready, once moonlight side is taken care of. |
@mohemohe Much appreciated, there was a redundant check that I missed. Pushed the fix, new CI builds should be ready shortly. |
@ns6089 There is no longer the hassle of switching options for each host. Moonlight: https://ci.appveyor.com/project/cgutman/moonlight-qt/builds/50287816/job/p870yhl7y1gd587n/artifacts sunshine_verbose_log_cd06345e30d2569961370781cbf64aa374d1c900.txt |
Hmm, it appears we send the right surfaces but then ffmpeg misconfigures the encoder and internal conversion happens. |
@mohemohe I tried to nudge quicksync in the right direction, CI builds are up. |
src/video.cpp
Outdated
{}, // SDR-specific options | ||
{}, // HDR-specific options | ||
{}, // YUV444 SDR-specific options | ||
{}, // YUV444 HDR-specific options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're relying on QSV to automatically determine the best profile by the input pixel format. Is it going to pick a regular HEVC Main profile to encode 4:4:4 on older CPUs that lack 4:4:4 encoding support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I brought back the explicit profile selection through the dictionary option so I think it should be fixed now.
src/video.cpp
Outdated
// Fallback options | ||
{}, // SDR-specific options | ||
{}, // HDR-specific options | ||
{}, // YUV444 SDR-specific options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QSV doesn't support H.264 High 4:4:4, so it ends up picking H.264 Main profile when the client asks for 4:4:4. We should probably block H.264 4:4:4 from being used with QSV to prevent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the code to explicitly request the profile QSV doesn't support, so I think it should now fail the encoder validation.
In the future (or if QSV still falls back to some valid profile) we might want to add more flexible codec blacklisting logic, but this PR is large enough already so I'd rather not do it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are definitely still some QSV issues, but I think we can merge this to unblock your other work and get some broader testing. I don't think there are regressions to existing functionality.
Locally I noticed:
- QSV seems to treat those profile options as soft suggestions and will simply ignore them if the hardware cannot support them (ex: AV1 Profile 1 or H.264 High 4:4:4 on any current Intel GPU)
- Some much older Intel GPUs seem to not even be able to handle the YUV 4:4:4 surfaces at all and fail at runtime to encode anything when asked to stream in 4:4:4
That's a bummer, but in this case I'm not sure what we can even do with the degree of QSV functionality that ffmpeg exposes.
|
I guess we can simply blacklist h.264 and av1 4:4:4 for qsv bypassing ffmpeg entirely, should be enough for a while. |
Either way, let's go ahead and merge this in its current state. I will add h.264/av1 blacklist shortly after. |
Quality Gate passedIssues Measures |
Description
Adds support for YUV 4:4:4 encoding, requires changes on moonlight side. Windows-only for now.
moonlight-common-c pull request: moonlight-stream/moonlight-common-c#91 merged
moonlight-qt pull request: moonlight-stream/moonlight-qt#1282 merged
Current state
nvenc in cuda mode leaks cpu memory on decoder destruction (nvenc-mapped cuda surfaces can't be unmapped and unregistered)Screenshot
Issues Fixed or Closed
Type of Change
.github/...
)Checklist
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.