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

[Baptiste Coudurier]

Hi,

On 06/22/2011 11:10 AM, Erik Slagter wrote:
> Hi all,
> 
> 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.
> 
> 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;
> +        }

{ placement is not following ffmpeg coding style:
if () {

} else {

}

> +        case(X264_PARAM_BAD_NAME):
> +        {
> +            av_log(avctx, AV_LOG_ERROR, "no such option: \"%s\"\n", opt);
> +            return(0);

please remove () with return.

>   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);

Having a macro avoids this changes and return -1, so please keep the
macro, remove the switch. It should only be a matter of:

ret = x264_param_parse.
if (ret == BAD_NAME) else if (ret == BAD_VALUE)

[...]

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