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

obs-ffmpeg: New default settings for AMD encoders #9352

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

Conversation

rhutsAMD
Copy link
Contributor

@rhutsAMD rhutsAMD commented Aug 2, 2023

Description

Introducing new default AMD encoder settings for improved perceptual quality for AVC/HEVC/AV1. These new default settings have been tuned to target stable recording on RX 5700 XT and up on the recent driver.

AVC AVC HEVC HEVC AV1 AV1
Up to 1080p60fps > 1080p60fps Up to 1080p60fps > 1080p60fps Up to 1080p60fps > 1080p60fps
RATE_CONTROL_METHOD CBR CBR CBR CBR CBR CBR
PEAK_BITRATE Same as TARGET_BITRATE Same as TARGET_BITRATE 1.5 X TARGET_BITRATE 1.5 X TARGET_BITRATE 1.5 X TARGET_BITRATE 1.5 X TARGET_BITRATE
VBV_BUFFER_SIZE Same as TARGET_BITRATE Same as TARGET_BITRATE Same as PEAK_BITRATE Same as PEAK_BITRATE Same as PEAK_BITRATE Same as PEAK_BITRATE
FILLER_DATA_ENABLE TRUE TRUE FALSE FALSE FALSE FALSE
ENFORCE_HRD TRUE TRUE Not set Not set Not set Not set
MAX_B_FRAMES 2 0 - - - -
B_PIC_PATTERN 2 0 - - - -
SCREEN_CONTENT_TOOLS - - - - TRUE TRUE
PALETTE_MODE - - - - TRUE TRUE
PRESET Quality Quality Quality Quality high quality Balanced
PROFILE high high main/main10 main/main10 main main
ENABLE_VBAQ/AQ_MODE true for RCMethod != (CQP | HQVBR | HQCBR) true for RCMethod != (CQP | HQVBR | HQCBR) true for RCMethod != (CQP | HQVBR | HQCBR) true for RCMethod != (CQP | HQVBR | HQCBR) Not set Not set

Motivation and Context

This PR provides new recommended defaults for the AMD encoders that have been optimized for perceptual quality. The settings are automatically applied depending on the resolution and framerate. The default settings can be overridden as expected and only provide an improved base foundation for encoder settings targeted at improving perceptual quality.

How Has This Been Tested?

Tested on RX 5000, 6000, and 7000 series cards using public driver version 24.2.1.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@flaeri
Copy link
Contributor

flaeri commented Aug 3, 2023

Testing this PR, and unfortunately I ran into a crash that seems to occur in the AMD drivers.
Crash:
https://obsproject.com/logs/A1hmycbXo3FBhlJJ

Log:
https://obsproject.com/logs/JZHZe6rt8vkBVQyV

Occured when stopping the last recording (w hevc).

I've tried reproducing it, but it seems to be somewhat intermittent, so I have not managed to find specific reproduction steps so far. Will update if I find a reliable repro.

@flaeri
Copy link
Contributor

flaeri commented Aug 3, 2023

I am a little bit worried about these defaults. This is primarily because it enables Pre-Analysis (PA).
PA eats a decent chunk of traditional GPU compute, and this can in turn lead to encoder lag if there is contention for GPU resources.

In my opinion, people with older/weaker hardware, like rx4/500 series, or APUs, this is likely to cause some problems, which are not currently easily remedied.
As it stands, the only way to disable PA is to type params into the text box, and I worry about the amount of people who would manage to actually disable PA on their own without the assistance from OBS support or someone knowledgeable, or consulting a wiki/guide.

