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(vaapi): use VBR rate control mode #3337

Closed
wants to merge 1 commit into from

Conversation

cgutman
Copy link
Collaborator

@cgutman cgutman commented Oct 29, 2024

Description

This is a possible alternative fix for #2864 that could be taken instead of #3332.

In the case of single frame VBV size, using CBR with VAAPI seems to have major quality problems with Intel and AMD drivers. VBR behaves much better in my testing (Intel H.264 case) and allows us to still enforce a reasonable frame size limit without causing the video quality to suffer. Interestingly, we also had to do something similar with the QuickSync encoder using the CBR_WITH_VBR quirk flag to force VBR mode.

As a nice side effect, it also allows the video bitrate to drop when content is static, rather than adding useless filler data to the bitstream to preserve the exact specified bitrate. This also fixes the decoder compatibility issue reported in #3331.

Screenshot

Issues Fixed or Closed

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

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.15%. Comparing base (79ada18) to head (6aa2981).
Report is 97 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3337   +/-   ##
=======================================
  Coverage   11.15%   11.15%           
=======================================
  Files          99       99           
  Lines       17183    17180    -3     
  Branches     8009     8008    -1     
=======================================
  Hits         1916     1916           
+ Misses      12580    12577    -3     
  Partials     2687     2687           
Flag Coverage Δ
Linux 8.45% <ø> (+<0.01%) ⬆️
Windows 5.22% <ø> (ø)
macOS-13 13.65% <ø> (+0.02%) ⬆️
macOS-14 12.63% <ø> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
src/platform/linux/vaapi.cpp 3.35% <ø> (+0.07%) ⬆️
src/video.cpp 30.32% <ø> (-0.03%) ⬇️

Copy link

@ns6089
Copy link
Contributor

ns6089 commented Oct 29, 2024

VBR is required because CBR with reasonably relaxed VBV still results in poor quality on Intel?

@cgutman
Copy link
Collaborator Author

cgutman commented Oct 29, 2024

What I'm finding in my VAAPI testing with several AMD and Intel GPUs with a single frame VBV is that VBR appears to behave like CBR except that it allows bitrate undershoots (which is what we want). The quality issues seen on Intel with CBR and a single frame VBV are similar to what we already saw with QSV and solved by forcing VBR there.

In my testing with an RX 7600, I noticed bitrate overshoots happening periodically with both VBR and CBR modes, despite setting a single frame VBV. VBR allowed the bitrate to drop when content was relatively static while CBR was pumping 150 Mbps constantly, so VBR seems like a clear win there.

I did verify that this change alone does not fix the poor encoding quality on Polaris cards (though my RDNA3 card was totally fine, regardless of the VBV).

So I'm thinking for the full solution:

@ns6089
Copy link
Contributor

ns6089 commented Oct 30, 2024

Sporadic 2x-3x overshoots for a frame are expected without pre-analysis pass due to the parallel nature of hardware encoding, but not sure this can be controlled in VAAPI. What I meant by "relaxed" VBV is that we don't have to choose between two extremities - single frame or "off", but may settle on something like 4 or 5 frames. Because otherwise it may default to something like 1 second since it performs better in the usual periodic GOP scenarios. The fact that CBR also enables zero padding is certainly a bummer though, again not sure if it can be controlled in VAAPI.

@cgutman
Copy link
Collaborator Author

cgutman commented Oct 31, 2024

What I meant by "relaxed" VBV is that we don't have to choose between two extremities - single frame or "off", but may settle on something like 4 or 5 frames.

Yeah, unfortunately even pretty relaxed VBV buffers like 250 ms still cause Polaris's encoder to produce garbage quality.

What I ended up doing for #3332 is basically choosing between 2 operating modes:

  • VBR + single frame VBV
  • CBR + default VBV

The former works great for Intel GPUs and AV1 on AMD GPUs (likely any codec for RDNA3 and later, but I wanted to be conservative). It is also what we use for QuickSync (which likely shares RC logic with Intel's VAAPI driver).

The latter is what we will default to for H.264 and HEVC on AMD GPUs, but we'll allow users to override if they want.

The coupling of RC mode and RC buffer size helps to balance the negative effects of each other. Single frame VBV avoids the VBR pitfall of producing massive frames using RC headroom provided by earlier smaller frames. CBR avoids some of the downsides of a larger VBV by ensuring a near constant frame size.

Closing this PR in favor of the full fix in #3332

@cgutman cgutman closed this Oct 31, 2024
@hebo6
Copy link

hebo6 commented Nov 1, 2024

The bitrate drop feature looks impressive. I hope this PR will be merged.

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.

3 participants