Re: [FFmpeg-devel] [PATCH] avcodec/nvenc: fix intraRefreshPeriod setting

["\"zhilizhao(赵志立)\"" <quinkblack@foxmail.com>]

> 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".