[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