[FFmpeg-devel] [PATCH] libavcodec/libx264.c: distinguish between x264 parameter errors

[Etienne Buira]

On Wed, Jun 22, 2011 at 08:10:47PM +0200, Erik Slagter wrote:
> Hi all,

Hi

> The current code in libx264.c (#define OPT_STR) does
> not distinguish between two types of errors returned
> by libx264: parameter not existing or parameter's value bad.
> 
> This ommitment has given me some headaches, because I
> didn't understand why my "profile=high" was "bad". After
> all it appeared that "profile" isn't a valid "x264opt" at
> all and needs to specified on the command line on it's own.
> 
> To save others this frustration, I'd like to have this
> trivial patch in, it makes ffmpeg throw different error
> messages for both errors. I had to change the #define into
> a proper function, but imho that's only for the better.

libx264 option passing is currently weird, you can look at a thread
that last from a few weeks ([RFC] Specifying KEYINT (-g) has no effect
in libx264).

> This is against current git.
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index cc5b983..2959ba1 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -198,14 +198,36 @@ static void check_default_settings(AVCodecContext *avctx)
>        }
>    }
> 
> -#define OPT_STR(opt, param)                                             \
> -    do {                                                                \
> -        if (param && x264_param_parse(&x4->params, opt, param) < 0) {   \
> -            av_log(avctx, AV_LOG_ERROR,                                 \
> -                   "bad value for '%s': '%s'\n", opt, param);           \
> -            return -1;                                                  \
> -        }                                                               \
> -    } while (0);                                                        \
> +static int opt_str(AVCodecContext *avctx, X264Context *x4, const char *opt, const char *param)
> +{
> +    switch(x264_param_parse(&x4->params, opt, param))
> +    {
> +        case(0):
> +        {
> +            break;
> +        }
> +
> +        case(X264_PARAM_BAD_NAME):
> +        {
> +            av_log(avctx, AV_LOG_ERROR, "no such option: \"%s\"\n", opt);
> +            return(0);
> +        }
> +
> +        case(X264_PARAM_BAD_VALUE):
> +        {
> +            av_log(avctx, AV_LOG_ERROR, "bad value: \"%s\" for option \"%s\"\n", param, opt);
> +            return(0);
> +        }
> +
> +        default:
> +        {
> +            av_log(avctx, AV_LOG_ERROR, "unknown parameter error, option: \"%s\", value: \"%s\"\n", param, opt);
> +            return(0);
> +        }
> +    }
> +
> +    return(1);
> +}

Why not a function which returns the x264_param_parse return value,
with an helper macro OPT_STR that will take care of returning -1 ?

>    static av_cold int X264_init(AVCodecContext *avctx)
>    {
> @@ -308,7 +330,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
>        x4->params.p_log_private        = avctx;
>        x4->params.i_log_level          = X264_LOG_DEBUG;
> 
> -    OPT_STR("weightp", x4->weightp);
> +    if(!opt_str(avctx, x4, "weightp", x4->weightp ? x4->weightp : "0"))
> +        return(-1);

The x4->weightp ? x4->weightp : "0" construct avoids the benefits of
OPT_STR, which is to overwrite the value only if it has really been
specified in the options.