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

[Erik Slagter]

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

Yeah, I know. And that's only one case. I am now focussing on getting
interlacing actually working with libx264, which means not only tff/bff
needs to be set, but also nal-hrd needs to be enabled and of course
_working_ from ffmpeg (currently not: "VBV parameters cannot be changed
when NAL HRD is in use")

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

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5110 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110622/8ecf9dd1/attachment.p7s>