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

[Jeffrey Chapuis]

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
Do I have to ask how you would code it?