[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".
From 9f9dcb3cceebb360468fea762b01780f65764a47 Mon Sep 17 00:00:00 2001
From: Josh Brewster
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.
This commit changes the way levels are converted to int from strings,
following an algoritm similar to that of x264 (currently defined in
common/base.c).
Signed-off-by: Josh Brewster
---
libavcodec/libx264.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
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 {
+level_id = av_strtod(x4->level, NULL);
+}
if (level_id <= 0)
av_log(avctx, AV_LOG_WARNING, "Failed to parse level\n");
--
2.26.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v2] libavcodec/libx264: fix reference frame computation based on level
> Hi,
>
> > From: ffmpeg-devel ffmpeg-devel-boun...@ffmpeg.org On Behalf Of
> > josh.brews...@protonmail.com
> > Sent: Friday, April 17, 2020 07:05
> > To: ffmpeg-devel@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@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.
From 98344809c22d574959eb4351a1cc03f431b4617c Mon Sep 17 00:00:00 2001
From: Josh Brewster
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
---
libavcodec/libx264.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index a08fe0ce76..a4caa5b5db 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; iparams.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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference frame computation based on level
> >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.
From af09a7c3d33db90092be3dea57ba449884003246 Mon Sep 17 00:00:00 2001
From: Josh Brewster
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
---
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; iparams.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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference frame computation based on level
> > > 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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference frame computation based on level
> > From: ffmpeg-devel ffmpeg-devel-boun...@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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
