[FFmpeg-devel] [PATCH] avfilter/vf_cropdetect: add ability to change limit/reset at runtime

[Jeffrey Chapuis]

On 17/01/2023 14:45, Paul B Mahol wrote:
> On 1/17/23, Jeffrey Chapuis <ashyni1987 at gmail.com> wrote:
>> On 17/01/2023 13:34, Paul B Mahol wrote:
>>> On 1/17/23, Jeffrey Chapuis <ashyni1987 at gmail.com> wrote:
>>>> On 17/01/2023 12:52, Paul B Mahol wrote:
>>>>> On 1/17/23, Jeffrey Chapuis <ashyni1987 at gmail.com> wrote:
>>>>>>> Le 10/01/2023 à 16:45, Paul B Mahol a écrit :
>>>>>>>> On 1/10/23, Jeffrey CHAPUIS <ashyni1987 at gmail.com> wrote:
>>>>>>>>> Hello,
>>>>>>>>> I decided to continue on a simpler path without 'reset/reset_count',
>>>>>>>>> it
>>>>>>>>> was
>>>>>>>>> only to experiment anyway, 'limit' is the main goal.
>>>>>>>>> 'limit' is added to the metadata to control that the result is
>>>>>>>>> associated to
>>>>>>>>> a change at runtime, it's after scaling with bitdetph but that's not
>>>>>>>>> really
>>>>>>>>> a problem (at least for me, we can always store the parameter before
>>>>>>>>> any
>>>>>>>>> alteration).
>>>>>>>>>
>>>>>>>>>> +    if (!strcmp(cmd, "limit")) {
>>>>>>>>>> +        if (s->limit < 1.0)
>>>>>>>>>> +            s->limit *= (1 << s->bitdepth) - 1;
>>>>>>>>>> +        s->frame_nb = s->reset_count;
>>>>>>>>>> +    }
>>>>>>>>> Should i remove the if statement here ? or keep it for future change
>>>>>>>>> eventually.
>>>>>>>>
>>>>>>>> Split variables, keep one variable settable by user and unchanged by
>>>>>>>> filter.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Notes I didn't think about?
>>>>>>>>>
>>>>>>>>> Thunderbird altered the patch somehow (remove empty new lines), it's
>>>>>>>>> edited
>>>>>>>>> manually.
>>>>>>>>
>>>>>>>> Attach patch instead.
>>>>>>>>
>>>>>>>>
>>>>>>>> Avoid using strcmp to check for this variable change, instead check
>>>>>>>> with previous and new value in process function.
>>>>>>>
>>>>>>>> Here is part of the updated patch, 'limit' exposed in metadata/log is
>>>>>>>> now
>>>>>>>> coherent with init().
>>>>>>>> Like 'limit/limit_user' is of type float, i've used what's done in
>>>>>>>> av_dict_set_int() to print it as float.
>>>>>>>> Compare 's->limit_user' and 's->limit' to check for a change instead
>>>>>>>> of
>>>>>>>> 'strcmp'.
>>>>>>>> Is there anything to adjust ?
>>>>>>>
>>>>>>> Forgot to update ref file for fate (full patch attached).
>>>>>>
>>>>>> Is the update code good?
>>>>>>
>>>>>>> +    char limit_str[22];
>>>>>>> +        snprintf(limit_str, sizeof(limit_str), "%f", s->limit_user);
>>>>>>> +        av_dict_set(metadata, "lavfi.cropdetect.limit", limit_str,
>>>>>>> 0);
>>>>>>
>>>>>> Should i create a function av_dict_set_float() in libavutil/dict.c and
>>>>>> libavutil/dict.h?
>>>>>
>>>>> Nope.
>>>>>
>>>>> Shouldnt limit variable be changed if < 1.0 before being used in
>>>>> process_command()  ?
>>>>
>>>> You mean before ff_filter_process_command() ?
>>>
>>> Inside that function.
>>>
>>>> I thought ff_filter_process_command() was only checking the command flag
>>>> and
>>>> input value.
>>>
>>> Call to ff_filter_process_command() does update to new values set by user.
>>>
>>> So if limit is lower than 1.0 have special meaning it needs to be
>>> handled properly.
>>>
>>> The ideal solution is thus to keep user supplied value always constant
>>> after its changed by user, and to do operations with it into new
>>> variables.
>>
>> I'm lost, limit_user already keep the user settings untouched before limit
>> is modified if < 1.0
> 
> That is an issue, limit should not ever change except if user set it.

> static int process_command(AVFilterContext *ctx, const char *cmd, const char *args,
>                             char *res, int res_len, int flags)
> {
>     CropDetectContext *s = ctx->priv;
>     int ret;
> 
> +   if (s->limit_user == s->limit)
> +       return AVERROR(ENOSYS);
> +
>     if ((ret = ff_filter_process_command(ctx, cmd, args, res, res_len, flags)) < 0)
>         return ret;
> 
>     if (s->limit_user != s->limit) {
>         s->limit_user = s->limit;
>         if (s->limit < 1.0)
>             s->limit *= (1 << s->bitdepth) - 1;
>         s->frame_nb = s->reset_count;
>     }
> 
>     return 0;
> }

I did not check if the limit was identical to the old one before ff_filter_process_command() set it,
and it wasn't upscale for bitdepth in that case, so we avoid touching limit all together now.
Is this correct?