Refactor and enhance NVENC codec profile options#2018
Refactor and enhance NVENC codec profile options#2018uknunknown wants to merge 1 commit intotvheadend:masterfrom
Conversation
Summary of ChangesHello @uknunknown, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors and enhances the NVENC codec profile options, introducing support for newer NVENC API features like P1-P7 presets and tuning information. It standardizes rate control settings, adds granular control over QP, max bitrate, and bitrate scaling, and improves the user interface by dynamically enabling or disabling relevant fields based on the selected rate control mode. These changes aim to provide more comprehensive and user-friendly control over NVIDIA hardware accelerated encoding. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
This PR has many changes in NVENC. I tested most of them, as much as I could; but we should give this PR few weeks in order to be tested by users more extensively. |
There was a problem hiding this comment.
Code Review
This pull request provides a significant and valuable refactoring of the NVENC codec options. It modernizes the implementation by using constants from nvEncodeAPI.h, adds support for new presets and tuning options, and aligns parameter handling with other codecs like VAAPI. The changes improve maintainability and introduce new features. My review includes a few suggestions to further improve code quality: fixing a potential logic bug in bitrate handling and cleaning up the source code by removing large blocks of commented-out reference text.
| int int_bitrate = (int)((double)(self->bit_rate) * 1024.0 * (1.0 + (self->bit_rate_scale_factor * ((double)(self->size.den) - 480.0) / 480.0))); | ||
| int int_max_bitrate = (int)((double)(self->max_bit_rate) * 1024.0 * (1.0 + (self->bit_rate_scale_factor * ((double)(self->size.den) - 480.0) / 480.0))); | ||
| // force max_bitrate to be >= with bitrate (to avoid crash) | ||
| if (int_bitrate > int_max_bitrate) { |
There was a problem hiding this comment.
There's a potential logic issue here. The check int_bitrate > int_max_bitrate runs even if self->max_bit_rate is 0 (which means 'skip'). In that case, int_max_bitrate is 0, and if a bit_rate is set, the condition becomes true, leading to an error log. However, the adjusted int_max_bitrate is never passed to FFmpeg because the check on line 175, if (self->max_bit_rate), will be false. This means the intended crash prevention might not work. The check should only be performed when max_bit_rate is explicitly set by the user.
if (self->max_bit_rate > 0 && int_bitrate > int_max_bitrate) {There was a problem hiding this comment.
when bitrate is 0 is expected to max_bitrate to be forced to 0. Fixed the printing: only print bitrate and max_bitrate when is used.
| //#define NV_ENC_PARAMS_RC_CONSTQP 0x0 /**< Constant QP mode */ | ||
| //#define NV_ENC_PARAMS_RC_VBR 0x1 /**< Variable bitrate mode */ | ||
| //#define NV_ENC_PARAMS_RC_CBR 0x2 /**< Constant bitrate mode */ |
There was a problem hiding this comment.
These defines for rate control modes are commented out and appear to be superseded by including <ffnvcodec/nvEncodeAPI.h>, which provides the official constants. To improve code clarity and remove obsolete code, these commented-out lines should be deleted.
There was a problem hiding this comment.
This is intentional ... so show the the value imported.
There was a problem hiding this comment.
Please don't do that, it's not good practice to add commented out code. If someone wants to know what it's for, most IDEs can easily show the definition with a single click.
Think about it this way: What would happen if this is done everywhere in the codebase? The code would probably be hard to read (and even hard to find, as there would be lots of comments).
| // this is a little outdated but is worth mentioning: | ||
| // https://blog.wmspanel.com/2016/10/nvidia-nvenc-gpu-nimble-streamer-transcoder.html | ||
| // ffmpeg -hide_banner -h encoder=h264_nvenc | ||
| //Encoder h264_nvenc [NVIDIA NVENC H.264 encoder]: | ||
| // General capabilities: dr1 delay hardware | ||
| // Threading capabilities: none | ||
| // Supported hardware devices: cuda cuda | ||
| // Supported pixel formats: yuv420p nv12 p010le yuv444p p016le yuv444p16le bgr0 rgb0 cuda | ||
| //h264_nvenc AVOptions: | ||
| // -preset <int> E..V....... Set the encoding preset (from 0 to 18) (default p4) | ||
| // default 0 E..V....... | ||
| // slow 1 E..V....... hq 2 passes | ||
| // medium 2 E..V....... hq 1 pass | ||
| // fast 3 E..V....... hp 1 pass | ||
| // hp 4 E..V....... | ||
| // hq 5 E..V....... | ||
| // bd 6 E..V....... | ||
| // ll 7 E..V....... low latency | ||
| // llhq 8 E..V....... low latency hq | ||
| // llhp 9 E..V....... low latency hp | ||
| // lossless 10 E..V....... | ||
| // losslesshp 11 E..V....... | ||
| // p1 12 E..V....... fastest (lowest quality) | ||
| // p2 13 E..V....... faster (lower quality) | ||
| // p3 14 E..V....... fast (low quality) | ||
| // p4 15 E..V....... medium (default) | ||
| // p5 16 E..V....... slow (good quality) | ||
| // p6 17 E..V....... slower (better quality) | ||
| // p7 18 E..V....... slowest (best quality) | ||
| // -tune <int> E..V....... Set the encoding tuning info (from 1 to 4) (default hq) | ||
| // hq 1 E..V....... High quality | ||
| // ll 2 E..V....... Low latency | ||
| // ull 3 E..V....... Ultra low latency | ||
| // lossless 4 E..V....... Lossless | ||
| // -profile <int> E..V....... Set the encoding profile (from 0 to 3) (default main) | ||
| // baseline 0 E..V....... | ||
| // main 1 E..V....... | ||
| // high 2 E..V....... | ||
| // high444p 3 E..V....... | ||
| // -level <int> E..V....... Set the encoding level restriction (from 0 to 62) (default auto) | ||
| // auto 0 E..V....... | ||
| // 1 10 E..V....... | ||
| // 1.0 10 E..V....... | ||
| // 1b 9 E..V....... | ||
| // 1.0b 9 E..V....... | ||
| // 1.1 11 E..V....... | ||
| // 1.2 12 E..V....... | ||
| // 1.3 13 E..V....... | ||
| // 2 20 E..V....... | ||
| // 2.0 20 E..V....... | ||
| // 2.1 21 E..V....... | ||
| // 2.2 22 E..V....... | ||
| // 3 30 E..V....... | ||
| // 3.0 30 E..V....... | ||
| // 3.1 31 E..V....... | ||
| // 3.2 32 E..V....... | ||
| // 4 40 E..V....... | ||
| // 4.0 40 E..V....... | ||
| // 4.1 41 E..V....... | ||
| // 4.2 42 E..V....... | ||
| // 5 50 E..V....... | ||
| // 5.0 50 E..V....... | ||
| // 5.1 51 E..V....... | ||
| // 5.2 52 E..V....... | ||
| // 6.0 60 E..V....... | ||
| // 6.1 61 E..V....... | ||
| // 6.2 62 E..V....... | ||
| // -rc <int> E..V....... Override the preset rate-control (from -1 to INT_MAX) (default -1) | ||
| // constqp 0 E..V....... Constant QP mode | ||
| // vbr 1 E..V....... Variable bitrate mode | ||
| // cbr 2 E..V....... Constant bitrate mode | ||
| // vbr_minqp 8388612 E..V....... Variable bitrate mode with MinQP (deprecated) | ||
| // ll_2pass_quality 8388616 E..V....... Multi-pass optimized for image quality (deprecated) | ||
| // ll_2pass_size 8388624 E..V....... Multi-pass optimized for constant frame size (deprecated) | ||
| // vbr_2pass 8388640 E..V....... Multi-pass variable bitrate mode (deprecated) | ||
| // cbr_ld_hq 8388616 E..V....... Constant bitrate low delay high quality mode | ||
| // cbr_hq 8388624 E..V....... Constant bitrate high quality mode | ||
| // vbr_hq 8388640 E..V....... Variable bitrate high quality mode | ||
| // -rc-lookahead <int> E..V....... Number of frames to look ahead for rate-control (from 0 to INT_MAX) (default 0) | ||
| // -surfaces <int> E..V....... Number of concurrent surfaces (from 0 to 64) (default 0) | ||
| // -cbr <boolean> E..V....... Use cbr encoding mode (default false) | ||
| // -2pass <boolean> E..V....... Use 2pass encoding mode (default auto) | ||
| // -gpu <int> E..V....... Selects which NVENC capable GPU to use. First GPU is 0, second is 1, and so on. (from -2 to INT_MAX) (default any) | ||
| // any -1 E..V....... Pick the first device available | ||
| // list -2 E..V....... List the available devices | ||
| // -delay <int> E..V....... Delay frame output by the given amount of frames (from 0 to INT_MAX) (default INT_MAX) | ||
| // -no-scenecut <boolean> E..V....... When lookahead is enabled, set this to 1 to disable adaptive I-frame insertion at scene cuts (default false) | ||
| // -forced-idr <boolean> E..V....... If forcing keyframes, force them as IDR frames. (default false) | ||
| // -b_adapt <boolean> E..V....... When lookahead is enabled, set this to 0 to disable adaptive B-frame decision (default true) | ||
| // -spatial-aq <boolean> E..V....... set to 1 to enable Spatial AQ (default false) | ||
| // -spatial_aq <boolean> E..V....... set to 1 to enable Spatial AQ (default false) | ||
| // -temporal-aq <boolean> E..V....... set to 1 to enable Temporal AQ (default false) | ||
| // -temporal_aq <boolean> E..V....... set to 1 to enable Temporal AQ (default false) | ||
| // -zerolatency <boolean> E..V....... Set 1 to indicate zero latency operation (no reordering delay) (default false) | ||
| // -nonref_p <boolean> E..V....... Set this to 1 to enable automatic insertion of non-reference P-frames (default false) | ||
| // -strict_gop <boolean> E..V....... Set 1 to minimize GOP-to-GOP rate fluctuations (default false) | ||
| // -aq-strength <int> E..V....... When Spatial AQ is enabled, this field is used to specify AQ strength. AQ strength scale is from 1 (low) - 15 (aggressive) (from 1 to 15) (default 8) | ||
| // -cq <float> E..V....... Set target quality level (0 to 51, 0 means automatic) for constant quality mode in VBR rate control (from 0 to 51) (default 0) | ||
| // -aud <boolean> E..V....... Use access unit delimiters (default false) | ||
| // -bluray-compat <boolean> E..V....... Bluray compatibility workarounds (default false) | ||
| // -init_qpP <int> E..V....... Initial QP value for P frame (from -1 to 51) (default -1) | ||
| // -init_qpB <int> E..V....... Initial QP value for B frame (from -1 to 51) (default -1) | ||
| // -init_qpI <int> E..V....... Initial QP value for I frame (from -1 to 51) (default -1) | ||
| // -qp <int> E..V....... Constant quantization parameter rate control method (from -1 to 51) (default -1) | ||
| // -weighted_pred <int> E..V....... Set 1 to enable weighted prediction (from 0 to 1) (default 0) | ||
| // -coder <int> E..V....... Coder type (from -1 to 2) (default default) | ||
| // default -1 E..V....... | ||
| // auto 0 E..V....... | ||
| // cabac 1 E..V....... | ||
| // cavlc 2 E..V....... | ||
| // ac 1 E..V....... | ||
| // vlc 2 E..V....... | ||
| // -b_ref_mode <int> E..V....... Use B frames as references (from 0 to 2) (default disabled) | ||
| // disabled 0 E..V....... B frames will not be used for reference | ||
| // each 1 E..V....... Each B frame will be used for reference | ||
| // middle 2 E..V....... Only (number of B frames)/2 will be used for reference | ||
| // -a53cc <boolean> E..V....... Use A53 Closed Captions (if available) (default true) | ||
| // -dpb_size <int> E..V....... Specifies the DPB size used for encoding (0 means automatic) (from 0 to INT_MAX) (default 0) | ||
| // -multipass <int> E..V....... Set the multipass encoding (from 0 to 2) (default disabled) | ||
| // disabled 0 E..V....... Single Pass | ||
| // qres 1 E..V....... Two Pass encoding is enabled where first Pass is quarter resolution | ||
| // fullres 2 E..V....... Two Pass encoding is enabled where first Pass is full resolution | ||
| // -ldkfs <int> E..V....... Low delay key frame scale; Specifies the Scene Change frame size increase allowed in case of single frame VBV and CBR (from 0 to 255) (default 0) |
There was a problem hiding this comment.
This large comment block contains the output of ffmpeg -h encoder=h264_nvenc. While this information is useful for developers, embedding it directly in the source code makes the file difficult to read and maintain. It would be better to move this reference material to external documentation, such as a project wiki or a file in a docs folder.
There was a problem hiding this comment.
@Flole998 is your call. Let me know if you want me to remove it.
There was a problem hiding this comment.
Please remove it, there's no need to include part of some third-party documentation that will inevitably be out of date at some point
| // ffmpeg -hide_banner -h encoder=hevc_nvenc | ||
| //Encoder hevc_nvenc [NVIDIA NVENC hevc encoder]: | ||
| // General capabilities: dr1 delay hardware | ||
| // Threading capabilities: none | ||
| // Supported hardware devices: cuda cuda | ||
| // Supported pixel formats: yuv420p nv12 p010le yuv444p p016le yuv444p16le bgr0 rgb0 cuda | ||
| //hevc_nvenc AVOptions: | ||
| // -preset <int> E..V....... Set the encoding preset (from 0 to 18) (default p4) | ||
| // default 0 E..V....... | ||
| // slow 1 E..V....... hq 2 passes | ||
| // medium 2 E..V....... hq 1 pass | ||
| // fast 3 E..V....... hp 1 pass | ||
| // hp 4 E..V....... | ||
| // hq 5 E..V....... | ||
| // bd 6 E..V....... | ||
| // ll 7 E..V....... low latency | ||
| // llhq 8 E..V....... low latency hq | ||
| // llhp 9 E..V....... low latency hp | ||
| // lossless 10 E..V....... lossless | ||
| // losslesshp 11 E..V....... lossless hp | ||
| // p1 12 E..V....... fastest (lowest quality) | ||
| // p2 13 E..V....... faster (lower quality) | ||
| // p3 14 E..V....... fast (low quality) | ||
| // p4 15 E..V....... medium (default) | ||
| // p5 16 E..V....... slow (good quality) | ||
| // p6 17 E..V....... slower (better quality) | ||
| // p7 18 E..V....... slowest (best quality) | ||
| // -tune <int> E..V....... Set the encoding tuning info (from 1 to 4) (default hq) | ||
| // hq 1 E..V....... High quality | ||
| // ll 2 E..V....... Low latency | ||
| // ull 3 E..V....... Ultra low latency | ||
| // lossless 4 E..V....... Lossless | ||
| // -profile <int> E..V....... Set the encoding profile (from 0 to 4) (default main) | ||
| // main 0 E..V....... | ||
| // main10 1 E..V....... | ||
| // rext 2 E..V....... | ||
| // -level <int> E..V....... Set the encoding level restriction (from 0 to 186) (default auto) | ||
| // auto 0 E..V....... | ||
| // 1 30 E..V....... | ||
| // 1.0 30 E..V....... | ||
| // 2 60 E..V....... | ||
| // 2.0 60 E..V....... | ||
| // 2.1 63 E..V....... | ||
| // 3 90 E..V....... | ||
| // 3.0 90 E..V....... | ||
| // 3.1 93 E..V....... | ||
| // 4 120 E..V....... | ||
| // 4.0 120 E..V....... | ||
| // 4.1 123 E..V....... | ||
| // 5 150 E..V....... | ||
| // 5.0 150 E..V....... | EDB7||
| // 5.1 153 E..V....... | ||
| // 5.2 156 E..V....... | ||
| // 6 180 E..V....... | ||
| // 6.0 180 E..V....... | ||
| // 6.1 183 E..V....... | ||
| // 6.2 186 E..V....... | ||
| // -tier <int> E..V....... Set the encoding tier (from 0 to 1) (default main) | ||
| // main 0 E..V....... | ||
| // high 1 E..V....... | ||
| // -rc <int> E..V....... Override the preset rate-control (from -1 to INT_MAX) (default -1) | ||
| // constqp 0 E..V....... Constant QP mode | ||
| // vbr 1 E..V....... Variable bitrate mode | ||
| // cbr 2 E..V....... Constant bitrate mode | ||
| // vbr_minqp 8388612 E..V....... Variable bitrate mode with MinQP (deprecated) | ||
| // ll_2pass_quality 8388616 E..V....... Multi-pass optimized for image quality (deprecated) | ||
| // ll_2pass_size 8388624 E..V....... Multi-pass optimized for constant frame size (deprecated) | ||
| // vbr_2pass 8388640 E..V....... Multi-pass variable bitrate mode (deprecated) | ||
| // cbr_ld_hq 8388616 E..V....... Constant bitrate low delay high quality mode | ||
| // cbr_hq 8388624 E..V....... Constant bitrate high quality mode | ||
| // vbr_hq 8388640 E..V....... Variable bitrate high quality mode | ||
| // -rc-lookahead <int> E..V....... Number of frames to look ahead for rate-control (from 0 to INT_MAX) (default 0) | ||
| // -surfaces <int> E..V....... Number of concurrent surfaces (from 0 to 64) (default 0) | ||
| // -cbr <boolean> E..V....... Use cbr encoding mode (default false) | ||
| // -2pass <boolean> E..V....... Use 2pass encoding mode (default auto) | ||
| // -gpu <int> E..V....... Selects which NVENC capable GPU to use. First GPU is 0, second is 1, and so on. (from -2 to INT_MAX) (default any) | ||
| // any -1 E..V....... Pick the first device available | ||
| // list -2 E..V....... List the available devices | ||
| // -delay <int> E..V....... Delay frame output by the given amount of frames (from 0 to INT_MAX) (default INT_MAX) | ||
| // -no-scenecut <boolean> E..V....... When lookahead is enabled, set this to 1 to disable adaptive I-frame insertion at scene cuts (default false) | ||
| // -forced-idr <boolean> E..V....... If forcing keyframes, force them as IDR frames. (default false) | ||
| // -spatial_aq <boolean> E..V....... set to 1 to enable Spatial AQ (default false) | ||
| // -spatial-aq <boolean> E..V....... set to 1 to enable Spatial AQ (default false) | ||
| // -temporal_aq <boolean> E..V....... set to 1 to enable Temporal AQ (default false) | ||
| // -temporal-aq <boolean> E..V....... set to 1 to enable Temporal AQ (default false) | ||
| // -zerolatency <boolean> E..V....... Set 1 to indicate zero latency operation (no reordering delay) (default false) | ||
| // -nonref_p <boolean> E..V....... Set this to 1 to enable automatic insertion of non-reference P-frames (default false) | ||
| // -strict_gop <boolean> E..V....... Set 1 to minimize GOP-to-GOP rate fluctuations (default false) | ||
| // -aq-strength <int> E..V....... When Spatial AQ is enabled, this field is used to specify AQ strength. AQ strength scale is from 1 (low) - 15 (aggressive) (from 1 to 15) (default 8) | ||
| // -cq <float> E..V....... Set target quality level (0 to 51, 0 means automatic) for constant quality mode in VBR rate control (from 0 to 51) (default 0) | ||
| // -aud <boolean> E..V....... Use access unit delimiters (default false) | ||
| // -bluray-compat <boolean> E..V....... Bluray compatibility workarounds (default false) | ||
| // -init_qpP <int> E..V....... Initial QP value for P frame (from -1 to 51) (default -1) | ||
| // -init_qpB <int> E..V....... Initial QP value for B frame (from -1 to 51) (default -1) | ||
| // -init_qpI <int> E..V....... Initial QP value for I frame (from -1 to 51) (default -1) | ||
| // -qp <int> E..V....... Constant quantization parameter rate control method (from -1 to 51) (default -1) | ||
| // -weighted_pred <int> E..V....... Set 1 to enable weighted prediction (from 0 to 1) (default 0) | ||
| // -b_ref_mode <int> E..V....... Use B frames as references (from 0 to 2) (default disabled) | ||
| // disabled 0 E..V....... B frames will not be used for reference | ||
| // each 1 E..V....... Each B frame will be used for reference | ||
| // middle 2 E..V....... Only (number of B frames)/2 will be used for reference | ||
| // -a53cc <boolean> E..V....... Use A53 Closed Captions (if available) (default true) | ||
| // -s12m_tc <boolean> E..V....... Use timecode (if available) (default false) | ||
| // -dpb_size <int> E..V....... Specifies the DPB size used for encoding (0 means automatic) (from 0 to INT_MAX) (default 0) | ||
| // -multipass <int> E..V....... Set the multipass encoding (from 0 to 2) (default disabled) | ||
| // disabled 0 E..V....... Single Pass | ||
| // qres 1 E..V....... Two Pass encoding is enabled where first Pass is quarter resolution | ||
| // fullres 2 E..V....... Two Pass encoding is enabled where first Pass is full resolution | ||
| // -ldkfs <int> E..V....... Low delay key frame scale; Specifies the Scene Change frame size increase allowed in case of single frame VBV and CBR (from 0 to 255) (default 0) |
There was a problem hiding this comment.
Similar to the H.264 encoder function, this large comment block with ffmpeg -h output for hevc_nvenc clutters the source file. For better code readability and maintainability, this should be moved to external documentation.
There was a problem hiding this comment.
@Flole998 is your call. Let me know if you want me to remove it.
fd7c88f to
6a01206
Compare
|
/gemini can you review the PR now? |
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring of the NVENC codec options. It aligns them with the newer nvEncodeAPI.h constants, introduces new presets and tuning options, and makes the configuration more consistent with the VAAPI implementation. The changes also improve the user interface by dynamically enabling/disabling options based on the selected rate control mode.
My review has identified one critical issue regarding the redefinition of rate control mode constants, which would cause incorrect values to be passed to FFmpeg. I've also provided a few suggestions to improve the readability and maintainability of the new JavaScript code for the UI.
Overall, this is a great enhancement. Once the critical issue is addressed, this PR will be in good shape.
| #define RC_MODE_DEPRECATED 0x800000 //< Rate control mode is deprecated | ||
| #define NV_ENC_PARAMS_RC_VBR_MINQP 0x800004 //< Variable bitrate mode with MinQP | ||
| #define NV_ENC_PARAMS_RC_CBR_LOWDELAY_HQ 0x800008 //< Constant bitrate low delay high quality mode | ||
| #define NV_ENC_PARAMS_RC_CBR_HQ 0x800010 //< Multi-pass constant bitrate mode | ||
| #define NV_ENC_PARAMS_RC_VBR_HQ 0x800020 //< Multi-pass variable bitrate mode |
There was a problem hiding this comment.
These macros redefine constants from <ffnvcodec/nvEncodeAPI.h> with incorrect values. The original values from the header are bitwise OR-ed with 0x800000. These modified values are then passed to FFmpeg, which will not recognize them, leading to incorrect behavior or errors. Please remove these redefinitions and use the original constants from the header file. The deprecated status is already indicated in the UI strings, so this custom value scheme is unnecessary.
src/webui/static/app/codec.js
Outdated
| function update_rc_settings_nvenc(form, bit_rate_value, max_bit_rate_value, bit_rate_scale_factor_value, qp_value) { | ||
| if (form.findField('bit_rate') && form.findField('max_bit_rate') && | ||
| form.findField('bit_rate_scale_factor') && form.findField('qp')) | ||
| { | ||
| form.findField('bit_rate').setDisabled(bit_rate_value); | ||
| form.findField('max_bit_rate').setDisabled(max_bit_rate_value); | ||
| form.findField('bit_rate_scale_factor').setDisabled(bit_rate_scale_factor_value); | ||
| form.findField('qp').setDisabled(qp_value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function can be made more readable and slightly more efficient by caching the results of form.findField() in local variables. This avoids repeated lookups and makes the code cleaner.
function update_rc_settings_nvenc(form, bit_rate_value, max_bit_rate_value, bit_rate_scale_factor_value, qp_value) {
var bit_rate_field = form.findField('bit_rate');
var max_bit_rate_field = form.findField('max_bit_rate');
var bit_rate_scale_factor_field = form.findField('bit_rate_scale_factor');
var qp_field = form.findField('qp');
if (bit_rate_field && max_bit_rate_field && bit_rate_scale_factor_field && qp_field)
{
bit_rate_field.setDisabled(bit_rate_value);
max_bit_rate_field.setDisabled(max_bit_rate_value);
bit_rate_scale_factor_field.setDisabled(bit_rate_scale_factor_value);
qp_field.setDisabled(qp_value);
}
}| function filter_based_on_rc_nvenc(form) { | ||
| if (form.findField('rc_mode')) { | ||
| switch (form.findField('rc_mode').getValue()) { | ||
| case -1: | ||
| // for auto --> let the driver decide as requested by documentation | ||
| update_rc_settings_nvenc(form, false, false, false, false); | ||
| break; | ||
| case 0: | ||
| // for constant quality: CQP we use qp | ||
| update_rc_settings_nvenc(form, true, true, true, false); | ||
| break; | ||
| case 1: | ||
| // for variable bitrate: VBR we use bitrate | ||
| update_rc_settings_nvenc(form, false, false, false, true); | ||
| break; | ||
| case 2: | ||
| // for constant bitrate: CBR we use bitrate | ||
| update_rc_settings_nvenc(form, false, true, false, true); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function can be made more readable and slightly more efficient by caching the result of form.findField('rc_mode').
function filter_based_on_rc_nvenc(form) {
var rc_mode_field = form.findField('rc_mode');
if (rc_mode_field) {
switch (rc_mode_field.getValue()) {
case -1:
// for auto --> let the driver decide as requested by documentation
update_rc_settings_nvenc(form, false, false, false, false);
break;
case 0:
// for constant quality: CQP we use qp
update_rc_settings_nvenc(form, true, true, true, false);
break;
case 1:
// for variable bitrate: VBR we use bitrate
update_rc_settings_nvenc(form, false, false, false, true);
break;
case 2:
// for constant bitrate: CBR we use bitrate
update_rc_settings_nvenc(form, false, true, false, true);
break;
}
}
}| function update_nvenc_ui(form) { | ||
| // first time we have to call this manually | ||
| filter_based_on_rc_nvenc(form); | ||
|
|
||
| // on rc_mode change | ||
| if (form.findField('rc_mode')) | ||
| form.findField('rc_mode').on('select', function(combo, record, index) { | ||
| // filter based in rc_mode | ||
| filter_based_on_rc_nvenc(form); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This function can be made more readable and slightly more efficient by caching the result of form.findField('rc_mode'). Also, it's good practice to use braces for if statements to avoid potential issues.
function update_nvenc_ui(form) {
// first time we have to call this manually
filter_based_on_rc_nvenc(form);
// on rc_mode change
var rc_mode_field = form.findField('rc_mode');
if (rc_mode_field) {
rc_mode_field.on('select', function(combo, record, index) {
// filter based in rc_mode
filter_based_on_rc_nvenc(form);
});
}
}6a01206 to
73c1727
Compare
|
/gemini can you review the PR now? |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring of the NVENC codec options. It successfully aligns them more closely with the VAAPI options, introduces new settings, and improves the overall code structure by centralizing configuration logic. The use of integer values for dictionary options instead of strings is a good improvement. The accompanying UI changes in codec.js are also a welcome addition to dynamically adjust settings based on the selected rate control mode.
However, I've identified a critical issue regarding the rate control mode constants. The code currently uses values from nvEncodeAPI.h, which do not match the values expected by FFmpeg's NVENC encoder, leading to incorrect behavior. I've also noted a few inconsistencies in the documentation comments for the new struct members. Addressing these points will ensure the new implementation is robust and maintainable.
| if ( (self->rc_mode == NV_ENC_PARAMS_RC_VBR) || (self->rc_mode == NV_ENC_PARAMS_RC_AUTO) | ||
| #if NVENCAPI_MAJOR_VERSION < 10 | ||
| || (self->rc_mode == NV_ENC_PARAMS_RC_VBR_MINQP) || (self->rc_mode == NV_ENC_PARAMS_RC_VBR_HQ) | ||
| #endif | ||
| ) { | ||
| // force max_bitrate to be >= with bitrate (to avoid crash) | ||
| if (int_bitrate > int_max_bitrate) { | ||
| tvherror_transcode(LST_NVENC, "Bitrate %d kbps is greater than Max bitrate %d kbps, increase Max bitrate to %d kbps", int_bitrate / 1024, int_max_bitrate / 1024, int_bitrate / 1024); | ||
| int_max_bitrate = int_bitrate; | ||
| } | ||
| if (int_bitrate) | ||
| tvhinfo_transcode(LST_NVENC, "Bitrate = %d kbps; Max bitrate = %d kbps", int_bitrate / 1024, int_max_bitrate / 1024); | ||
| } | ||
| else if(self->rc_mode == NV_ENC_PARAMS_RC_CBR) { | ||
| // in CBR mode max_bitrate is ignored by nvenc, so we only log bitrate | ||
| if (int_bitrate) { | ||
| tvhinfo_transcode(LST_NVENC, "Bitrate = %d kbps", int_bitrate / 1024); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for handling bitrate based on rc_mode uses constants from nvEncodeAPI.h (e.g., NV_ENC_PARAMS_RC_VBR). As mentioned in another comment, these values do not match what FFmpeg's NVENC encoder expects for its rc option. This check needs to be updated to use the correct FFmpeg integer values for rate control modes (e.g., 1 for VBR, 2 for CBR) after the constants in codec_profile_nvenc_class_rc_list are corrected.
| static const struct strtab tab[] = { | ||
| {N_("Auto"), NV_ENC_PARAMS_RC_AUTO}, | ||
| {N_("Constant QP mode"), NV_ENC_PARAMS_RC_CONSTQP}, | ||
| {N_("VBR mode"), NV_ENC_PARAMS_RC_VBR}, | ||
| {N_("CBR mode"), NV_ENC_PARAMS_RC_CBR}, | ||
| {N_("CBR LD HQ"), NV_ENC_PARAMS_RC_CBR_LD_HQ}, | ||
| {N_("CBR High Quality"), NV_ENC_PARAMS_RC_CBR_HQ}, | ||
| {N_("VBR High Quality"), NV_ENC_PARAMS_RC_VBR_HQ}, | ||
| {N_("auto"), NV_ENC_PARAMS_RC_AUTO}, | ||
| {N_("CQP"), NV_ENC_PARAMS_RC_CONSTQP}, | ||
| {N_("VBR"), NV_ENC_PARAMS_RC_VBR}, | ||
| {N_("CBR"), NV_ENC_PARAMS_RC_CBR}, | ||
| #if NVENCAPI_MAJOR_VERSION < 10 | ||
| {N_("VBR min Q (deprecated)"), NV_ENC_PARAMS_RC_VBR_MINQP}, | ||
| {N_("CBR LD HQ (deprecated)"), NV_ENC_PARAMS_RC_CBR_LOWDELAY_HQ}, | ||
| {N_("CBR High Quality (deprecated)"), NV_ENC_PARAMS_RC_CBR_HQ}, | ||
| {N_("VBR High Quality (deprecated)"), NV_ENC_PARAMS_RC_VBR_HQ}, | ||
| #endif | ||
| }; |
There was a problem hiding this comment.
There's a critical issue here with the rate control mode values. This list uses constants from nvEncodeAPI.h (e.g., NV_ENC_PARAMS_RC_VBR, NV_ENC_PARAMS_RC_CBR), which are 2 and 4 respectively. However, the rc option for FFmpeg's NVENC encoders expects different integer values (e.g., 1 for VBR, 2 for CBR).
This mismatch causes incorrect rate control modes to be selected during encoding. For example, selecting VBR in the UI will result in CBR encoding, and selecting CBR will result in an entirely different mode or an error.
To fix this, you should define and use constants that match FFmpeg's AV_NVENC_RC_* enum values, since libavcodec/nvenc.h is a private header and cannot be included directly.
For example:
#define TVH_NVENC_RC_VBR 1
#define TVH_NVENC_RC_CBR 2
// ... and so onThen use these new defines in this list. The checks in tvh_codec_profile_nvenc_open must also be updated to use these new constants.
| function filter_based_on_rc_nvenc(form) { | ||
| var rc_mode_field = form.findField('rc_mode'); | ||
| if (rc_mode_field) { | ||
| switch (rc_mode_field.getValue()) { | ||
| case -1: | ||
| // for auto --> let the driver decide as requested by documentation | ||
| update_rc_settings_nvenc(form, false, false, false, false); | ||
| break; | ||
| case 0: | ||
| // for constant quality: CQP we use qp | ||
| update_rc_settings_nvenc(form, true, true, true, false); | ||
| break; | ||
| case 1: | ||
| // for variable bitrate: VBR we use bitrate | ||
| update_rc_settings_nvenc(form, false, false, false, true); | ||
| break; | ||
| case 2: | ||
| // for constant bitrate: CBR we use bitrate | ||
| update_rc_settings_nvenc(form, false, true, false, true); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This switch statement has hardcoded case values (1 for VBR, 2 for CBR) which are correct for FFmpeg's NVENC rc option. However, the C backend is currently providing incorrect values from nvEncodeAPI.h (2 for VBR, 4 for CBR), causing this UI logic to fail.
While fixing the C backend to provide the correct values (1 and 2) will make this code work, this implementation is brittle. Any future change to the C enum values would require a corresponding change here. This is a high-severity issue as the UI does not currently function as intended for VBR and CBR modes.
|
@Flole998 I need to change the rc transfer ... integer is a weak approach because is susceptible to future issues/incompatibilities. Based on my discussion with gemini it seems like we should set rc / rc_mode using strings (not integers). |
73c1727 to
19465ce
Compare
|
/gemini can you review the PR now? |
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring and enhancement of the NVENC codec options. It aligns them with nvEncodeAPI.h constants and the VAAPI implem
37AF
entation, improving consistency and adding new features like P1-P7 presets and tuning options. The changes are well-structured and the new UI logic to dynamically show/hide options is a great improvement. I've identified a few issues, including some documentation inconsistencies, typos, and a couple of more significant problems related to the use of constants and preprocessor directives that should be addressed.
| {"hq", NV_ENC_PARAMS_RC_CONSTQP}, | ||
| {"ll", NV_ENC_PARAMS_RC_VBR}, | ||
| {"ull", NV_ENC_PARAMS_RC_CBR}, | ||
| {"lossless", NV_ENC_PARAMS_RC_VBR_MINQP}, |
There was a problem hiding this comment.
The tunetab is using NV_ENC_PARAMS_RC_* constants for its values. While the numeric values might currently align, this is confusing and brittle. It should use the NV_ENC_TUNING_INFO_* constants for clarity and to prevent potential bugs if the values ever diverge.
{"hq", NV_ENC_TUNING_INFO_HIGH_QUALITY},
{"ll", NV_ENC_TUNING_INFO_LOW_LATENCY},
{"ull", NV_ENC_TUNING_INFO_ULTRA_LOW_LATENCY},
{"lossless", NV_ENC_TUNING_INFO_LOSSLESS},| {N_("5.0"), NV_ENC_LEVEL_HEVC_5}, | ||
| {N_("5.1"), NV_ENC_LEVEL_HEVC_51}, | ||
| {N_("5.2"), NV_ENC_LEVEL_HEVC_52}, | ||
| #if NVENCAPI_MAJOR_VERSION >= 12 |
There was a problem hiding this comment.
The preprocessor condition NVENCAPI_MAJOR_VERSION >= 12 is inconsistent with other similar checks in this file (e.g., for H.264 and in tvh_codec_profile_nvenc_hevc_open) which use NVENCAPI_MAJOR_VERSION >= 10. This should be changed to >= 10 for consistency, as these HEVC levels were available before SDK version 12.
#if NVENCAPI_MAJOR_VERSION >= 10
| /** | ||
| * NVENC Maximum bitrate [maxrate] | ||
| * @note | ||
| * int: |
There was a problem hiding this comment.
The documentation for max_bit_rate indicates its type is int, but the field is declared as a double. This should be corrected to double for consistency.
* double:| * NVENC Override the preset rate-control method [rc] | ||
| * @note | ||
| * int: | ||
| * VALUE - rc mode accepted values from nvEncodeAPI.h (0,1,2) |
There was a problem hiding this comment.
The documentation for rc states that accepted values are (0,1,2). However, the code defines and uses a wider range of values (e.g., 3, 4, 8, 16, 32). The comment should be updated to be more accurate or generic to avoid confusion.
| */ | ||
| int rc; | ||
| /** | ||
| * NVENC Set target quality level for constant quality mode in VBR rate control [rc] |
There was a problem hiding this comment.
The documentation for cq incorrectly references [rc] at the end. This appears to be a copy-paste error and should be [cq].
* NVENC Set target quality level for constant quality mode in VBR rate control [cq]| else | ||
| if(self->rc == NV_ENC_PARAMS_RC_CONSTQP) { |
There was a problem hiding this comment.
The else if statement is split across two lines, which is unconventional and harms readability. It should be combined into a single else if line.
} else if(self->rc == NV_ENC_PARAMS_RC_CONSTQP) {| .id = "qp", // Don't change | ||
| .name = N_("Constant QP"), | ||
| .group = 3, | ||
| .desc = N_("Fixed QP of P frames (from 0 to 51, 0=skip).[if disabled will not send paramter to libav]"), |
There was a problem hiding this comment.
There is a typo in the description. paramter should be parameter.
.desc = N_("Fixed QP of P frames (from 0 to 51, 0=skip).[if disabled will not send parameter to libav]"),| .type = PT_DBL, | ||
| .id = "max_bit_rate", // Don't change | ||
| .name = N_("Max bitrate (kb/s)"), | ||
| .desc = N_("Maximum bitrate (0=skip).[if disabled will not send paramter to libav]"), |
There was a problem hiding this comment.
There is a typo in the description. paramter should be parameter.
.desc = N_("Maximum bitrate (0=skip).[if disabled will not send parameter to libav]"),
src/webui/static/app/codec.js
Outdated
| form.findField('bit_rate_scale_factor') && form.findField('qp')) | ||
| { |
There was a problem hiding this comment.
The opening brace { is on a new line, which is inconsistent with the coding style used elsewhere in this file. For consistency, it should be on the same line as the if statement.
form.findField('bit_rate_scale_factor') && form.findField('qp')) {| static htsmsg_t * | ||
| codec_profile_nvenc_class_preset_list(void *obj, const char *lang) | ||
| codec_profile_nvenc_class_tune_list(void *obj __attribute__((unused)), const char *lang) |
There was a problem hiding this comment.
The unused attribute is non-standard, lets not use it
|
@Flole998 let's stop here with phase 1 of improvements. Please let me know if you think something needs to be fixed. |
|
- leverage ffnvcodec/nvEncodeAPI.h constants - update the presets with new introduced values (P1-P7) - update presets deprecated with proper values - bring nvenc closer to vaapi format: qp, maxrate, bit_rate_scale_factor - defined new set of variables for new settings (with docstrincs) - update the avDictionary set from string to integer - updated the order of the settings to match the vaapi order - consolidated common settings for h264 and hevc - applied filters from vaapi to nvenc for rc_mode setting CoPilot change log: Updated NVENC codec profile handling to support new presets, rate control modes, and tuning options, with improved mapping to ffmpeg/libavcodec/nvenc.h and nvEncodeAPI.h. Added new fields for QP, bitrate scaling, and max bitrate, and restructured the UI logic in codec.js to dynamically enable or disable relevant fields based on selected rate control mode. Improved code documentation and maintainability, and updated copyright.
01c5769 to
2d3799f
Compare
|
CoPilot change log:
Updated NVENC codec profile handling to support new presets, rate control modes, and tuning options, with improved mapping to ffmpeg/libavcodec/nvenc.h and nvEncodeAPI.h. Added new fields for QP, bitrate scaling, and max bitrate, and restructured the UI logic in codec.js to dynamically enable or disable relevant fields based on selected rate control mode. Improved code documentation and maintainability, and updated copyright.
removed profile from nvenc.c because is handled by profile_class.c:
static int
tvh_codec_profile_base_open(TVHCodecProfile *self, AVDictionary **opts)
{
AV_DICT_SET_TVH_REQUIRE_META(LST_NONE, opts, 1);
// profile
if (self->profile != FF_AV_PROFILE_UNKNOWN) {
AV_DICT_SET_INT(LST_CODEC, opts, "profile", self->profile, 0);
}
return 0;
}
replaced quality with cq (because nvenc doesn't have quality)