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

[Etienne Buira]

On Wed, Jun 22, 2011 at 08:35:37PM +0200, Erik Slagter wrote:
> >> -#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 ?
> 
> Personally I don't mind having a few lines of code extra if it means
> the purpose of the code is clear at a glance. But I assume the ffmpeg
> devels try to squeeze out as much lines as possible ;-)

The purpose was actually to have fewer lines, such as keeping OPT_STR,
changing it to call opt_str which would look like
static int opt_str(...)
{
    int ret = x264_param_parse(...)
    switch(ret) {
    case X264_PARAM_BAD_NAME: av_log(...); break;
    case X264_PARAM_BAD_VALUE: av_log(...); break;
    default: av_log(...); break;
    }
    return ret;
}

> >>     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.
> 
> The reason I separated this code out is that I thought a "null" value (at the
> actual option parsing, not the default settings) would signify an absent
> value (which is perfectly legal for some options). I now see that an empty
> string is used for that. This means indeed room for optimisation.

You didn't understand.
The goal is to have presets sets some values, and override them only if
they are specified. That's what the 'if (param&&' in OPT_STR does now.