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

[Tobias Rapp]

On 10.02.2016 10:59, Paul B Mahol wrote:
> On 2/10/16, Tobias Rapp <t.rapp at noa-archive.com> wrote:
>> 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.
>>
>
> Nope, this works fine here. It prints values less than 1.0

Then I have problems understanding the documentation text of "less" and 
"greater".

>>>>
>>>> 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.
>
> You can post another patch anytime here and if it makes sense I will
> gladly apply it.

OK.