[FFmpeg-devel] [PATCH] avfilter: add metadata filters

[Tobias Rapp]

On 10.02.2016 10:01, Paul B Mahol wrote:
> On 2/10/16, Tobias Rapp <t.rapp at noa-archive.com> wrote:
>> On 06.02.2016 23:30, Paul B Mahol wrote:
>>> On 2/6/16, Paul B Mahol <onemda at gmail.com> wrote:
>>>> Hi,
>>>>
>>>> patch attached.
>>>>
>>>
>>> Improved version attached.
>>>
>>> [...]
>>> +
>>> +static int string(const char *value1, const char *value2, size_t length)
>>> +{
>>> +    return !strncmp(value1, value2, length);
>>> +}
>>
>> If I understand correctly this function is used to compare if the start
>> of value2 matches value1. Maybe this function should be called
>> "starts_with" (also in the "function" option enum)?
>>
>>> +
>>> +static int equal(const char *value1, const char *value2, size_t length)
>>> +{
>>> +    float f1, f2;
>>> +
>>> +    if (sscanf(value1, "%f", &f1) + sscanf(value2, "%f", &f2) != 2)
>>> +        return 0;
>>> +
>>> +    return f1 != f2;
>>> +}
>>> +
>>> +static int less(const char *value1, const char *value2, size_t length)
>>> +{
>>> +    float f1, f2;
>>> +
>>> +    if (sscanf(value1, "%f", &f1) + sscanf(value2, "%f", &f2) != 2)
>>> +        return 0;
>>> +
>>> +    return f1 > f2;
>>> +}
>>> +
>>> +static int greater(const char *value1, const char *value2, size_t length)
>>> +{
>>> +    float f1, f2;
>>> +
>>> +    if (sscanf(value1, "%f", &f1) + sscanf(value2, "%f", &f2) != 2)
>>> +        return 0;
>>> +
>>> +    return f1 < f2;
>>> +}
>>> +
>>> [...]
>>
>> I think it would be better to not compare float values directly with
>> "==", "<" or ">". Instead use some code like "fabsf(f1 - f2) <= epsilon".
>>
>> BTW: Is the return value of "equal", "less" and "greater" inverse on
>> purpose?
>
> Not for equal, but for other it is.

OK, now I got it that the argument order is flipped but matches the 
documentation. In that case it is a bit counter-intuitive that one has 
to do something like

mode=print:key=mykey:value=1.0:function=less

to print all metadata values of "mykey" which are *bigger* than 1.0.

>>
>> Another sidenote: I have seen that some filters use a common expression
>> language (e.g. aeval, crop, scale). Not sure if this expression language
>> supports string operators or if it is limited to numbers but in my
>> opinion it would be great to re-use it here.
>
> Added and applied. Filter is easily extendable.

Thanks for adding expression support.

I know my response has come late and I'm only an occasional FFmpeg 
developer so my thoughts might not have big weight. Still I think it 
would have been more polite to first respond on the list to all my 
comments (see the "starts_with" suggestion above) before applying the patch.

Regards,
Tobias