[FFmpeg-devel] [RFC] Specifying KEYINT (-g) has no effect in libx264
Stefano Sabatini
stefano.sabatini-lala at poste.it
Wed Jun 8 10:47:16 CEST 2011
On date Friday 2011-05-27 12:54:47 +0200, Etienne Buira encoded:
> Hi Baptiste.
>
> On Thu, May 26, 2011 at 05:54:03PM -0700, Baptiste Coudurier wrote:
> > > + 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->slices);
> >
> > I believe it is possible to parse the options array and if AVOption type
> > is OPT_STRING, free the offset, that should work.
>
> You believe right :)
>
> > Same, where did this go ? It seems it went under #if 0 ?
> > Please make sure it works :)
>
> Sorry, I thought you were looking for a draft for comment.
> Put back at their original place, diff didn't see that for some of them.
>
> upa.
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index b0cca65..8490a9d 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,8 +44,58 @@ 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 *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 const AVOption options[];
> +
> static void X264_log(void *p, int level, const char *fmt, va_list args)
> {
> static const int level_map[] = {
> @@ -163,6 +214,7 @@ static int X264_frame(AVCodecContext *ctx, uint8_t *buf,
> static av_cold int X264_close(AVCodecContext *avctx)
> {
> X264Context *x4 = avctx->priv_data;
> + AVOption const * o;
>
> av_freep(&avctx->extradata);
> av_free(x4->sei);
> @@ -170,13 +222,9 @@ static av_cold int X264_close(AVCodecContext *avctx)
> if (x4->enc)
> x264_encoder_close(x4->enc);
>
> - av_free(x4->preset);
> - av_free(x4->tune);
> - av_free(x4->profile);
> - av_free(x4->level);
> - av_free(x4->stats);
> - av_free(x4->weightp);
> - av_free(x4->x264opts);
> + for (o = options; o->name; o++)
> + if (o->type == FF_OPT_TYPE_STRING)
> + av_free(*(char **) (((uint8_t*)x4)+o->offset));
This is not anymore necessary now that av_opt_free() is called.
> return 0;
> }
> @@ -215,6 +263,18 @@ static void check_default_settings(AVCodecContext *avctx)
> } \
> } while (0); \
>
> +static struct {
> + const char *name;
> + unsigned int partitionflag;
> +} x264partitions[] = {
> + {"i4x4", X264_ANALYSE_I4x4},
> + {"i8x8", X264_ANALYSE_I8x8},
> + {"p8x8", X264_ANALYSE_PSUB16x16},
> + {"p4x4", X264_ANALYSE_PSUB8x8},
> + {"b8x8", X264_ANALYSE_BSUB16x16},
> + {NULL},
> +};
As I suggested, you could put this in options:
{"partitions", "macroblock subpartition sizes to consider", OFFSET(partitions), FF_OPT_TYPE_FLAGS, {.dbl = 0 }, INT_MIN, INT_MAX, VE, "partitions"},
{"b8x8", NULL, 0, FF_OPT_TYPE_CONST, {.dbl = X264_ANALYSE_BSUB16x16 }, INT_MIN, INT_MAX, VE, "partitions"},
{"i4x4", NULL, 0, FF_OPT_TYPE_CONST, {.dbl = X264_ANALYSE_I4x4 }, INT_MIN, INT_MAX, VE, "partitions"},
{"i8x8", NULL, 0, FF_OPT_TYPE_CONST, {.dbl = X264_ANALYSE_I8x8 }, INT_MIN, INT_MAX, VE, "partitions"},
{"p4x4", NULL, 0, FF_OPT_TYPE_CONST, {.dbl = X264_ANALYSE_PSUB8x8 }, INT_MIN, INT_MAX, VE, "partitions"},
{"p8x8", NULL, 0, FF_OPT_TYPE_CONST, {.dbl = X264_ANALYSE_PSUB16x16 }, INT_MIN, INT_MAX, VE, "partitions"},
and while at it deprecate the corresponding option in AVCodecContext.
> +
> static av_cold int X264_init(AVCodecContext *avctx)
> {
> X264Context *x4 = avctx->priv_data;
> @@ -222,87 +282,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
> 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;
> -
> - x4->params.i_keyint_min = avctx->keyint_min;
> - if (x4->params.i_keyint_min > x4->params.i_keyint_max)
> - x4->params.i_keyint_min = x4->params.i_keyint_max;
> -
> - x4->params.i_scenecut_threshold = avctx->scenechange_threshold;
> -
> - x4->params.b_deblocking_filter = avctx->flags & CODEC_FLAG_LOOP_FILTER;
> - x4->params.i_deblocking_filter_alphac0 = avctx->deblockalpha;
> - x4->params.i_deblocking_filter_beta = avctx->deblockbeta;
> -
> - x4->params.rc.i_qp_min = avctx->qmin;
> - x4->params.rc.i_qp_max = avctx->qmax;
> - x4->params.rc.i_qp_step = avctx->max_qdiff;
> -
> - x4->params.rc.f_qcompress = avctx->qcompress; /* 0.0 => cbr, 1.0 => constant qp */
> - x4->params.rc.f_qblur = avctx->qblur; /* temporally blur quants */
> - x4->params.rc.f_complexity_blur = avctx->complexityblur;
> -
> - x4->params.i_frame_reference = avctx->refs;
> -
> - x4->params.analyse.inter = 0;
> - if (avctx->partitions) {
> - if (avctx->partitions & X264_PART_I4X4)
> - x4->params.analyse.inter |= X264_ANALYSE_I4x4;
> - if (avctx->partitions & X264_PART_I8X8)
> - x4->params.analyse.inter |= X264_ANALYSE_I8x8;
> - if (avctx->partitions & X264_PART_P8X8)
> - x4->params.analyse.inter |= X264_ANALYSE_PSUB16x16;
> - if (avctx->partitions & X264_PART_P4X4)
> - x4->params.analyse.inter |= X264_ANALYSE_PSUB8x8;
> - if (avctx->partitions & X264_PART_B8X8)
> - x4->params.analyse.inter |= X264_ANALYSE_BSUB16x16;
> - }
> -
> - x4->params.analyse.i_direct_mv_pred = avctx->directpred;
> -
> - x4->params.analyse.b_weighted_bipred = avctx->flags2 & CODEC_FLAG2_WPRED;
> -
> - if (avctx->me_method == ME_EPZS)
> - x4->params.analyse.i_me_method = X264_ME_DIA;
> - else if (avctx->me_method == ME_HEX)
> - x4->params.analyse.i_me_method = X264_ME_HEX;
> - else if (avctx->me_method == ME_UMH)
> - x4->params.analyse.i_me_method = X264_ME_UMH;
> - else if (avctx->me_method == ME_FULL)
> - x4->params.analyse.i_me_method = X264_ME_ESA;
> - else if (avctx->me_method == ME_TESA)
> - x4->params.analyse.i_me_method = X264_ME_TESA;
> - else x4->params.analyse.i_me_method = X264_ME_HEX;
> -
> - x4->params.rc.i_aq_mode = avctx->aq_mode;
> - x4->params.rc.f_aq_strength = avctx->aq_strength;
> - x4->params.rc.i_lookahead = avctx->rc_lookahead;
> -
> - x4->params.analyse.b_psy = avctx->flags2 & CODEC_FLAG2_PSY;
> - x4->params.analyse.f_psy_rd = avctx->psy_rd;
> - x4->params.analyse.f_psy_trellis = avctx->psy_trellis;
> -
> - x4->params.analyse.i_me_range = avctx->me_range;
> - x4->params.analyse.i_subpel_refine = avctx->me_subpel_quality;
> -
> - x4->params.analyse.b_mixed_references = avctx->flags2 & CODEC_FLAG2_MIXED_REFS;
> - x4->params.analyse.b_chroma_me = avctx->me_cmp & FF_CMP_CHROMA;
> - x4->params.analyse.b_transform_8x8 = avctx->flags2 & CODEC_FLAG2_8X8DCT;
> - x4->params.analyse.b_fast_pskip = avctx->flags2 & CODEC_FLAG2_FASTPSKIP;
>
> - x4->params.analyse.i_trellis = avctx->trellis;
> - x4->params.analyse.i_noise_reduction = avctx->noise_reduction;
> -
> - x4->params.rc.b_mb_tree = !!(avctx->flags2 & CODEC_FLAG2_MBTREE);
> - x4->params.rc.f_ip_factor = 1 / fabs(avctx->i_quant_factor);
> - x4->params.rc.f_pb_factor = avctx->b_quant_factor;
> - x4->params.analyse.i_chroma_qp_offset = avctx->chromaoffset;
> + /* FIXME: grep for CODEC_FLAG */
I hate FIXMEs (and they are doomed to stay unfixed forever), please
explain what this means.
> if (!x4->preset)
> check_default_settings(avctx);
> @@ -317,7 +298,108 @@ static av_cold int X264_init(AVCodecContext *avctx)
> x4->params.i_log_level = X264_LOG_DEBUG;
>
> OPT_STR("weightp", x4->weightp);
> -
> + OPT_STR("keyint", x4->keyint_max);
> + OPT_STR("bframes", x4->bframes);
> + if (x4->cabac) {
> + if (!strcmp(x4->cabac, "ac") || !strcmp(x4->cabac, "1")) {
> + OPT_STR("cabac", "1");
> + } else {
> + OPT_STR("cabac", "0");
> + }
> + }
Is this mapping required?
> + OPT_STR("b-adapt", x4->badapt);
> + OPT_STR("b-bias", x4->bbias);
> + x4->params.i_bframe_pyramid = avctx->flags2 & CODEC_FLAG2_BPYRAMID ? X264_B_PYRAMID_NORMAL : X264_B_PYRAMID_NONE;
> + OPT_STR("keyint-min", x4->keyintmin);
> + OPT_STR("scenecut", x4->scenecut);
> + x4->params.b_deblocking_filter = avctx->flags & CODEC_FLAG_LOOP_FILTER;
> + OPT_STR("deblock", x4->deblockalpha);
> + if (x4->deblockbeta) {
> + if (sscanf(x4->deblockbeta, "%d", &x4->params.i_deblocking_filter_beta) != 1) {
> + av_log(avctx, AV_LOG_ERROR, "Bad value for deblockbeta (should be an int)\n");
> + return -1;
AVERROR(EINVAL)
> + }
> + }
Can someone explain why some options can be set with OPT_STR and others
need to be directly set on x4->params?
> + OPT_STR("qpmin", x4->qpmin);
> + OPT_STR("qpmax", x4->qpmax);
> + OPT_STR("qpstep", x4->qpstep);
> + OPT_STR("qcomp", x4->qcomp); /* 0.0 => cbr, 1.0 => constant qp */
> + OPT_STR("qblur", x4->qblur); /* temporally blur quants */
> + OPT_STR("cplxblur", x4->cplxblur);
> + OPT_STR("ref", x4->ref);
> + if (x4->partitions) {
> + const char *p = x4->partitions;
> + while (*p) {
> + int i;
> + char sign = *(p++);
> + for (i=0; x264partitions[i].name && !av_strstart(p, x264partitions[i].name, &p); i++);
> + if (!x264partitions[i].name) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to parse partitions (unknown partition name)\n");
> + return -1;
> + }
> + switch(sign) {
> + case '+': x4->params.analyse.inter |= x264partitions[i].partitionflag; break;
> + case '-': x4->params.analyse.inter &= ~x264partitions[i].partitionflag; break;
> + default: av_log(avctx, AV_LOG_ERROR, "Unable to parse partitions (sign not found)\n");
> + return -1;
> + }
> + if (*p == ',') p++;
> + }
> + }
Ditto.
> + if (x4->directpred) {
> + if (isdigit(x4->directpred[0])) {
> + int i, v = atoi(x4->directpred);
> + for (i=0 ; x264_direct_pred_names[i] && i<v ; i++);
> + if (i==v) {
> + OPT_STR("direct-pred", x264_direct_pred_names[i]);
> + } else {
> + av_log(avctx, AV_LOG_ERROR, "Wrong value for directpred\n");
> + return -1;
> + }
> + } else
> + OPT_STR("direct-pred", x4->directpred);
> + }
That's a weird way of parsing code. Isn't this directly handled by
x264_param_parse() (or in other way: why all the applications have to
parse this again and again)?
BTW, can someone explain what happen when
x264_param_parse(&x4->params, opt, NULL)
is called with a NULL argument? I suppose the default value is set.
> + x4->params.analyse.b_weighted_bipred = avctx->flags2 & CODEC_FLAG2_WPRED;
> + if (x4->me_method) {
> + if (!strcmp(x4->me_method, "epzs")) {
> + OPT_STR("me", "dia");
> + } else if (!strcmp(x4->me_method, "full")) {
> + OPT_STR("me", "esa");
> + } else
> + OPT_STR("me", x4->me_method);
> + }
Same here, I'd like to avoid this mapping, unless the new mapping will
not break previous commandlines/preset, even in this case I'd like to
put this under and #if LIBAVCODEC_VERSION < NEXT.
> + OPT_STR("aq-mode", x4->aqmode);
> + OPT_STR("aq-strength", x4->aqstrength);
> + OPT_STR("rc-lookahead", x4->rc_lookahead);
> + x4->params.analyse.b_psy = avctx->flags2 & CODEC_FLAG2_PSY;
> + OPT_STR("psy-rd", x4->psy_rd);
> + if (x4->psy_trellis) {
> + if (sscanf(x4->psy_trellis, "%f", &x4->params.analyse.f_psy_trellis) != 1) {
> + av_log(avctx, AV_LOG_ERROR, "Bad value for psy_trellis (should be a float)\n");
> + return -1;
AVERROR(EINVAL).
> + }
> + }
Again, why can't we set this with OPT_STR? Also what prevents to use
AVOptions for parsing the value, and directly set it in
&x4->params.analyse.f_psy_trellis?
(You would need to define the option with AV_OPT_TYPE_FLOAT).
> + OPT_STR("me-range", x4->me_range);
> + OPT_STR("subq", x4->subq);
> + x4->params.analyse.b_mixed_references = avctx->flags2 & CODEC_FLAG2_MIXED_REFS;
> + OPT_STR("chroma-me", x4->chroma_me);
> + x4->params.analyse.b_transform_8x8 = avctx->flags2 & CODEC_FLAG2_8X8DCT;
> + x4->params.analyse.b_fast_pskip = avctx->flags2 & CODEC_FLAG2_FASTPSKIP;
> + OPT_STR("trellis", x4->trellis);
> + OPT_STR("nr", x4->nr);
> + x4->params.rc.b_mb_tree = !!(avctx->flags2 & CODEC_FLAG2_MBTREE);
> + if (x4->ip_factor) {
> + float f;
> + char param[255];
> + if (sscanf(x4->ip_factor, "%f", &f) != 1) {
> + av_log(avctx, AV_LOG_ERROR, "Invalid i_qfactor\n");
Best practices recommend to say which is the failing value for a
better feedback.
> + return -1;
AVERROR(EINVAL)
> + }
> + snprintf(param, 254, "%f", 1/fabs(f));
^^^
sizeof(param)-1
--
FFmpeg = Frenzy and Fostering Miracolous Philosophical Entertaining God
More information about the ffmpeg-devel
mailing list