diff mbox series

[FFmpeg-devel,v2,4/6] avcodec/libsvtav1: Fix CQP mode doesn't work as expection

Message ID 1631928405-26935-4-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2,1/6] avcodec/libsvtav1: Fix override setting of caps_internal | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Lance Wang Sept. 18, 2021, 1:26 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/libsvtav1.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jan Ekström Sept. 25, 2021, 12:06 p.m. UTC | #1 
On Sat, Sep 18, 2021 at 4:27 AM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---

Commit message should probably be something along the lines of

"""
avcodec/libsvtav1: properly enforce CQP mode when set in wrapper

SVT-AV1 seems to have switched their default from CQP to CRF in February,
so enforce the controlling option accordingly.
"""

>  libavcodec/libsvtav1.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index 0dc25ca..b029e01 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -208,6 +208,8 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>      if (param->rate_control_mode) {
>          param->max_qp_allowed       = avctx->qmax;
>          param->min_qp_allowed       = avctx->qmin;
> +    } else {
> +        param->enable_tpl_la = 0; /* CQP need turn off enable_tp_la */

As the default changing in the underlying library has now shown us, I
think (for now) it's better to move this next to
param->rate_control_mode earlier in the function, and then do
something like:

param->enable_tpl_la = !!param->rate_control_mode;

(I would have utilized param->enable_tpl_la = param->rate_control_mode
== SVTAV1_RC_MODE_CQP;` but alas SVT-AV1 does not have such
enums/defines that make such things more readable).

This way the parameter is set correctly no matter if the default is
switched over at SVT-AV1. In the future the wrapper can be reworked so
that by default SVT-AV1's own rate control defaults are utilized, and
then if either bit rate or cqp or something like that is set, we can
start enforcing that.

Jan
Lance Wang Sept. 25, 2021, 1:11 p.m. UTC | #2 
On Sat, Sep 25, 2021 at 03:06:23PM +0300, Jan Ekström wrote:
> On Sat, Sep 18, 2021 at 4:27 AM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> 
> Commit message should probably be something along the lines of
> 
> """
> avcodec/libsvtav1: properly enforce CQP mode when set in wrapper
> 
> SVT-AV1 seems to have switched their default from CQP to CRF in February,
> so enforce the controlling option accordingly.
> """

Sure, I'll update it by your suggestion.

> 
> >  libavcodec/libsvtav1.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> > index 0dc25ca..b029e01 100644
> > --- a/libavcodec/libsvtav1.c
> > +++ b/libavcodec/libsvtav1.c
> > @@ -208,6 +208,8 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
> >      if (param->rate_control_mode) {
> >          param->max_qp_allowed       = avctx->qmax;
> >          param->min_qp_allowed       = avctx->qmin;
> > +    } else {
> > +        param->enable_tpl_la = 0; /* CQP need turn off enable_tp_la */
> 
> As the default changing in the underlying library has now shown us, I
> think (for now) it's better to move this next to
> param->rate_control_mode earlier in the function, and then do
> something like:
> 
> param->enable_tpl_la = !!param->rate_control_mode;

Yes, It's better idea. I'll update by your suggestion.

> 
> (I would have utilized param->enable_tpl_la = param->rate_control_mode
> == SVTAV1_RC_MODE_CQP;` but alas SVT-AV1 does not have such
> enums/defines that make such things more readable).
> 
> This way the parameter is set correctly no matter if the default is
> switched over at SVT-AV1. In the future the wrapper can be reworked so
> that by default SVT-AV1's own rate control defaults are utilized, and
> then if either bit rate or cqp or something like that is set, we can
> start enforcing that.
> 
> Jan
> _______________________________________________
> 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 mbox series

Patch

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 0dc25ca..b029e01 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -208,6 +208,8 @@  static int config_enc_params(EbSvtAv1EncConfiguration *param,
     if (param->rate_control_mode) {
         param->max_qp_allowed       = avctx->qmax;
         param->min_qp_allowed       = avctx->qmin;
+    } else {
+        param->enable_tpl_la = 0; /* CQP need turn off enable_tp_la */
     }
 
     /* 2 = IDR, closed GOP, 1 = CRA, open GOP */