I would personally prefer one or a combination of the following:

  • Keep PA off by default, change the default rate control for streaming to HQCBR
    • Rate controls requiring PA would automatically enable it (HQCBR, QVBR etc), which it currently does today
  • Settings checkbox to enable/disable PA
  • Keep PA off unless RC = CBR (other RCs requiring it functions as is, and enable PA (HQCBR, QVBR etc)

This would allow rate controls where PA is less impactful (VBR, CQP etc) to be easily (and/or automatically) be used without the additional GPU load.

I understand the reasoning and desire to have AMF look better by default for streaming, and at the very least be easier/simpler to achieve, and agree with that sentiment.

Just my two cents. Thank you for the PR, it seems to work well for me outside of that one crash, which seems to have been a one off (cannot reproduce it).

@nowrep
Copy link
Contributor

nowrep commented Aug 5, 2023

FILLER_DATA_ENABLE=true has no effect with ENFORCE_HRD=false (which is the default) - no filler data will be inserted.

Also ENFORCE_HRD=false will allow the bitrate to peak very high even with CBR, which doesn't seem to be a good thing for streaming.

@WizardCM WizardCM added the Enhancement Improvement to existing functionality label Aug 5, 2023
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from a4d1018 to d83d845 Compare August 8, 2023 19:08
@rhutsAMD
Copy link
Contributor Author

rhutsAMD commented Aug 9, 2023

I am a little bit worried about these defaults. This is primarily because it enables Pre-Analysis (PA). PA eats a decent chunk of traditional GPU compute, and this can in turn lead to encoder lag if there is contention for GPU resources.

In my opinion, people with older/weaker hardware, like rx4/500 series, or APUs, this is likely to cause some problems, which are not currently easily remedied. As it stands, the only way to disable PA is to type params into the text box, and I worry about the amount of people who would manage to actually disable PA on their own without the assistance from OBS support or someone knowledgeable, or consulting a wiki/guide.

I would personally prefer one or a combination of the following:

  • Keep PA off by default, change the default rate control for streaming to HQCBR

    • Rate controls requiring PA would automatically enable it (HQCBR, QVBR etc), which it currently does today
  • Settings checkbox to enable/disable PA

  • Keep PA off unless RC = CBR (other RCs requiring it functions as is, and enable PA (HQCBR, QVBR etc)

This would allow rate controls where PA is less impactful (VBR, CQP etc) to be easily (and/or automatically) be used without the additional GPU load.

I understand the reasoning and desire to have AMF look better by default for streaming, and at the very least be easier/simpler to achieve, and agree with that sentiment.

Just my two cents. Thank you for the PR, it seems to work well for me outside of that one crash, which seems to have been a one off (cannot reproduce it).

Thank you for providing feedback on the PR and testing it.

Older/weaker hardware such as RX 4/500 series do not support PA and will not be allowed to enable it. To clarify, PA is only supported on Radeon RX 5000 Series or newer GPUs as well as Ryzen 2000 U/H series or newer APUs. GPUOpen-LibrariesAndSDKs/AMF/blob/master/amf/doc/AMF_Video_PreAnalysis_API.md#13-supported-hardware

The recommended quality settings are to improve the quality at 1080p60 or lower resolutions/throughput.

PA is kept off/not set for higher than 1080p60. The same idea applies for the preset where balanced mode is applied for all codecs for higher than 1080p60.

Since you mention having the RC method set to HQCBR for streaming, is there a way to identify the recording or streaming use case at the encoder plugin level? My understanding is that the AMF encoder plugin in OBS just receives textures and encodes them like the other encoders in OBS. It implements the encoder interface (obs_encoder_info) and is not aware of what OBS does with the textures after it is done encoding. Is there some way to tell if the encoded textures will be used for recording/streaming? If I remember correctly, OBS is currently enforcing CBR only for streaming rtmp-common.c#L665

We are also interested in having a PA enable/disable checkbox but from past discussions it seems that UI/interface changes need to be done in a vendor agnostic way by OBS developers.

@flaeri
Copy link
Contributor

flaeri commented Aug 9, 2023

PA support: I'm particularly worried about the APUs. They don't have a lot of GPU to play with, and I believe basically any game would lead to GPU contention, and ultimately encoder lag.

HQCBR: I'm not certain, but I think you are correct. What I was envisioning was the defaults in the UI being different in streaming vs recording, but now that my memory has been jogged, that is not something that can be done in a simple manner.

PA bool/checkbox: Which leaves us back at the issue of the UI/UX, which is tricky. I know they don't want more checkboxes, but I am failing to see any other way to neatly handle it.


If it is just kept as is (PA enabled by default, no easy way to disable), then I truly believe that every APU user that tries streaming/recording gameplay will have a very poor experience, and be unlikely to figure it out on their own. There are a lot of APU users, and having the defaults be unusable(for gameplay), where the only solution is very arcane just really vexes me, personally. I think it would lead to substantially more support request as well.

At this point, I am just retreading the same ground, so I don't really have anything more to add. Thank you for your time and effort :)

@flaeri
Copy link
Contributor

flaeri commented Aug 10, 2023

OBS log: log.txt

I figured it would be pertinent to add some data on my claims instead of people just having to take my word for it. Here is an example of the APUs I'm talking about.

Its a simple older game (assault android cactus), running at 1366x768, targeting 60fps. All else equal between the two OBS sessions. Running stock, it hovers around 50fps, and is pretty pleasant to play. With the PR its hovering 40fps, but the lows are very noticable, quite jarring/stuttery (big fluctuations), a poor experience.

Regarding OBS, as you can see in the log, current master/stock managed to stream/record without any render or encoder lag. With the PR it is unfortunately unusable skipped frames due to encoding lag: 3895/4201 (92.7%)


Side thought regarding PA handling. If bool/checkbox is out of the question, then perhaps we could at least somehow exclude APUs somehow? MAX_THROUGHPUT is not supported on this chip, and returns 0. Maybe that would be a simple way to exclude APUs (assuming similar chips behave the same). As far as I know there is currently no way for AMF to identify low power/APU hardware.

@hxzael
Copy link

hxzael commented Aug 14, 2023

I don't think having AMF_PA_TAQ_MODE set to 2 by default is a good idea considering how using AMF_PA_LOOKAHEAD_BUFFER_DEPTH with it will overload the encoder in OBS at resolutions > 864p. The only way to use it without overloading is to set the preset to Balanced or Speed but even then there will be constant frame skips present. Not to mention that TAQ Mode 2 will constantly crash OBS when you stop Recording/Streaming regardless if it overloads or not.

In ffmpeg, if AMF_PA_TAQ_MODE set to 2 is used in combination with AMF_PA_LOOKAHEAD_BUFFER_DEPTH at 1080p60, you'll frequently find the encoding speed going below 1x, which would explain the overloads in OBS. Here too, it's necessary to dial down the preset back to Balanced or Speed to achieve real-time encoding speeds again.

OBS Log file with the suggested defaults above:
https://obsproject.com/logs/ID4dOFS2O1OrJxSL

@hxzael
Copy link

hxzael commented Aug 16, 2023

I don't think having AMF_PA_TAQ_MODE set to 2 by default is a good idea considering how using AMF_PA_LOOKAHEAD_BUFFER_DEPTH with it will overload the encoder in OBS at resolutions > 864p. The only way to use it without overloading is to set the preset to Balanced or Speed but even then there will be constant frame skips present. Not to mention that TAQ Mode 2 will constantly crash OBS when you stop Recording/Streaming regardless if it overloads or not.

In ffmpeg, if AMF_PA_TAQ_MODE set to 2 is used in combination with AMF_PA_LOOKAHEAD_BUFFER_DEPTH at 1080p60, you'll frequently find the encoding speed going below 1x, which would explain the overloads in OBS. Here too, it's necessary to dial down the preset back to Balanced or Speed to achieve real-time encoding speeds again.

OBS Log file with the suggested defaults above: https://obsproject.com/logs/ID4dOFS2O1OrJxSL

I'm aware the log file I provided has a lot of clutter from other existing plugins, here is another log file from a clean Scene Collection where I reproduced the results I explained earlier.

https://obsproject.com/logs/bX4QINZDD6lxtcen

I have also attached a crash log that happened after stopping the recording.
crash log AMF.txt

@akiirui
Copy link

akiirui commented Nov 30, 2023

HQCBR and HQVBR are almost unusable, even on the 7900XTX. For 1080p60 recording, they are all overloaded.

@lextra2
Copy link

lextra2 commented Jan 13, 2024

HQCBR and HQVBR are almost unusable, even on the 7900XTX. For 1080p60 recording, they are all overloaded.

That is because rhuts forgot to tell you to use "Preset: Balanced" & "PASceneChangeDetectionEnable=false"
Otherwise the encoder overloads.

@lextra2
Copy link

lextra2 commented Jan 19, 2024

@rhutsAMD
My suggestions; Set scenecut of PreAnalysis to false, e.g.
PASceneChangeDetectionEnable=false
AMF_PA_SCENE_CHANGE_DETECTION_ENABLE

Reason: x264 & NVENC both have their scenecut for cbr/vbr disabled because a fixed keyint is preferred for streaming and having it disabled gives the encoder more headroom.

Also, use Preset: Balanced for all "HQ" presets.

Reason: The image quality difference of Balanced & Quality is minimal, but Quality is really pushing the encoder and so to maximize compatibility, Balanced would be the better option.

@Johl7
Copy link

Johl7 commented Jan 20, 2024

Yep, scenecut(scenehange) for real time livestreaming only hurt quality, also agree on bal preset, for higher end models qual is prob a no big deal but on my card for example(6650 xt), if i use PA stuff and bframes >2 qual preset can become quite a burden for the encoder, depending on the PA params in use.

@lextra2
Copy link

lextra2 commented Feb 7, 2024

Hello,
any updates? Whats the hold up?
As previously discussed, using AMF_VIDEO_ENCODER_QUALITY_PRESET_BALANCED & AMF_PA_SCENE_CHANGE_DETECTION_ENABLE=false should make the new settings work for most people.

QVBR, HQVBR & HQCBR are already using PreAnalysis anyways.

@Fenrirthviti
Copy link
Member

Please refrain from comments asking why something hasn't been merged yet.

We know this is here, we'll get to it as soon as we have time, in appropriate priority.

@Johl7
Copy link

Johl7 commented Feb 7, 2024

To add some things ive noticed regarding some of the settings in this post. For the PA engine i would suggest either to use Vulkan or DX12, from some testings ive done, OpenCL doesnt seem to work with PA propper. DX12 and Vulkan had better performance compared to the current default(DX11), at least in the few testings ive done.
PAEngineType=11(DX12) and PAEngineType=10(Vulkan). Correction, OpenCL does seem to work.

@lextra2
Copy link

lextra2 commented Feb 7, 2024

PAEngineType=11(DX12) and PAEngineType=10(Vulkan).

Where are you getting those values from?
The docs don't say anything about DX12 support

Nevermind found it
@rhutsAMD could you please update the docs of PreAnalysis.h#L95 with the relevant enumerations? Its very useful information which otherwise gets lost.

@rhutsAMD
Copy link
Contributor Author

rhutsAMD commented Feb 7, 2024

PAEngineType=11(DX12) and PAEngineType=10(Vulkan).

Where are you getting those values from? The docs don't say anything about DX12 support

Nevermind found it @rhutsAMD could you please update the docs of PreAnalysis.h#L95 with the relevant enumerations? Its very useful information which otherwise gets lost.

Thank you for noticing this. The header and the PreAnalysis API doc on GPUOpen have now been updated to reflect the additional PAEngineType options.

Regarding the new recommended defaults for AMF in OBS, there is still some internal testing going on and we expect to have an updated set of recommended defaults. I will update this PR with the new set once those are ready.

@rhutsAMD
Copy link
Contributor Author

The PR has been updated and the settings have been retested on the recent public driver version 24.2.1.

@hxzael
Copy link

hxzael commented Feb 29, 2024

I have a question regarding the recommended lookahead buffer-depth setting of 20 in these recommendations.

The Preanalysis API (https://github.com/GPUOpen-LibrariesAndSDKs/AMF/blob/master/amf/doc/AMF_Video_PreAnalysis_API.pdf) specifies that AMF_PA_TAQ_MODE only works when AMF_PA_LOOKAHEAD_BUFFER_DEPTH is set to 11, 21 or 41. Do values other than these 3 work considering you're selecting 20 instead here? Would love some clarification on this, thank you.

@rhutsAMD
Copy link
Contributor Author

rhutsAMD commented Mar 1, 2024

I have a question regarding the recommended lookahead buffer-depth setting of 20 in these recommendations.

The Preanalysis API (https://github.com/GPUOpen-LibrariesAndSDKs/AMF/blob/master/amf/doc/AMF_Video_PreAnalysis_API.pdf) specifies that AMF_PA_TAQ_MODE only works when AMF_PA_LOOKAHEAD_BUFFER_DEPTH is set to 11, 21 or 41. Do values other than these 3 work considering you're selecting 20 instead here? Would love some clarification on this, thank you.

The three LAB depths ( short lookahead(11), medium lookahead(21) and long lookahead(41) ) are tested recommendations, rather than requirements as the original wording suggests.

I have updated the doc to clarify the wording of the example values as being suggestions.

@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from 6355128 to 2be502e Compare March 6, 2024 14:47
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

The fourth commit ("@rhutsAMD
CI: Pull in latest submodules from master branch) is superfluous. Just rebasing the branch should be sufficient.

@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch 3 times, most recently from f176eff to 9b8f0b5 Compare March 6, 2024 19:56
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from 9b8f0b5 to cac06ed Compare March 11, 2024 14:15
@rhutsAMD rhutsAMD requested a review from RytoEX March 11, 2024 14:18
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from 495ed36 to 8392a7c Compare March 18, 2024 19:06
@rhutsAMD rhutsAMD requested a review from RytoEX March 18, 2024 19:06
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from 8392a7c to 0b53378 Compare April 1, 2024 20:23
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch 2 times, most recently from ef95d8f to 39c4751 Compare April 15, 2024 15:13
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Other than two nits, do not use merge commits to fast-forward history. Use git rebase.

plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
plugins/obs-ffmpeg/texture-amf.cpp Outdated Show resolved Hide resolved
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch 2 times, most recently from 1a234c6 to fec9534 Compare August 13, 2024 18:15
@rhutsAMD
Copy link
Contributor Author

Other than two nits, do not use merge commits to fast-forward history. Use git rebase.

Rebased to bring in latest changes.

@rhutsAMD rhutsAMD requested a review from RytoEX August 13, 2024 18:16
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from fec9534 to c67fb6f Compare August 13, 2024 18:22
@rhutsAMD
Copy link
Contributor Author

hi @RytoEX , I addressed your requested changes. Could you please revisit this PR?

@Ryu0833
Copy link

Ryu0833 commented Aug 21, 2024

hello my first time here just i am curious the "AMF_VIDEO_ENCODER_RATE_CONTROL_PREANALYSIS_ENABLE" default value is 0 in https://github.com/GPUOpen-LibrariesAndSDKs/AMF/blob/master/amf/doc/AMF_Video_Encode_API.md .but in the obs wiki:https://obsproject.com/wiki/AMF-Options , by default is'RateControlPreanalysisEnable=1' when the new default settings for amd encoders well be,will still enable by default 'RateControlPreanalysisEnable' ? and when it enbale it get alot stutter in obs and games thx and sorry for my bad english.

@lextra2
Copy link

lextra2 commented Aug 21, 2024

@Ryu0833

PreAnalysis is enabled for

_RATE_CONTROL_METHOD_QUALITY_VBR
_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR
_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR

It has been this way forever.
If your encoder overloads with any of these presets, switch to CBR, VBR or CQP.
You may also try running OBS as Admin, or the following command AMF/FFMPEG Options
PASceneChangeDetectionEnable=false

@Ryu0833
Copy link

Ryu0833 commented Aug 21, 2024

thx for response @lextra2

i am not talking about Preanalysis "AMF_VIDEO_ENCODER_PRE_ANALYSIS_ENABLE" ,ik it deafault disabled but i am talking about the "AMF_VIDEO_ENCODER_RATE_CONTROL_PREANALYSIS_ENABLE" the 'RateControlPreanalysisEnable' it enable without the Preanalysis and it hurts the performance. in the obs wiki it on by default just asking should leave it on by default or change it to off in the new default amd encoder

@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch 2 times, most recently from 98aad9a to 8034c36 Compare August 28, 2024 20:49
@rhutsAMD
Copy link
Contributor Author

I have pushed some changes to the new default settings PR which help improve performance. These changes include removing the new cases where PA would be enabled by default, as well as removing the addition of enabling the PA-dependent features by default. Additionally, the B-picture pattern for AVC was updated from 3 to 2.

@Ryu0833
Copy link

Ryu0833 commented Aug 30, 2024

Hello @rhutsAMD i did test the build and i have some feedback about it :

  1. "ENABLE_VBAQ" it always enable even when use rc "CBR" and it hurt the image quality.in the line 1498 it well always set to true.

  2. "PreAnalysis" why using "AMF_PA_TAQ_MODE" the need "AMF_PA_LOOKAHEAD_BUFFER_DEPTH" that use more compute power why not using the "AMF_PA_PAQ_MODE" with "AMF_PA_CAQ_STRENGTH_LOW" or "AMF_PA_CAQ_STRENGTH_MEDIUM".

  3. "PREENCODE_ENABLE" always true and it hurt performance with no change in quality maybe make true if the bitrate is high or using VBR.

  4. "HIGH_MOTION_QUALITY_BOOST_ENABLE" it good without Preanaalysis and good combo if B-frame enable.

  5. "B-frames" it only use in rx 6000 and 7000 why dont the rx 5000 at the minmum have only "1b-frame" it help maintain the video quality.

  6. What are the changes made in driver version 24.x.x compared to 23.12.1 are not documented in the AMF repository. The 23.12.1 driver provided good quality, smooth video with no lag or rendering issues in OBS .it can any chane that roll back the encoder only not the driver 23.12.1.

and my AMF option that i use to streaming twitch:
"RateControlPreanalysisEnable=0 EnableVBAQ=false"
if using with the PreAnalysis:"RateControlPreanalysisEnable=0 EnablePreAnalysis=true PASceneChangeDetectionEnable=false PAPerceptualAQMode=1 PACAQStrength=0"

i hope my feedback it well help improve the amd encoder quality and performance and sorry for my bad english.have good day.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

I've made another pass and caught some things that I'd previously missed.

Comment on lines 1531 to 1540
"\trate_control: %s\n"
"\tbitrate: %d\n"
"\tcqp: %d\n"
"\tkeyint: %d\n"
"\tpreset: %s\n"
"\tprofile: %s\n"
"\tb-frames: %d\n"
"\twidth: %d\n"
"\theight: %d\n"
"\toverriding params: %s",
Copy link
Member

Choose a reason for hiding this comment

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

Pre-analysis state should be logged.
Normalize spacing so that the longest line uses one space after the colon:

label: %format_specifier

Comment on lines 1531 to 1540
"\trate_control: %s\n"
"\tbitrate: %d\n"
"\tcqp: %d\n"
"\tkeyint: %d\n"
"\tpreset: %s\n"
"\tprofile: %s\n"
"\tb-frames: %d\n"
"\twidth: %d\n"
"\theight: %d\n"
"\toverriding params: %s",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we need to rename params to overriding params.

Comment on lines 1892 to 1900
"\trate_control: %s\n"
"\tbitrate: %d\n"
"\tcqp: %d\n"
"\tkeyint: %d\n"
"\tpreset: %s\n"
"\tprofile: %s\n"
"\twidth: %d\n"
"\theight: %d\n"
"\toverriding params: %s",
Copy link
Member

Choose a reason for hiding this comment

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

See other comment about pre-analysis, spacing, and params.

Comment on lines 2300 to 2310
"\trate_control: %s\n"
"\tbitrate: %d\n"
"\tcqp: %d\n"
"\tkeyint: %d\n"
"\tpreset: %s\n"
"\tprofile: %s\n"
"\twidth: %d\n"
"\theight: %d\n"
"\tscreen content tools: %s\n"
"\tpalette mode: %s\n"
"\toverriding params: %s",
Copy link
Member

Choose a reason for hiding this comment

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

See other comment about pre-analysis, spacing, and params.

rc_str, bitrate, qp, gop_size, preset, profile, enc->cx, enc->cy,
ffmpeg_opts);
"true", "true", ffmpeg_opts);
Copy link
Member

Choose a reason for hiding this comment

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

Although I'd previously resolved the thread, it seems that the original feedback on this line was never addressed. Don't hard-code logged settings values, even if you're hard-coding them when setting them. Just actually get the setting so that if the setting itself isn't hard-coded in the future, we don't have to wonder why the logs don't make sense.

@RytoEX RytoEX self-assigned this Sep 13, 2024
Copy link

@Ryu0833 Ryu0833 left a comment

Choose a reason for hiding this comment

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

Fix property "ENABLE_VBAQ" always true even when using "rc=CBR" for AVC and HEVC

Comment on lines 1495 to 1404
if (rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_CONSTANT_QP &&
rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR &&
rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR)
set_avc_property(enc, ENABLE_VBAQ, true);

Copy link

Choose a reason for hiding this comment

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

for working correctly

Suggested change
if (rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_CONSTANT_QP &&
rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR &&
rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR)
set_avc_property(enc, ENABLE_VBAQ, true);
if (rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_CONSTANT_QP &&
rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR &&
rc != AMF_VIDEO_ENCODER_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR) {
set_avc_property(enc, ENABLE_VBAQ, true);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 1862 to 1738
if (rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_CONSTANT_QP &&
rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR &&
rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR)
set_hevc_property(enc, ENABLE_VBAQ, true);

Copy link

Choose a reason for hiding this comment

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

same for HEVC

Suggested change
if (rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_CONSTANT_QP &&
rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR &&
rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR)
set_hevc_property(enc, ENABLE_VBAQ, true);
if (rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_CONSTANT_QP &&
rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_HIGH_QUALITY_VBR &&
rc != AMF_VIDEO_ENCODER_HEVC_RATE_CONTROL_METHOD_HIGH_QUALITY_CBR) {
set_hevc_property(enc, ENABLE_VBAQ, true);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch 3 times, most recently from 1f354ea to 24bff6c Compare October 30, 2024 21:28
@rhutsAMD rhutsAMD force-pushed the amf-new-default-encoding-settings branch from 24bff6c to afa9401 Compare October 30, 2024 21:29
@rhutsAMD rhutsAMD requested a review from RytoEX October 30, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.