[FFmpeg-devel] [PATCH 1/9] lavu/opt: introduce av_opt_is_set_to_default()

[Lukasz Marek]

On 11.11.2014 15:45, Stefano Sabatini wrote:
>> +        if (o->default_val.str)
>> +            av_parse_video_rate(&q, o->default_val.str);
>
> you should check the return value here as well for
> consistency. Alternatively you can place an assert().

I dont agree. Assert cannot be placed. Return value is not checked is 
set_defaults so errors are ignored. It can just leave invalid value. 
Assert here would terminate an application. In general I think it is not 
regular case - i doub't there is any option with invalid default -  but 
all the best we can do is to compare set value with the default, not 
validate the default

>> +        return !av_cmp_q(*(AVRational*)dst, q);
>> +    case AV_OPT_TYPE_COLOR: {
>> +        uint8_t color[4] = {0, 0, 0, 0};
>> +        if (o->default_val.str)
>> +            av_parse_color(color, o->default_val.str, -1, NULL);
>
> ditto

The same as above

>
>> +        return ((uint8_t *)dst)[0] == color[0] && ((uint8_t *)dst)[1] == color[1] &&
>> +               ((uint8_t *)dst)[2] == color[2] && ((uint8_t *)dst)[3] == color[3];
>
> memcmp or maybe a cast to uint32_t, but that's subjective.

Casting would be strict aliasing violation I guess. changed to memcmp, 
it is more clear.

>> +    }
>> +    default:
>> +        av_log(NULL, AV_LOG_WARNING, "Not supported option type: %d\n", o->type);
>
> nit: also state name and value, so that's easier to debug

Added name, value has nothing to do with it. changed NULL to obj.

>
>> +        break;
>> +    }
>
>> +    return AVERROR_PATCHWELCOME;
>
> unreachable code, probably an assert is better here

It is reached from default section (there is a break, not a return).


>> +    printf("\nTesting av_opt_is_set_to_default()\n");
>> +    {
>> +        TestContext test_ctx = { 0 };
>> +        const AVOption *o = NULL;
>> +        test_ctx.class = &test_class;
>> +
>> +        av_log_set_level(AV_LOG_QUIET);
>> +
>> +        while (o = av_opt_next(&test_ctx, o)) {
>
>> +            if (av_opt_is_set_to_default_by_name(&test_ctx, o->name, 0) <= 0)
>> +                printf("option not detected as set to default: '%s'\n", o->name);
>> +            else
>> +                printf("option     detected as set to default: '%s'\n", o->name);
>
> probably less verbose for a test:
>
>     ret = av_opt_is_set_to_default_by_name(&test_ctx, o->name, 0);
>     printf("name:%s default:%s error:%s", o->name, !!ret, av_err2str(ret));

Changed

Updated patch attached
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavu-opt-introduce-av_opt_is_set_to_default.patch
Type: text/x-patch
Size: 8226 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141111/a0ee4bcc/attachment.bin>