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

qsv: lowpowerfix #6024

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

qsv: lowpowerfix #6024

wants to merge 3 commits into from

Conversation

DOram424
Copy link

@DOram424 DOram424 commented May 3, 2024

Fixes problems in handbrake qsv encoding when running with a CPU that supports low power encoding for the in use codec. Low power support is much more limited than non low power support and passing low power to encoder init functions of the intel library will trigger an error if the hardware doesn't support it for that rate control method / codec pairing. This is a particular problem under windows where low power is always defaulted to on and encodes crash so the default is changed to off. Patch also fixes problems under linux where capability checking failure caused encodes to fallback to CQP encoding even though ICQ works fine. I'm pretty certain this will close at least #5932.

The following example shows what happens pre/post patch when encoding using a 13th Gen intel CPU with kernel master firmware and driver at time of posting.

Pre-patch under linux:

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=0"
Runs a high power CQP encode with video size = 158833956 at roughly 60fps

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=1"
Runs a high power CQP encode with video size = 158833956 at roughly 60fps

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=1:force-cqp=1"
Runs a high power CQP encode with video size = 158833956 at roughly 60fps

Post-patch:

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=0"
Runs a high power ICQ encode with video size = 198500428 at roughly 35fps.

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=1"
Fails to run a low power ICQ encode as not hardware supported.

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=1:force-cqp=1"
Runs a low power CQP encode with video size = 159432944 at roughly 300fps

HandBrakeCLI -i in.mkv -o out.mkv -e qsv_h265 -q 20 -x "lowpower=0:force-cqp=1"
Runs a high power CQP encode with video size = 158833956 at roughly 60fps

Any desktop CPU newer than 8th gen will show these problems provided the correct Huc and Guc firmwares are loaded and working, but exactly what will happen is hard to predict. For example with the 9th gen CPU that doesn't support any low power hevc methods the above example won't fail but but problems will be visible when the codec is changed to qsv_h264. This is why it's hard to be certain if it will fix existing bugs without testing on the users hardware.

Tested on:

  • Windows 10+ (via MinGW)
  • macOS 10.13+
  • Ubuntu Linux

Low power encoding is not supported on any current hardware for the
default codec / bitrate method used by hevc or most hardware for the
defaults used in x264. Remove code that defaults low power to on in
windows so it doesn't crash or fallback for users with new hardware.
Since all hardware (>G8) which supports low power only supports low power on a subset of encoding methods having lowpower=on for all the query_capabilities tests results in them universally failing if running with on a recent CPU. Fix the code so that it uses lowpower=0 when testing capabilities so these don't cause encoder fails and/or fallback to different parameters.
@galinart
Copy link
Contributor

galinart commented May 3, 2024

One of the solution for this problem might be create a separate capabilities structure for lowpower hw and for non-lowpower and dispatch accordantly by user option. Some of the CPU supports both and lowpower off by default does not look proper solution.

@DOram424
Copy link
Author

DOram424 commented May 4, 2024

I'd definitely favour a sort of explicit user process .. whereby they change a parameter and the encode breaks - as they hopefully won't attribute that to anything but their own fiddling with internal parameters. Some CPUs do support low power but it's a lot less reliable and it does alter the encode parameters so it really has to be driven by an aware user or be reliably testable. Whether that is via the GUI (with low power on by default if it works) .. or via the command line .. is just a matter of more code doing the same thing and some testing.

For now at least it is not easy to reliably test the low power parameter prior to encode time as almost all the capability checking code that I'm patching here is entirely spurious (i.e. HB_QSV_CAP_LOWPOWER_ENCODE .. or other capabilities have no effect on whether or not the relevant parameter is set at encode time) . This is partly because the older intel libraries encouraged the codebase to incluide a lot of code like this:

if (HB_CHECK_MFX_VERSION(version, 1, 6)

which is adding runtime support for linking with older mfx libraries and is largely .. harmless but can drive users with specific setups onto untested code paths. Worse is the abstract parameter logic for older libraries being generally applied that is not general, cannot be abstracted from at least version / rate control / codec and which is basically just arbitrary. This applies to low power and more so to more important parameters such as the GOP configuration code in enc_qsv.c which was probably required by an old library but is basically just random noise now. By convention in libvpl setting a parameter to 0 gets default behaviour so default 0 is the way to go for low power and every other poorly defined parameter without a functional test.

I'm happy to open a feature request, but at least now .. there's a bug - let's fix it and default low power to off. Without a functional test and without using the same code for configuring encodes as we do for testing at start time we just can't and never will be able to predict if a parameter change is going to work when we encode. Keep it off and force users to be aware when they switch it on as that gets most of the functional benefit of a tidy code base for a fraction of the effort.

@DOram424
Copy link
Author

DOram424 commented May 5, 2024

Yeuch I left a terse reply.

I should have said that my 13th gen CPU supports low power for hevc, but it doesn't support the handbrake default rate control method of ICQ. Without the patch if low power is on by default it will cause handbrake to fallback to CQP encoding. That produces a noticeably worse encode / quality trade off compared to ICQ even if you notice it's changed and compensate for the change in quality factor.

I actually do encode with low power, and when I get a chance will put in a feature request. It is possible to do just as good encodes with CQP as it is with ICQ and it doesn't need a lot of changes to get the parameters through to the encoder.

@galinart
Copy link
Contributor

Yeuch I left a terse reply.

I should have said that my 13th gen CPU supports low power for hevc, but it doesn't support the handbrake default rate control method of ICQ. Without the patch if low power is on by default it will cause handbrake to fallback to CQP encoding. That produces a noticeably worse encode / quality trade off compared to ICQ even if you notice it's changed and compensate for the change in quality factor.

I actually do encode with low power, and when I get a chance will put in a feature request. It is possible to do just as good encodes with CQP as it is with ICQ and it doesn't need a lot of changes to get the parameters through to the encoder.

Thanks for the patch!

What OS is this patch target for? Linux or Windows? I still don't understand based on your multiple comments. Please be clear in details.

There is no any ICQ issues on Windows that I am aware.
Feel free to provide the full HandBrake log for your particular issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants