[FFmpeg-devel] [RFC] Specifying KEYINT (-g) has no effect in libx264

Baptiste Coudurier baptiste.coudurier at gmail.com
Thu May 26 01:57:48 CEST 2011


Hi Etienne,

First thanks a lot for the patch.

On 05/25/2011 04:45 PM, Etienne Buira wrote:
> On Mon, May 23, 2011 at 01:32:15AM -0700, Baptiste Coudurier wrote:
>> On 5/22/11 8:33 PM, Michael Niedermayer wrote:
>>> On Sun, May 22, 2011 at 10:40:07PM +0200, Stefano Sabatini wrote:
>>>> Hi,
>>>>
>>>> see issue #157.
>>>>
>>>> The problem was possibly introduced in:
>>>>
>>>> commit 0140d3f0921e5cbb6ea8706acb0307f7ff57a133
>>>> Author: Baptiste Coudurier <baptiste.coudurier at gmail.com>
>>>> Date:   Sat Apr 16 16:50:50 2011 -0700
>>>>
>>>>     In libx264 wrapper, add -preset and -tune options
>>>>
>>>> and depends on the fact that x264_param_default_preset() which is
>>>> called in x264_init(), calls x264_param_default() which defaults the
>>>> already set options.
>>>>
>>>> Indeed order of options matters, with the current mechanism options
>>>> are set in the relevant contexts, and globally processed in the object
>>>> initialization, so the order specified on the commandline is basically
>>>> ignored.
>>>>
>>>> In the case of libx264 we may process profile/preset *before* the
>>>> other options set in the context, so that specific settings will
>>>> override profile/preset information.
>>
>> No, order doesn't matter, as long as a preset is specified, it will
>> override other options.
>>
>>>> This has the problem that most of the times user-set options are
>>>> changed to medium profile or libx264 will complain&exit, see:
>>>>
>>>> thus making fine-tuned user options pointless (since they are changed
>>>> to medium preset and/or will be rejected by libx264).
>>
>> No, you can still specify fine-tuned options without a preset.
>>
>>> I see 3 obvious solutions
>>>
>>> A. is mplayers to just use -x264opts which ive added
>>>
>>> B. keep track of which options have been set by the user and which are
>>>    default.
>>
>> 'B' is how it should be done. It's trivial, just remap "g" in the
>> libx264.c the way weightp was mapped.
> 
> Hi all.
> 
> Patch attached.
> 
> But now, I think it would be better to keep track of what options have
> been set at libavutil level. I don't know if ffmpeg would benefit a lot
> from that, but certainly libavutil.
> 
> 
> ffmpeg_move_relevant_options_to_libx264.diff
> 
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index d9bac17..efc8b46 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include "libavutil/opt.h"
> +#include "libavutil/avstring.h"
>  #include "avcodec.h"
>  #include <x264.h>
>  #include <math.h>
> @@ -43,6 +44,55 @@ typedef struct X264Context {
>      char *stats;
>      char *weightp;
>      char *x264opts;
> +    char *keyint_max;
> +    char *bframes;
> +    char *cabac;
> +    char *badapt;
> +    char *bbias;
> +    char *keyintmin;
> +    char *scenecut;
> +    char *deblockalpha;
> +    char *deblockbeta;
> +    char *qpmin;
> +    char *qpmax;
> +    char *qpstep;
> +    char *qcomp;
> +    char *qblur;
> +    char *cplxblur;
> +    char *ref;
> +    char *partitions;
> +    char *directpred;
> +    char *me_method;
> +    char *aqmode;
> +    char *aqstrength;
> +    char *rc_lookahead;
> +    char *psy_rd;
> +    char *psy_trellis;
> +    char *me_range;
> +    char *subq;
> +    char *chroma_me;
> +    char *trellis;
> +    char *nr;
> +    char *ip_factor;
> +    char *pb_factor;
> +    char *chromaoffset;
> +    char *pass;
> +    char *slices;
> +    char *flags, *flags2;
> +    char *psnr;
> +    char *b_pyramid;
> +    char *wpred;
> +    char *psy;
> +    char *mixed_refs;
> +    char *dct8x8;
> +    char *fast_pskip;
> +    char *mb_tree;
> +    char *intra_refresh;
> +    char *ssim;
> +    char *aud;
> +    char *interlaced;
> +    char *opengop;
> +    char *repeatheaders;
>  } X264Context;
>  
>  static void X264_log(void *p, int level, const char *fmt, va_list args)
> @@ -170,6 +220,57 @@ static av_cold int X264_close(AVCodecContext *avctx)
>      av_free(x4->level);
>      av_free(x4->stats);
>      av_free(x4->weightp);
> +    av_free(x4->x264opts);
> +    av_free(x4->keyint_max);
> +    av_free(x4->bframes);
> +    av_free(x4->cabac);
> +    av_free(x4->badapt);
> +    av_free(x4->bbias);
> +    av_free(x4->keyintmin);
> +    av_free(x4->scenecut);
> +    av_free(x4->deblockalpha);
> +    av_free(x4->deblockbeta);
> +    av_free(x4->qpmin);
> +    av_free(x4->qpmax);
> +    av_free(x4->qpstep);
> +    av_free(x4->qcomp);
> +    av_free(x4->qblur);
> +    av_free(x4->cplxblur);
> +    av_free(x4->ref);
> +    av_free(x4->partitions);
> +    av_free(x4->directpred);
> +    av_free(x4->me_method);
> +    av_free(x4->aqmode);
> +    av_free(x4->aqstrength);
> +    av_free(x4->rc_lookahead);
> +    av_free(x4->psy_rd);
> +    av_free(x4->psy_trellis);
> +    av_free(x4->me_range);
> +    av_free(x4->subq);
> +    av_free(x4->chroma_me);
> +    av_free(x4->trellis);
> +    av_free(x4->nr);
> +    av_free(x4->ip_factor);
> +    av_free(x4->pb_factor);
> +    av_free(x4->chromaoffset);
> +    av_free(x4->pass);
> +    av_free(x4->slices);
> +    av_free(x4->flags);
> +    av_free(x4->flags2);
> +    av_free(x4->psnr);
> +    av_free(x4->b_pyramid);
> +    av_free(x4->wpred);
> +    av_free(x4->psy);
> +    av_free(x4->mixed_refs);
> +    av_free(x4->dct8x8);
> +    av_free(x4->fast_pskip);
> +    av_free(x4->mb_tree);
> +    av_free(x4->intra_refresh);
> +    av_free(x4->ssim);
> +    av_free(x4->aud);
> +    av_free(x4->interlaced);
> +    av_free(x4->opengop);
> +    av_free(x4->repeatheaders);
>  
>      return 0;
>  }
> @@ -208,95 +309,137 @@ static void check_default_settings(AVCodecContext *avctx)
>          }                                                               \
>      } while (0);                                                        \
>  
> -static av_cold int X264_init(AVCodecContext *avctx)
> -{
> -    X264Context *x4 = avctx->priv_data;
> -
> -    x4->sei_size = 0;
> -    x264_param_default(&x4->params);
> -
> -    x4->params.i_keyint_max         = avctx->gop_size;
> -
> -    x4->params.i_bframe          = avctx->max_b_frames;
> -    x4->params.b_cabac           = avctx->coder_type == FF_CODER_TYPE_AC;
> -    x4->params.i_bframe_adaptive = avctx->b_frame_strategy;
> -    x4->params.i_bframe_bias     = avctx->bframebias;
> -    x4->params.i_bframe_pyramid  = avctx->flags2 & CODEC_FLAG2_BPYRAMID ? X264_B_PYRAMID_NORMAL : X264_B_PYRAMID_NONE;
> -    avctx->has_b_frames          = avctx->flags2 & CODEC_FLAG2_BPYRAMID ? 2 : !!avctx->max_b_frames;
> +#define OPT_DBLSTR(opt, param1, param2, err)                                \
> +    do {                                                                    \
> +        if (param1) {                                                       \
> +            char buf[255];                                                  \
> +            if (snprintf(buf, 254, "%s%s%s", param1,                        \
> +                         param2 ? ":" : "", param2 ? param2 : "") > 254) {  \
> +                av_log(avctx, AV_LOG_ERROR,                                 \
> +                       "String too long when dealing with %s param\n", opt);\
> +                return -1;                                                  \
> +            }                                                               \
> +            OPT_STR(opt, buf);                                              \
> +        } else if (param2) {                                                \
> +            av_log(avctx, AV_LOG_ERROR, "%s\n", err);                       \
> +            return -1;                                                      \
> +        }                                                                   \
> +    } while(0);                                                             \
> +
> +#define OPT_FLAG(opt, override, flarr, flnm, flagstr)                       \
> +    do {                                                                    \
> +        char *optdstr; int ret;                                             \
> +        if (override) {                                                     \
> +            OPT_STR(opt, override);                                         \
> +        } else if ((ret=flagset(flnm, flagstr, flarr, &optdstr)) > 0) {     \
> +            OPT_STR(opt, optdstr);                                          \
> +        } else if (ret < 0) {                                               \
> +            av_log(avctx, AV_LOG_ERROR, "Error when parsing flags\n");      \
> +            return -1;                                                      \
> +        }                                                                   \
> +    } while (0);                                                            \

Humm, can you make a patch without OPT_DBLSTR and without OPT_FLAG ?
Mention which options are left over. We'll see how it can be done.

> [...]
>
> +static struct {
> +    const char *ffname;
> +    const char *x264name;
> +} x264mes[] = {
> +    {"epzs", "dia"},
> +    {"hex", "hex"},
> +    {"umh", "umh"},
> +    {"full", "esa"},
> +    {"tesa", "tesa"},
> +    {NULL},
> +};

Do you need a struct for me ? I think you can pass the string directly
to OPT_STR.

> [...]
>
> +    {"pass1", "1", "0"},
> +    {"pass2", "1", "0"},

These 2 do not need remapping

> +    {"psnr", "1", "0"},
> +    {"ildct", "1", "0"},

Nor these ones.

> +    {"global_header", "0", "1"},

Nor this one.

> [...]
>
> +    {"ssim", "1", "0"},

This one does not need remapping.

> +    {"aud", "1", "0"},

Nor this one.

> [...]
>
> +    /* Defaults from ffmpeg before OPT_x move */
> +    x4->params.b_open_gop = 1;

You need to apply presets etc... before setting individual options.
see x264.h for instructions about order.

> [...]
>  
> +    /* Should be cleaned out */
> +    avctx->has_b_frames          = avctx->flags2 & CODEC_FLAG2_BPYRAMID ? 2 : !!avctx->max_b_frames;

This needs to be done at the end of init.

[...]

-- 
Baptiste COUDURIER
Key fingerprint          8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                           http://www.ffmpeg.org


More information about the ffmpeg-devel mailing list