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

[Paul B Mahol]

On 1/17/23, Jeffrey Chapuis <ashyni1987 at gmail.com> wrote:
> On 17/01/2023 15:29, Paul B Mahol wrote:
>> On 1/17/23, Jeffrey Chapuis <ashyni1987 at gmail.com> wrote:
>>> 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);
>>>> +
>>
>> Remove this 3 lines.
>>
>>>>     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)
>>
>> here replace this with s->limit_user.
>>
>>>>             s->limit *= (1 << s->bitdepth) - 1;
>>
>> same here
>>
>>>>         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?
>
> You actually want to make limit_user the variable being upscale for bitdepth
> and used in filter_frame(),
> and keep limit untouched for metadata and log ? reverse everything ?
>
> The filter is working as expected, right now, I'm more and more lost.
>
>> 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.
>
> This was the only thing missing.
>
> Full patch attached.
>

That code in patch will always trigger check in process_command() when
user modify limit.
Currently it does not matter as only that option can be changed but that is bad
solution for future maintenance.

So yes, you need to rename all variables of limit to limit_user or
reverse your current logic.