| Message ID | tED_QT-aSN-J5hZNeJf_TJHxJ0nXgn5Khb6sV9uB0npYv7Uy4byXSkImdKerkqjAtJ_qy9Bqc8fjo954t8oIU82t1MdoauuyKuYCDqzSkRw=@protonmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v3] libavcodec/libx264: fix reference frame computation based on level | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
> > > I only made sure that the level was positive because its initial > > > value was -1. > > > > > > > else if (x4->params.i_level_idc >= 0) { > > > > Let me know if I need to reject 0 too. It seemed like premature optimization > > > > as the level simply wouldn't be present in x264_levels. > > > > I'd say yes, level_idc = 0 is possible but invalid by PARSE_X264_OPT(), which seems > > make no sense to calculate refs from x264_levels[] table. > > > > - Linjie > > Changed to > 0, thanks. Hi, is there anything else I need to do to have this merged?
On 26/04/2020 12:46, Josh Brewster wrote: >>>> I only made sure that the level was positive because its initial >>>> value was -1. >>>> >>>>> else if (x4->params.i_level_idc >= 0) { >>>>> Let me know if I need to reject 0 too. It seemed like premature optimization >>>>> as the level simply wouldn't be present in x264_levels. >>> >>> I'd say yes, level_idc = 0 is possible but invalid by PARSE_X264_OPT(), which seems >>> make no sense to calculate refs from x264_levels[] table. >>> >>> - Linjie >> >> Changed to > 0, thanks. > > Hi, is there anything else I need to do to have this merged? From a precursory look at what x264 and we're doing here your patch is correct. It doesn't break from a quick test, and looks OK to me. Would rather someone else has a look at it too but I will again in a couple days if no one does.
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Josh de Kock > Sent: Tuesday, April 28, 2020 23:47 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference > frame computation based on level > > On 26/04/2020 12:46, Josh Brewster wrote: > >>>> I only made sure that the level was positive because its initial > >>>> value was -1. > >>>> > >>>>> else if (x4->params.i_level_idc >= 0) { > >>>>> Let me know if I need to reject 0 too. It seemed like premature > optimization > >>>>> as the level simply wouldn't be present in x264_levels. > >>> > >>> I'd say yes, level_idc = 0 is possible but invalid by PARSE_X264_OPT(), > which seems > >>> make no sense to calculate refs from x264_levels[] table. > >>> > >>> - Linjie > >> > >> Changed to > 0, thanks. > > > > Hi, is there anything else I need to do to have this merged? > > From a precursory look at what x264 and we're doing here your patch is > correct. It doesn't break from a quick test, and looks OK to me. Would > rather someone else has a look at it too but I will again in a couple > days if no one does. > Should be ok IMHO, thx. - Linjie
> > From: ffmpeg-devel ffmpeg-devel-bounces@ffmpeg.org On Behalf Of > > Josh de Kock > > Sent: Tuesday, April 28, 2020 23:47 > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference > > frame computation based on level > > On 26/04/2020 12:46, Josh Brewster wrote: > > > > > Hi, is there anything else I need to do to have this merged? > > > > From a precursory look at what x264 and we're doing here your patch is > > correct. It doesn't break from a quick test, and looks OK to me. Would > > rather someone else has a look at it too but I will again in a couple > > days if no one does. > > Should be ok IMHO, thx. > > - Linjie Thanks for the feedback, I'll wait for it to be merged then.
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Josh Brewster > Sent: Tuesday, May 5, 2020 23:52 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference > frame computation based on level > > > > From: ffmpeg-devel ffmpeg-devel-bounces@ffmpeg.org On Behalf Of > > > Josh de Kock > > > Sent: Tuesday, April 28, 2020 23:47 > > > To: ffmpeg-devel@ffmpeg.org > > > Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference > > > frame computation based on level > > > On 26/04/2020 12:46, Josh Brewster wrote: > > > > > > > Hi, is there anything else I need to do to have this merged? > > > > > > From a precursory look at what x264 and we're doing here your patch is > > > correct. It doesn't break from a quick test, and looks OK to me. Would > > > rather someone else has a look at it too but I will again in a couple > > > days if no one does. > > > > Should be ok IMHO, thx. > > > > - Linjie > > Thanks for the feedback, I'll wait for it to be merged then. > FYI, already merged several days ago with the help of Josh in: https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/79f001675a2bae16e243f30a3e7de9da6aeb3c2d
On 05/05/2020 17:02, Fu, Linjie wrote: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Josh Brewster >> Sent: Tuesday, May 5, 2020 23:52 >> To: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference >> frame computation based on level >> >>>> From: ffmpeg-devel ffmpeg-devel-bounces@ffmpeg.org On Behalf Of >>>> Josh de Kock >>>> Sent: Tuesday, April 28, 2020 23:47 >>>> To: ffmpeg-devel@ffmpeg.org >>>> Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference >>>> frame computation based on level >>>> On 26/04/2020 12:46, Josh Brewster wrote: >>>> >>>>> Hi, is there anything else I need to do to have this merged? >>>> >>>> From a precursory look at what x264 and we're doing here your patch is >>>> correct. It doesn't break from a quick test, and looks OK to me. Would >>>> rather someone else has a look at it too but I will again in a couple >>>> days if no one does. >>> >>> Should be ok IMHO, thx. >>> >>> - Linjie >> >> Thanks for the feedback, I'll wait for it to be merged then. >> > > FYI, already merged several days ago with the help of Josh in: > https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/79f001675a2bae16e243f30a3e7de9da6aeb3c2d > Ah, it seems my 'Patch applied' email never came though. Yes, I merged this.
From af09a7c3d33db90092be3dea57ba449884003246 Mon Sep 17 00:00:00 2001 From: Josh Brewster <josh.brewster@protonmail.com> Date: Thu, 16 Apr 2020 22:50:29 +0200 Subject: [PATCH] libavcodec/libx264: fix reference frame computation based on level The current implementation allows passing levels to libavcodec as integers (such as "31" instead of "3.1"). However, in this case, the maximum reference frame value per level was ignored because libavcodec converted the string to 310 instead of 31. Since libx264 has correctly parsed the level to int (x4->params.i_level_idc), we should rely on this value instead of attempting to parse the level string on our own. Signed-off-by: Josh Brewster <josh.brewster@protonmail.com> --- libavcodec/libx264.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index a08fe0ce76..c6cce9ff80 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -692,25 +692,13 @@ FF_ENABLE_DEPRECATION_WARNINGS x4->params.rc.f_qcompress = avctx->qcompress; /* 0.0 => cbr, 1.0 => constant qp */ if (avctx->refs >= 0) x4->params.i_frame_reference = avctx->refs; - else if (x4->level) { + else if (x4->params.i_level_idc > 0) { int i; int mbn = AV_CEIL_RSHIFT(avctx->width, 4) * AV_CEIL_RSHIFT(avctx->height, 4); - int level_id = -1; - char *tail; int scale = X264_BUILD < 129 ? 384 : 1; - if (!strcmp(x4->level, "1b")) { - level_id = 9; - } else if (strlen(x4->level) <= 3){ - level_id = av_strtod(x4->level, &tail) * 10 + 0.5; - if (*tail) - level_id = -1; - } - if (level_id <= 0) - av_log(avctx, AV_LOG_WARNING, "Failed to parse level\n"); - for (i = 0; i<x264_levels[i].level_idc; i++) - if (x264_levels[i].level_idc == level_id) + if (x264_levels[i].level_idc == x4->params.i_level_idc) x4->params.i_frame_reference = av_clip(x264_levels[i].dpb / mbn / scale, 1, x4->params.i_frame_reference); } -- 2.26.0