feat(linux/vaapi): VAAPI encoder improvements#5388
Conversation
psyke83
force-pushed
the
more_vaapi_options
branch
from
3afed13 to
fe4d3b2
Compare
|
I'm getting the following error during encoder probing (causing a fallback to vulkan) when enabling BLBRC while using fe4d3b2: |
psyke83
force-pushed
the
more_vaapi_options
branch
from
fe4d3b2 to
9f99d14
Compare
|
The latest rebase should include the necessary fix. I can't test BLBRC on my AMD card, so let me know if it causes any other issues when enabled. |
|
Been running this for a few hours yesterday with all options (VBR, BLBRC, STRICT) flipped on and quality set to high. Subjectively I'm impressed with the quality I'm getting especially on red-heavy scenes with movement it's a lot less washed out compared to Vulkan encoding. Unsure where that comes from. Also host processing latency is just about .2 ms higher than current Vulkan default options which is negliable for me (1.9ms->2.1ms average). |
|
I added a full drop-down menu for rate control and included all possible modes. The default RC behaviour should now be exactly the same as before:
Unfortunately this added a lot of code, most of which is borrowed from the Windows AMF encoder logic and UI section, but I suppose I do prefer the UI category layout and named options now. |
psyke83
force-pushed
the
more_vaapi_options
branch
from
06cd818 to
4111844
Compare
|
Can you fix the doxygen errors? |
psyke83
force-pushed
the
more_vaapi_options
branch
2 times, most recently
from
f99f0fa to
c29a347
Compare
psyke83
force-pushed
the
more_vaapi_options
branch
2 times, most recently
from
6904ac2 to
19e9651
Compare
|
With those improvements to VAAPI and the current problems/caveats when using Vulkan (Hardware/driver specific issues, requiring a RADV_EXPERIMENTAL flag, at least one known memory leak in ffmpeg) I'm wondering if we should prefer Just something to consider (for another PR). I've been using both but VAAPI seems to give me subjectively better image quality with the new options also seems to be the more mature option of the two. |
|
@Kishi85 Vulkan is probably a better choice for people using Portal capture and/or sandboxed versions of Sunshine, especially with midrange or lower GPUs. When the EGL context priority can't be set properly, VAAPI will drop frames during high GPU load. |
|
Thats a good point I didn't know/hadn't considered yet. |
ReenigneArcher
force-pushed
the
more_vaapi_options
branch
from
19e9651 to
6b5a394
Compare
Bundle ReportChanges will increase total bundle size by 3.79kB (0.16%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sunshine-esmAssets Changed:
Files in
Files in
|
|
1 Test failing, just an ordering issue. |
psyke83
force-pushed
the
more_vaapi_options
branch
from
6b5a394 to
981a4a0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5388 +/- ##
=========================================
Coverage ? 27.15%
=========================================
Files ? 111
Lines ? 25480
Branches ? 11248
=========================================
Hits ? 6919
Misses ? 16606
Partials ? 1955
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
|
|
I'm a bit confused as to why doxygen is entering into the linux/freebsd macro block for the Windows builds, but I think the latest commit should fix it? Let me know if I need to modify this further. |
Have to predefine macros for doxygen or docs fail to build. Also since the hosted docs are built on Linux, it wouldn't be able to show anything behind other macros if we didn't predefine them. I think you have the right fix for this though. |
ReenigneArcher
force-pushed
the
more_vaapi_options
branch
from
0f46031 to
d96afd4
Compare
|
@Kishi85 I'm not 100% certain if the Sonar warning is highlighting a real issue. Can you verify that BLBRC is activating correctly? With Mine always reports BLBRC as OFF (even if I hardcode to true in video.cpp) due to lack of driver support, so I'm not 100% certain if there really is an issue with initialization order not parsing the user config correctly. I've verified that RC, strict vbv and quality is being passed correctly, but those are initialized separately through vaapi.cpp. |
Attaching a debugger, setting a breakpoint where Sonar complains and inspecting EDIT: My card or driver also does not seem to support BLBRC but that is masked by this issue. It seems that only getting a warning (and fallback to Vulkan but that might be due to just running in my dev env) when hardcoding the value to |
* Split rate control selection logic out from VBV bitrate enforcement. When VBV bitrate limits are enabled, the VBV constraint is now also possible to apply to CBR rate control. * Add opt-in option to allow VBR rate control. If selected, the existing RC logic detection will try to select VBR, then CBR, then fall back to CQP. If unselected, only CBR followed by CQP will be attempted. * Add encoder quality profile selection. As VAAPI doesn't mandate specific quality levels, we must probe the maximum level during initialization. If VAConfigAttribEncQualityRange returns a value, the maximum value will represent the fastest/lowest quality and value 1 sets highest quality. This value is then mapped onto the compression_level AVOption which sets the VAEncMiscParameterTypeQualityLevel via ffmpeg's vaapi_encode.c. * Add Block Level Bitrate Control (BLBRC) option that can be combined with VBR or CBR rate control on supported encoders.
psyke83
force-pushed
the
more_vaapi_options
branch
from
d96afd4 to
6cfafda
Compare
|
I've changed the config to pass as a pointer to align with other examples of bools being parsed in the code (e.g. As a local test, since my VAAPI driver doesn't expose any low power modes, I modified the hevc_vaapi entry: Enabling blbrc gives this log: Leaving disabled, hevc works fine: I also did a sanity check by comparing the output of standalone ffmpeg when blbrc is enabled, and it always reports OFF for me. As long as Sonar stops complaining after this change, I think the PR should be solid now. |
|
|
Although I believe the blbrc config is being parsed correctly (see my previous comment), I'm not sure how to address the Sonar warning. I don't think that my changes in this PR are fundamentally different to the other AVOptions being parsed via video.cpp for other encoders, so the potential initialization ordering issue could apply to all other options across all encoders. I'll mark as ready for review, but someone else should check if I'm overlooking an obvious blunder. |
|
The main problem here is that Alternatively would just adding the necessary common_option during config parsing work? As that would always guarantee the correct value being set after parsing. Could doing it like this be a solution? diff --git a/src/config.cpp b/src/config.cpp
index 7d8f0145..66d8ba22 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -28,6 +28,7 @@
#include "platform/common.h"
#include "rtsp.h"
#include "utility.h"
+#include "video.h"
#ifdef _WIN32
#include <shellapi.h>
@@ -1653,6 +1654,9 @@ namespace config {
video.vaapi.vaapi_rc = vaapi::rc_from_view<vaapi::rc_e>(video.vaapi.vaapi_rc_str, video.vaapi.vaapi_rc);
}
bool_f(vars, "vaapi_blbrc", (bool &) video.vaapi.blbrc);
+ video::vaapi.h264.common_options.emplace_back("blbrc"s, &video.vaapi.blbrc);
+ video::vaapi.hevc.common_options.emplace_back("blbrc"s, &video.vaapi.blbrc);
+ video::vaapi.av1.common_options.emplace_back("blbrc"s, &video.vaapi.blbrc);
bool_f(vars, "vaapi_strict_rc_buffer", video.vaapi.strict_rc_buffer);
int_f(vars, "vk_tune", video.vk.tune);
diff --git a/src/video.cpp b/src/video.cpp
index 2abc1eb4..c34751aa 100644
--- a/src/video.cpp
+++ b/src/video.cpp
@@ -1191,7 +1191,6 @@ namespace video {
// Common options
{
{"async_depth"s, 1},
- {"blbrc"s, &config::video.vaapi.blbrc},
{"idr_interval"s, std::numeric_limits<int>::max()},
},
{}, // SDR-specific options
@@ -1205,7 +1204,6 @@ namespace video {
// Common options
{
{"async_depth"s, 1},
- {"blbrc"s, &config::video.vaapi.blbrc},
{"sei"s, 0},
{"idr_interval"s, std::numeric_limits<int>::max()},
},
@@ -1220,7 +1218,6 @@ namespace video {
// Common options
{
{"async_depth"s, 1},
- {"blbrc"s, &config::video.vaapi.blbrc},
{"sei"s, 0},
{"idr_interval"s, std::numeric_limits<int>::max()},
},UPDATE: After testing I can confirm your current solution is working as intended as I'm getting the BLBRC warning when enabling the option after properly installing Sunshine including this PR. |
|
This seems like an existing pattern issue, and 4 other cases of it in master all in |
4 participants
Description
Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage
See our AI usage policy.