Re: [FFmpeg-devel] [PATCH] Multi NVENC Split Frame Encoding in HEVC and AV1
Hi Timo,
Thank you for your review. Sorry for both superfulous spaces. Thank you for
pointing them out, I will be more careful on the next patches. Please check my
answers below.
Best regards,
Diego
From: Timo Rothenpieler
Date: Thursday, 11. April 2024 at 15:50
To: FFmpeg development discussions and patches
Cc: Diego Felix de Souza
Subject: Re: [FFmpeg-devel] [PATCH] Multi NVENC Split Frame Encoding in HEVC
and AV1
External email: Use caution opening links or attachments
On 11/04/2024 13:58, Diego Felix de Souza via ffmpeg-devel wrote:
> From: Diego Felix de Souza
>
> When Split frame encoding is enabled, each input frame is partitioned into
> horizontal strips which are encoded independently and simultaneously by
> separate NVENCs, usually resulting in increased encoding speed compared to
> single NVENC encoding.
> ---
> libavcodec/nvenc.c | 16
> libavcodec/nvenc.h | 2 ++
> libavcodec/nvenc_av1.c | 8
> libavcodec/nvenc_hevc.c | 8
> 4 files changed, 34 insertions(+)
>
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index b6c5ed3e6b..f4d0d21715 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -1696,6 +1696,22 @@ FF_ENABLE_DEPRECATION_WARNINGS
> if (ctx->weighted_pred == 1)
> ctx->init_encode_params.enableWeightedPrediction = 1;
>
> +#ifdef NVENC_HAVE_SPLIT_FRAME_ENCODING
> +if (avctx->codec->id != AV_CODEC_ID_H264 )
nit: superfluous space
In general, this check is probably not necessary, given the h264 encoder
does not have this AVOption so it'll always be zero-initialized there,
which should be fine?
Or would setting it to the equivalent of "auto" cause issues on h264?
You are right, this check is not necessary and the SDK will disregard the any
value in the
splitEncodeMode for H.264. I put it to be clear that this functionality is not
available for
H.264. I will remove it in the next patch for the sake of simplicity.
> +ctx->init_encode_params.splitEncodeMode = ctx->split_encode_mode;
> +
> +if ((ctx->split_encode_mode != NV_ENC_SPLIT_DISABLE_MODE) &&
> +((ctx->weighted_pred == 1) && (avctx->codec->id == AV_CODEC_ID_HEVC ))) {
I think the ffmpeg recommended style would indent this line by 4 spaces.
What I usually do, even though it's technically against
style-guidelines, is indent the extra lines by 4, and then put the
opening { on its own line, since imo it greatly improves readability.
I remove the 4 spaces to fit in an 80 caracters line. However, I agree that it
would
improve readability like you suggested. I will change it in the next version.
also a superfluous space at the end.
> +av_log(avctx, AV_LOG_WARNING, "Split encoding is not "
> +"supported if any of the following features: weighted
> prediction, "
> +"alpha layer encoding, subframe mode, output into video memory "
> +"buffer, picture timing/buffering period SEI message insertion "
> +"with DX12 interface are enabled in case of HEVC. For AV1, split
> "
> +"encoding is not supported when output into video memory buffer "
> +"is enabled.\n");
This message is a bit of a mouthful, and most of it is not applicable to
users.
I'd maybe shorten it to something like:
"Split encoding not supported if weighted prediction enabled."
Since that's the only option that can actually cause issues with the
current nvenc.c implementation.
Sure, I will short the warning message.
> +}
> +#endif
> +
> if (ctx->bluray_compat) {
> ctx->aud = 1;
> ctx->dpb_size = FFMIN(FFMAX(avctx->refs, 0), 6);
> diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h
> index 85ecaf1b5f..09de00badc 100644
> --- a/libavcodec/nvenc.h
> +++ b/libavcodec/nvenc.h
> @@ -81,6 +81,7 @@ typedef void ID3D11Device;
> // SDK 12.1 compile time feature checks
> #if NVENCAPI_CHECK_VERSION(12, 1)
> #define NVENC_NO_DEPRECATED_RC
> +#define NVENC_HAVE_SPLIT_FRAME_ENCODING
> #endif
>
> // SDK 12.2 compile time feature checks
> @@ -280,6 +281,7 @@ typedef struct NvencContext
> int tf_level;
> int lookahead_level;
> int unidir_b;
> +int split_encode_mode;
> } NvencContext;
>
> int ff_nvenc_encode_init(AVCodecContext *avctx);
> diff --git a/libavcodec/nvenc_av1.c b/libavcodec/nvenc_av1.c
> index d37ee07bff..45dc3c26e0 100644
> --- a/libavcodec/nvenc_av1.c
> +++ b/libavcodec/nvenc_av1.c
> @@ -157,6 +157,14 @@ static const AVOption options[] = {
> { "1","", 0,
Re: [FFmpeg-devel] [PATCH] Multi NVENC Split Frame Encoding in HEVC and AV1
On 11/04/2024 13:58, Diego Felix de Souza via ffmpeg-devel wrote:
From: Diego Felix de Souza
When Split frame encoding is enabled, each input frame is partitioned into
horizontal strips which are encoded independently and simultaneously by
separate NVENCs, usually resulting in increased encoding speed compared to
single NVENC encoding.
---
libavcodec/nvenc.c | 16
libavcodec/nvenc.h | 2 ++
libavcodec/nvenc_av1.c | 8
libavcodec/nvenc_hevc.c | 8
4 files changed, 34 insertions(+)
diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index b6c5ed3e6b..f4d0d21715 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -1696,6 +1696,22 @@ FF_ENABLE_DEPRECATION_WARNINGS
if (ctx->weighted_pred == 1)
ctx->init_encode_params.enableWeightedPrediction = 1;
+#ifdef NVENC_HAVE_SPLIT_FRAME_ENCODING
+if (avctx->codec->id != AV_CODEC_ID_H264 )
nit: superfluous space
In general, this check is probably not necessary, given the h264 encoder
does not have this AVOption so it'll always be zero-initialized there,
which should be fine?
Or would setting it to the equivalent of "auto" cause issues on h264?
+ctx->init_encode_params.splitEncodeMode = ctx->split_encode_mode;
+
+if ((ctx->split_encode_mode != NV_ENC_SPLIT_DISABLE_MODE) &&
+((ctx->weighted_pred == 1) && (avctx->codec->id == AV_CODEC_ID_HEVC ))) {
I think the ffmpeg recommended style would indent this line by 4 spaces.
What I usually do, even though it's technically against
style-guidelines, is indent the extra lines by 4, and then put the
opening { on its own line, since imo it greatly improves readability.
also a superfluous space at the end.
+av_log(avctx, AV_LOG_WARNING, "Split encoding is not "
+"supported if any of the following features: weighted prediction, "
+"alpha layer encoding, subframe mode, output into video memory "
+"buffer, picture timing/buffering period SEI message insertion "
+"with DX12 interface are enabled in case of HEVC. For AV1, split "
+"encoding is not supported when output into video memory buffer "
+"is enabled.\n");
This message is a bit of a mouthful, and most of it is not applicable to
users.
I'd maybe shorten it to something like:
"Split encoding not supported if weighted prediction enabled."
Since that's the only option that can actually cause issues with the
current nvenc.c implementation.
+}
+#endif
+
if (ctx->bluray_compat) {
ctx->aud = 1;
ctx->dpb_size = FFMIN(FFMAX(avctx->refs, 0), 6);
diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h
index 85ecaf1b5f..09de00badc 100644
--- a/libavcodec/nvenc.h
+++ b/libavcodec/nvenc.h
@@ -81,6 +81,7 @@ typedef void ID3D11Device;
// SDK 12.1 compile time feature checks
#if NVENCAPI_CHECK_VERSION(12, 1)
#define NVENC_NO_DEPRECATED_RC
+#define NVENC_HAVE_SPLIT_FRAME_ENCODING
#endif
// SDK 12.2 compile time feature checks
@@ -280,6 +281,7 @@ typedef struct NvencContext
int tf_level;
int lookahead_level;
int unidir_b;
+int split_encode_mode;
} NvencContext;
int ff_nvenc_encode_init(AVCodecContext *avctx);
diff --git a/libavcodec/nvenc_av1.c b/libavcodec/nvenc_av1.c
index d37ee07bff..45dc3c26e0 100644
--- a/libavcodec/nvenc_av1.c
+++ b/libavcodec/nvenc_av1.c
@@ -157,6 +157,14 @@ static const AVOption options[] = {
{ "1","", 0,
AV_OPT_TYPE_CONST, { .i64 = NV_ENC_LOOKAHEAD_LEVEL_1 }, 0, 0, VE, .unit = "lookahead_level" },
{ "2","", 0,
AV_OPT_TYPE_CONST, { .i64 = NV_ENC_LOOKAHEAD_LEVEL_2 }, 0, 0, VE, .unit = "lookahead_level" },
{ "3","", 0,
AV_OPT_TYPE_CONST, { .i64 = NV_ENC_LOOKAHEAD_LEVEL_3 }, 0, 0, VE, .unit = "lookahead_level" },
+#endif
+#ifdef NVENC_HAVE_SPLIT_FRAME_ENCODING
+{ "split_encode_mode", "Specifies the split encoding mode",
OFFSET(split_encode_mode), AV_OPT_TYPE_INT, { .i64 = NV_ENC_SPLIT_DISABLE_MODE }, 0,
NV_ENC_SPLIT_DISABLE_MODE, VE, .unit = "split_encode_mode" },
+{ "disabled", "Disabled for all configurations",
0, AV_OPT_TYPE_CONST, { .i64 = NV_ENC_SPLIT_DISABLE_MODE }, 0, 0, VE, .unit =
"split_encode_mode" },
+{ "auto", "Enabled or disabled depending on the preset and tuning
info",0, AV_OPT_TYPE_CONST, { .i64 = NV_ENC_SPLIT_AUTO_MODE }, 0, 0, VE,
.unit = "split_encode_mode" },
+{ "forced","Enabled with number of horizontal strips selected by the
driver",0, AV_OPT_TYPE_CONST, { .i64 = NV_ENC_SPLIT_AUTO_FORCED_MODE }, 0, 0, VE, .unit
= "split_encode_mode" },
+{ "2", "Enabled with number of horizontal strips forced to 2 when
