libx264: Don't leave max_b_frames as -1 if the user didn't set it

Message ID 1326491519-80982-1-git-send-email-martin@martin.st
State Committed
Commit 57facb73ab940b1117c22ef9b70a6e5ec237112d
Headers show

Commit Message

Martin Storsjö Jan. 13, 2012, 9:51 p.m.
max_b_frames is initialized to -1 for libx264, to allow
distinguishing between an explicit user set 0 and a default not
touched 0 (see bb73cda2).

If max_b_frames is left as -1, this affects dts generation (where
expressions like max_b_frames != 0 are used), so make sure it is
left at the default 0 after the libx264 init function returns.

This avoids unnecessarily producing dts != pts when using
profile=baseline.
---
 libavcodec/libx264.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Anton Khirnov Jan. 13, 2012, 10:13 p.m. | #1
On Fri, 13 Jan 2012 23:51:59 +0200, Martin Storsjö <martin@martin.st> wrote:
> max_b_frames is initialized to -1 for libx264, to allow
> distinguishing between an explicit user set 0 and a default not
> touched 0 (see bb73cda2).
> 
> If max_b_frames is left as -1, this affects dts generation (where
> expressions like max_b_frames != 0 are used), so make sure it is
> left at the default 0 after the libx264 init function returns.
> 
> This avoids unnecessarily producing dts != pts when using
> profile=baseline.
> ---
>  libavcodec/libx264.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 9b34795..b3581f1 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -448,6 +448,9 @@ static av_cold int X264_init(AVCodecContext *avctx)
>      // update AVCodecContext with x264 parameters
>      avctx->has_b_frames = x4->params.i_bframe ?
>          x4->params.i_bframe_pyramid ? 2 : 1 : 0;
> +    if (avctx->max_b_frames < 0)
> +        avctx->max_b_frames = 0;
> +
>      avctx->bit_rate = x4->params.rc.i_bitrate*1000;
>  #if FF_API_X264_GLOBAL_OPTS
>      avctx->crf = x4->params.rc.f_rf_constant;
> -- 
> 1.7.3.1
> 

Ok.
Janne Grunau Jan. 13, 2012, 10:40 p.m. | #2
On 2012-01-13 23:51:59 +0200, Martin Storsjö wrote:
> max_b_frames is initialized to -1 for libx264, to allow
> distinguishing between an explicit user set 0 and a default not
> touched 0 (see bb73cda2).

it's generally initialized to -1
 
> If max_b_frames is left as -1, this affects dts generation (where
> expressions like max_b_frames != 0 are used), so make sure it is
> left at the default 0 after the libx264 init function returns.
> 
> This avoids unnecessarily producing dts != pts when using
> profile=baseline.
> ---
>  libavcodec/libx264.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 9b34795..b3581f1 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -448,6 +448,9 @@ static av_cold int X264_init(AVCodecContext *avctx)
>      // update AVCodecContext with x264 parameters
>      avctx->has_b_frames = x4->params.i_bframe ?
>          x4->params.i_bframe_pyramid ? 2 : 1 : 0;
> +    if (avctx->max_b_frames < 0)
> +        avctx->max_b_frames = 0;

so I'm wondering if it wouldn't make more sense to set it to 0
in avcodec_open2()

Janne
Martin Storsjö Jan. 13, 2012, 10:43 p.m. | #3
On Fri, 13 Jan 2012, Janne Grunau wrote:

> On 2012-01-13 23:51:59 +0200, Martin Storsjö wrote:
>> max_b_frames is initialized to -1 for libx264, to allow
>> distinguishing between an explicit user set 0 and a default not
>> touched 0 (see bb73cda2).
>
> it's generally initialized to -1

No, it's not. The -1 on the max_b_frames line in libavcodec/options.c is 
the minimum for the option, not the default (which is 0). The default is 
only overridden for libx264 to be -1, as set in bb73cda2.

// martin
Janne Grunau Jan. 14, 2012, 12:07 a.m. | #4
On 2012-01-14 00:43:45 +0200, Martin Storsjö wrote:
> On Fri, 13 Jan 2012, Janne Grunau wrote:
> 
> >On 2012-01-13 23:51:59 +0200, Martin Storsjö wrote:
> >>max_b_frames is initialized to -1 for libx264, to allow
> >>distinguishing between an explicit user set 0 and a default not
> >>touched 0 (see bb73cda2).
> >
> >it's generally initialized to -1
> 
> No, it's not. The -1 on the max_b_frames line in
> libavcodec/options.c is the minimum for the option, not the default
> (which is 0). The default is only overridden for libx264 to be -1,
> as set in bb73cda2.

yeah, missed that. patch ok then

Janne

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 9b34795..b3581f1 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -448,6 +448,9 @@  static av_cold int X264_init(AVCodecContext *avctx)
     // update AVCodecContext with x264 parameters
     avctx->has_b_frames = x4->params.i_bframe ?
         x4->params.i_bframe_pyramid ? 2 : 1 : 0;
+    if (avctx->max_b_frames < 0)
+        avctx->max_b_frames = 0;
+
     avctx->bit_rate = x4->params.rc.i_bitrate*1000;
 #if FF_API_X264_GLOBAL_OPTS
     avctx->crf = x4->params.rc.f_rf_constant;