diff mbox series

[FFmpeg-devel,2/2] avcodec/libsvtav1: clean up unconditional override of avctx bit rate

Message ID 20220327180211.12038-2-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/libsvtav1: signal CPB properties through side data | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Jan Ekström March 27, 2022, 6:02 p.m. UTC 
This value is only documented to be utilized if rate control is
set to one of the bit rate based modes. These modes are not the
default and thus the wrapper itself configures a bit rate according
to the avctx if bit rate based encoding is requested, making the
relevant value already be set when relevant.
---
 libavcodec/libsvtav1.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Lance Wang March 29, 2022, 1:09 p.m. UTC | #1 
On Sun, Mar 27, 2022 at 09:02:11PM +0300, Jan Ekström wrote:
> This value is only documented to be utilized if rate control is
> set to one of the bit rate based modes. These modes are not the
> default and thus the wrapper itself configures a bit rate according
> to the avctx if bit rate based encoding is requested, making the
> relevant value already be set when relevant.
> ---
>  libavcodec/libsvtav1.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index d89fefdf4c..fe0d100beb 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -271,7 +271,6 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>          param->frame_rate_denominator = avctx->time_base.num * avctx->ticks_per_frame;
>      }
>  
> -    avctx->bit_rate                 = param->target_bit_rate;
>      if (avctx->bit_rate) {
>          param->max_qp_allowed       = avctx->qmax;
>          param->min_qp_allowed       = avctx->qmin;
> -- 
> 2.35.1

The patch set is LGTM

> 
> _______________________________________________
> 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".
James Almer March 30, 2022, 2:20 a.m. UTC | #2 
On 3/27/2022 3:02 PM, Jan Ekström wrote:
> This value is only documented to be utilized if rate control is
> set to one of the bit rate based modes. These modes are not the
> default and thus the wrapper itself configures a bit rate according
> to the avctx if bit rate based encoding is requested, making the
> relevant value already be set when relevant.

You can pass about anything with svtav1-params, which can change 
param->target_bit_rate. It's better to reflect that in the AVCodecContext.

> ---
>   libavcodec/libsvtav1.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index d89fefdf4c..fe0d100beb 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -271,7 +271,6 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>           param->frame_rate_denominator = avctx->time_base.num * avctx->ticks_per_frame;
>       }
>   
> -    avctx->bit_rate                 = param->target_bit_rate;
>       if (avctx->bit_rate) {
>           param->max_qp_allowed       = avctx->qmax;
>           param->min_qp_allowed       = avctx->qmin;
Jan Ekström March 30, 2022, 9:36 a.m. UTC | #3 
On Wed, Mar 30, 2022 at 5:21 AM James Almer <jamrial@gmail.com> wrote:
>
> On 3/27/2022 3:02 PM, Jan Ekström wrote:
> > This value is only documented to be utilized if rate control is
> > set to one of the bit rate based modes. These modes are not the
> > default and thus the wrapper itself configures a bit rate according
> > to the avctx if bit rate based encoding is requested, making the
> > relevant value already be set when relevant.
>
> You can pass about anything with svtav1-params, which can change
> param->target_bit_rate. It's better to reflect that in the AVCodecContext.
>

Yea, of course then you need to put it under if
(param->rate_control_mode > 0) since the bit rate value by default is
not unset but 2mbps, and thus you need to only apply the value IFF bit
rate based rate control is utilized.

Since that is what removing this override was fixing, you would get a
bit rate value of 2mbps even though that was not utilized nor set by
anything (other than being the default).

Jan
diff mbox series

Patch

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index d89fefdf4c..fe0d100beb 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -271,7 +271,6 @@  static int config_enc_params(EbSvtAv1EncConfiguration *param,
         param->frame_rate_denominator = avctx->time_base.num * avctx->ticks_per_frame;
     }
 
-    avctx->bit_rate                 = param->target_bit_rate;
     if (avctx->bit_rate) {
         param->max_qp_allowed       = avctx->qmax;
         param->min_qp_allowed       = avctx->qmin;