On 17/01/2023 15:29, Paul B Mahol wrote:
> On 1/17/23, Jeffrey Chapuis <ashyni1987@gmail.com> wrote:
>> On 17/01/2023 14:45, Paul B Mahol wrote:
>>> On 1/17/23, Jeffrey Chapuis <ashyni1987@gmail.com> wrote:
>>>> On 17/01/2023 13:34, Paul B Mahol wrote:
>>>>> On 1/17/23, Jeffrey Chapuis <ashyni1987@gmail.com> wrote:
>>>>>> On 17/01/2023 12:52, Paul B Mahol wrote:
>>>>>>> On 1/17/23, Jeffrey Chapuis <ashyni1987@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.