[FFmpeg-devel] [RFC] Specifying KEYINT (-g) has no effect in libx264

[Baptiste Coudurier]

On 06/08/2011 01:47 AM, Stefano Sabatini wrote:
> On date Friday 2011-05-27 12:54:47 +0200, Etienne Buira encoded:
>> [...]
>> 
>> +    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)?

It is there only not to break applications.

> 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. 

I don't think so, but you can see in the sources :)

>> +    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.

Well, to be honest, I think breaking is the best option if we want to
avoid this mess.

>> +    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).

These options are for backward compatibility.

>> +        }
>> +        snprintf(param, 254, "%f", 1/fabs(f));
>                            ^^^
> sizeof(param)-1

This should be "sizeof(param)", man snprintf

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