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

[Paul B Mahol <onemda@gmail.com>]

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?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".