| Message ID | tencent_8AAF6541531F284A53F17378D5D089010E05@qq.com |
|---|---|
| State | Accepted |
| Commit | 0d0c39832fe0ceb58797548d544621eb7bd0755c |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/nvenc: fix intraRefreshPeriod setting | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
> 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".
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);
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(-)