[FFmpeg-devel] [PATCH v2] libavcodec/libx264: fix reference frame computation based on level

[Josh Brewster]

> Hi,
>
> > From: ffmpeg-devel ffmpeg-devel-bounces at ffmpeg.org On Behalf Of
> > josh.brewster at protonmail.com
> > Sent: Friday, April 17, 2020 07:05
> > To: ffmpeg-devel at ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH] libavcodec/libx264: fix reference frame
> > computation based on level
> > Hell, can someone please review this patch? It fixes a wrong reference frame
> > computation problem when using parameters such as "-level 31" instead of
> > "-level 3.1".
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index a08fe0ce76..1149f2d668 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -701,11 +701,14 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >
> >          if (!strcmp(x4->level, "1b")) {
> >              level_id = 9;
> >
> >
> > -          } else if (strlen(x4->level) <= 3){
> >
> >
> >
> > -          } else if (av_strtod(x4->level, NULL) < 7){
> >                level_id = av_strtod(x4->level, &tail) * 10 + 0.5;
> >                if (*tail)
> >                    level_id = -1;
> >            }
> >
> >
> > -          else {
> >
> >
>
> Wrong coding style for "else", should be "} else {"
>
> > -              level_id = av_strtod(x4->level, NULL);
> >
> >
> > -          }
> >            if (level_id <= 0)
> >                av_log(avctx, AV_LOG_WARNING, "Failed to parse level\\n");
> >
> >
>
> This part of code of parsing level_id seems redundant, since PARSE_X264_OPT("level", level) has
> been called and did the same thing inside libx264, which converts x4->level to x4->params.i_level_idc
> correctly (equals 31), regardless of "-level 3.1 or level 31".
>
> Hence it would be better to use x4->params.i_level_idc directly.
>
> -   Linjie
>
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

Hi Linjie, thanks for the feedback. I have changed the patch to use the right parameter. 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libavcodec-libx264-fix-reference-frame-computation-b.patch
Type: text/x-patch
Size: 2148 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200417/7e08cd01/attachment.bin>