Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* Re: [FFmpeg-devel] [PATCH] avcodec/nvenc: fix intraRefreshPeriod setting
  2023-07-06 12:00 [FFmpeg-devel] [PATCH] avcodec/nvenc: fix intraRefreshPeriod setting Zhao Zhili
@ 2023-07-06 10:23 ` Timo Rothenpieler
  2023-07-06 12:03   ` "zhilizhao(赵志立)"
  0 siblings, 1 reply; 3+ messages in thread
From: Timo Rothenpieler @ 2023-07-06 10:23 UTC (permalink / raw)
  To: ffmpeg-devel

On 06/07/2023 14:00, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Regression since 99dfdb45. intraRefreshPeriod access cc->gopLength,
> which has been overwritten to NVENC_INFINITE_GOPLENGTH before.
> 
> Fixes #10445.
> ---
>   libavcodec/nvenc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index 06579a502b..13fafcd246 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -1173,6 +1173,7 @@ static av_cold int nvenc_setup_h264_config(AVCodecContext *avctx)
>           h264->enableIntraRefresh = 1;
>           h264->intraRefreshPeriod = cc->gopLength;
>           h264->intraRefreshCnt = cc->gopLength - 1;
> +        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
>   #ifdef NVENC_HAVE_SINGLE_SLICE_INTRA_REFRESH
>           h264->singleSliceIntraRefresh = ctx->single_slice_intra_refresh;
>   #endif
> @@ -1297,6 +1298,7 @@ static av_cold int nvenc_setup_hevc_config(AVCodecContext *avctx)
>           hevc->enableIntraRefresh = 1;
>           hevc->intraRefreshPeriod = cc->gopLength;
>           hevc->intraRefreshCnt = cc->gopLength - 1;
> +        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
>   #ifdef NVENC_HAVE_SINGLE_SLICE_INTRA_REFRESH
>           hevc->singleSliceIntraRefresh = ctx->single_slice_intra_refresh;
>   #endif
> @@ -1415,6 +1417,7 @@ static av_cold int nvenc_setup_av1_config(AVCodecContext *avctx)
>           av1->enableIntraRefresh = 1;
>           av1->intraRefreshPeriod = cc->gopLength;
>           av1->intraRefreshCnt = cc->gopLength - 1;
> +        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
>   
>           av1->idrPeriod = NVENC_INFINITE_GOPLENGTH;
>       } else if (cc->gopLength > 0) {
> @@ -1619,9 +1622,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
>       if(ctx->single_slice_intra_refresh)
>           ctx->intra_refresh = 1;
>   
> -    if (ctx->intra_refresh)
> -        ctx->encode_config.gopLength = NVENC_INFINITE_GOPLENGTH;
> -
>       nvenc_recalc_surfaces(avctx);
>   
>       nvenc_setup_rate_control(avctx);

Shouldn't it be enough to move this block down a bit, below 
nvenc_setup_codec_config?
That way the codec specific configs can still access the value, and the 
logic of setting it to infinite for intra refresh mode doesn't have to 
be duplicated everywhere.

Though I'm not sure which way I prefer. Having it in the intra_refresh 
specific block in each codec definitely also has its advantage of 
keeping that logic in one place.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [FFmpeg-devel] [PATCH] avcodec/nvenc: fix intraRefreshPeriod setting
@ 2023-07-06 12:00 Zhao Zhili
  2023-07-06 10:23 ` Timo Rothenpieler
  0 siblings, 1 reply; 3+ messages in thread
From: Zhao Zhili @ 2023-07-06 12:00 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Zhao Zhili

From: Zhao Zhili <zhilizhao@tencent.com>

Regression since 99dfdb45. intraRefreshPeriod access cc->gopLength,
which has been overwritten to NVENC_INFINITE_GOPLENGTH before.

Fixes #10445.
---
 libavcodec/nvenc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index 06579a502b..13fafcd246 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -1173,6 +1173,7 @@ static av_cold int nvenc_setup_h264_config(AVCodecContext *avctx)
         h264->enableIntraRefresh = 1;
         h264->intraRefreshPeriod = cc->gopLength;
         h264->intraRefreshCnt = cc->gopLength - 1;
+        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
 #ifdef NVENC_HAVE_SINGLE_SLICE_INTRA_REFRESH
         h264->singleSliceIntraRefresh = ctx->single_slice_intra_refresh;
 #endif
@@ -1297,6 +1298,7 @@ static av_cold int nvenc_setup_hevc_config(AVCodecContext *avctx)
         hevc->enableIntraRefresh = 1;
         hevc->intraRefreshPeriod = cc->gopLength;
         hevc->intraRefreshCnt = cc->gopLength - 1;
+        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
 #ifdef NVENC_HAVE_SINGLE_SLICE_INTRA_REFRESH
         hevc->singleSliceIntraRefresh = ctx->single_slice_intra_refresh;
 #endif
@@ -1415,6 +1417,7 @@ static av_cold int nvenc_setup_av1_config(AVCodecContext *avctx)
         av1->enableIntraRefresh = 1;
         av1->intraRefreshPeriod = cc->gopLength;
         av1->intraRefreshCnt = cc->gopLength - 1;
+        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
 
         av1->idrPeriod = NVENC_INFINITE_GOPLENGTH;
     } else if (cc->gopLength > 0) {
@@ -1619,9 +1622,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if(ctx->single_slice_intra_refresh)
         ctx->intra_refresh = 1;
 
-    if (ctx->intra_refresh)
-        ctx->encode_config.gopLength = NVENC_INFINITE_GOPLENGTH;
-
     nvenc_recalc_surfaces(avctx);
 
     nvenc_setup_rate_control(avctx);
-- 
2.25.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/nvenc: fix intraRefreshPeriod setting
  2023-07-06 10:23 ` Timo Rothenpieler
@ 2023-07-06 12:03   ` "zhilizhao(赵志立)"
  0 siblings, 0 replies; 3+ messages in thread
From: "zhilizhao(赵志立)" @ 2023-07-06 12:03 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



> On Jul 6, 2023, at 18:23, Timo Rothenpieler <timo@rothenpieler.org> wrote:
> 
> On 06/07/2023 14:00, Zhao Zhili wrote:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> Regression since 99dfdb45. intraRefreshPeriod access cc->gopLength,
>> which has been overwritten to NVENC_INFINITE_GOPLENGTH before.
>> Fixes #10445.
>> ---
>>  libavcodec/nvenc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
>> index 06579a502b..13fafcd246 100644
>> --- a/libavcodec/nvenc.c
>> +++ b/libavcodec/nvenc.c
>> @@ -1173,6 +1173,7 @@ static av_cold int nvenc_setup_h264_config(AVCodecContext *avctx)
>>          h264->enableIntraRefresh = 1;
>>          h264->intraRefreshPeriod = cc->gopLength;
>>          h264->intraRefreshCnt = cc->gopLength - 1;
>> +        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
>>  #ifdef NVENC_HAVE_SINGLE_SLICE_INTRA_REFRESH
>>          h264->singleSliceIntraRefresh = ctx->single_slice_intra_refresh;
>>  #endif
>> @@ -1297,6 +1298,7 @@ static av_cold int nvenc_setup_hevc_config(AVCodecContext *avctx)
>>          hevc->enableIntraRefresh = 1;
>>          hevc->intraRefreshPeriod = cc->gopLength;
>>          hevc->intraRefreshCnt = cc->gopLength - 1;
>> +        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
>>  #ifdef NVENC_HAVE_SINGLE_SLICE_INTRA_REFRESH
>>          hevc->singleSliceIntraRefresh = ctx->single_slice_intra_refresh;
>>  #endif
>> @@ -1415,6 +1417,7 @@ static av_cold int nvenc_setup_av1_config(AVCodecContext *avctx)
>>          av1->enableIntraRefresh = 1;
>>          av1->intraRefreshPeriod = cc->gopLength;
>>          av1->intraRefreshCnt = cc->gopLength - 1;
>> +        cc->gopLength = NVENC_INFINITE_GOPLENGTH;
>>            av1->idrPeriod = NVENC_INFINITE_GOPLENGTH;
>>      } else if (cc->gopLength > 0) {
>> @@ -1619,9 +1622,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>      if(ctx->single_slice_intra_refresh)
>>          ctx->intra_refresh = 1;
>>  -    if (ctx->intra_refresh)
>> -        ctx->encode_config.gopLength = NVENC_INFINITE_GOPLENGTH;
>> -
>>      nvenc_recalc_surfaces(avctx);
>>        nvenc_setup_rate_control(avctx);
> 
> Shouldn't it be enough to move this block down a bit, below nvenc_setup_codec_config?
> That way the codec specific configs can still access the value, and the logic of setting it to infinite for intra refresh mode doesn't have to be duplicated everywhere.

Then people lost the context and hard to get why it should be put after
nvenc_setup_codec_config(), and might move it before by accident.

Another benefit to set cc->gopLength at here is idrPeriod can be set
to gopLength unconditionally:

http://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/311694.html

To reduce code duplication, the codec specific intra_refresh configure
can be replaced by a macro, but it doesn’t looks good IMO.

> 
> Though I'm not sure which way I prefer. Having it in the intra_refresh specific block in each codec definitely also has its advantage of keeping that logic in one place.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-06 12:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-06 12:00 [FFmpeg-devel] [PATCH] avcodec/nvenc: fix intraRefreshPeriod setting Zhao Zhili
2023-07-06 10:23 ` Timo Rothenpieler
2023-07-06 12:03   ` "zhilizhao(赵志立)"

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git