Re: [FFmpeg-devel] [PATCH v2] lavfi/drawtext: Add localtime_ms for millisecond precision

[Thilo Borgmann <thilo.borgmann@mail.de>]

Am 20.01.22 um 16:32 schrieb Thilo Borgmann:
> Am 20.01.22 um 16:03 schrieb Andreas Rheinhardt:
>> Thilo Borgmann:
>>> Am 20.01.22 um 13:04 schrieb Thilo Borgmann:
>>>> Am 19.01.22 um 04:16 schrieb "zhilizhao(赵志立)":
>>>>>
>>>>>
>>>>>> On Jan 18, 2022, at 8:52 PM, Thilo Borgmann <thilo.borgmann@mail.de>
>>>>>> wrote:
>>>>>>
>>>>>> Am 16.01.22 um 12:06 schrieb Nicolas George:
>>>>>>> Thilo Borgman (12022-01-14):
>>>>>>>> v6 does:
>>>>>>>>
>>>>>>>> $> ffmpeg ... drawtext="fontfile=...:text='%{localtime   \:%a %b
>>>>>>>> %d %Y %S}'"           (seconds)
>>>>>>>> $> ffmpeg ... drawtext="fontfile=...:text='%{localtime_ms\:%a %b
>>>>>>>> %d %Y %S}'"           (milliseconds)
>>>>>>>>
>>>>>>>> I suggest v7 should according to your remark:
>>>>>>>>
>>>>>>>> $> ffmpeg ... drawtext="fontfile=...:text='%{localtime   \:%a %b
>>>>>>>> %d %Y %S}'"           (seconds)
>>>>>>>> $> ffmpeg ... drawtext="fontfile=...:text='%{localtime   \:%a %b
>>>>>>>> %d %Y %S}':show_ms=1" (milliseconds)
>>>>>>>>
>>>>>>>> Good?
>>>>>>>
>>>>>>> I dislike both versions, from a user interface point of view: if there
>>>>>>> is a format string, then it stands to reason, for the user, that the
>>>>>>> resulting text is governed by the format string, not by an extra
>>>>>>> option
>>>>>>> somewhere else.
>>>>>>>
>>>>>>> There is no "use_four_digit_year=1" option, there is %Y instead of %y.
>>>>>>>
>>>>>>> There is no "use_slashes=1" option, you write %Y/%m/%d instead of
>>>>>>> %Y-%m-%d.
>>>>>>>
>>>>>>> There are no "omit_date=1" and "omit_hour=1" options, you just write
>>>>>>> what you want in the format string.
>>>>>>>
>>>>>>> My proposal goes the same way:
>>>>>>>
>>>>>>> $> ffmpeg ... drawtext="fontfile=...:text='%{localtime   \:%a %b %d
>>>>>>> %Y %S.%3N}'"
>>>>>>>
>>>>>>> It has several merits over your proposal:
>>>>>>>
>>>>>>> - It can be extended later to support printing the milliseconds at
>>>>>>>    another place than the end (for example to put the time in
>>>>>>> brackets).
>>>>>>>
>>>>>>> - It can be extended to support microseconds or centiseconds (%6N,
>>>>>>> %2N).
>>>>>>>
>>>>>>> - It is somewhat compatible with GNU date and possibly a few others.
>>>>>>>
>>>>>>> And I do not think it is harder to implement.
>>>>>>
>>>>>> Ok, did introduce a variable: %[1-6]N
>>>>>> Parsing and clipping value to valid range of 1-6.
>>>>>> Default 3.
>>>>>>
>>>>>> That way it is position independent and can show any number of
>>>>>> decimals from 1 to 6.
>>>>>>
>>>>>
>>>>>> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
>>>>>> index 2a88692cbd..448b174dbb 100644
>>>>>> --- a/libavfilter/vf_drawtext.c
>>>>>> +++ b/libavfilter/vf_drawtext.c
>>>>>> @@ -51,6 +51,7 @@
>>>>>>    #include "libavutil/opt.h"
>>>>>>    #include "libavutil/random_seed.h"
>>>>>>    #include "libavutil/parseutils.h"
>>>>>> +#include "libavutil/time.h"
>>>>>>    #include "libavutil/timecode.h"
>>>>>>    #include "libavutil/time_internal.h"
>>>>>>    #include "libavutil/tree.h"
>>>>>> @@ -1045,14 +1046,82 @@ static int func_strftime(AVFilterContext
>>>>>> *ctx, AVBPrint *bp,
>>>>>>                             char *fct, unsigned argc, char **argv,
>>>>>> int tag)
>>>>>>    {
>>>>>>        const char *fmt = argc ? argv[0] : "%Y-%m-%d %H:%M:%S";
>>>>>> +    int64_t unow;
>>>>>>        time_t now;
>>>>>>        struct tm tm;
>>>>>> -
>>>>>> -    time(&now);
>>>>>> -    if (tag == 'L')
>>>>>> +    char *begin;
>>>>>> +    char *tmp;
>>>>>> +    int len;
>>>>>> +    char *fmt_new;
>>>>>> +    const char *fmt_tmp;
>>>>>> +    int div;
>>>>>> +
>>>>>> +    unow = av_gettime();
>>>>>> +    now  = unow / 1000000;
>>>>>> +    if (tag == 'L' || tag == 'm')
>>>>>>            localtime_r(&now, &tm);
>>>>>>        else
>>>>>>            tm = *gmtime_r(&now, &tm);
>>>>>> +
>>>>>> +    // manually parse format for %N (fractional seconds)
>>>>>> +    begin = (char*)fmt;
>>>>>
>>>>> Make begin and tmp const char *, so the cast can be removed.
>>>>>
>>>>>> +    while ((begin = av_stristr(begin, "%"))) {
>>>>>
>>>>> How about strstr() since ‘%’ is caseless?
>>>>>
>>>>>> +        tmp = begin + 1;
>>>>>> +        len = 0;
>>>>>> +        // count digits between % and possible N
>>>>>> +        while (*tmp != '\0' && av_isdigit((int)*tmp)) {
>>>>>> +            len++;
>>>>>> +            tmp++;
>>>>>> +        }
>>>>>> +        // N encountered, insert time
>>>>>> +        if (*tmp == 'N') {
>>>>>> +            int num_digits = 3; // default show millisecond [1,6]
>>>>>> +
>>>>>> +            // if digits given, parse as number in [1,6]
>>>>>> +            if (len > 0) {
>>>>>> +                av_sscanf(begin + 1, "%i", &num_digits);
>>>>>> +                num_digits = av_clip(num_digits, 1, 6); // ensure
>>>>>> valid value
>>>>>
>>>>> We can ignore len > 1, then the code can be simplified as
>>>>>
>>>>> if (len == 1)
>>>>>       num_digits = av_clip(*(begin + 1) - ‘\0’, 1, 6)
>>>>>
>>>>>
>>>>>> +            }
>>>>>> +
>>>>>> +            len += 2; // add % and N to get length of string part
>>>>>> +
>>>>>> +            switch(num_digits) {
>>>>>> +            case 1:
>>>>>> +                fmt_tmp = "%.*s%01d%s";
>>>>>> +                div     = 100000;
>>>>>> +                break;
>>>>>> +            case 2:
>>>>>> +                fmt_tmp = "%.*s%02d%s";
>>>>>> +                div     = 10000;
>>>>>> +                break;
>>>>>> +            case 3:
>>>>>> +                fmt_tmp = "%.*s%03d%s";
>>>>>> +                div     = 1000;
>>>>>> +                break;
>>>>>> +            case 4:
>>>>>> +                fmt_tmp = "%.*s%04d%s";
>>>>>> +                div     = 100;
>>>>>> +                break;
>>>>>> +            case 5:
>>>>>> +                fmt_tmp = "%.*s%05d%s";
>>>>>> +                div     = 10;
>>>>>> +                break;
>>>>>> +            case 6:
>>>>>> +                fmt_tmp = "%.*s%06d%s";
>>>>>> +                div     = 1;
>>>>>> +                break;
>>>>>> +            }
>>>>>
>>>>> The switch-case can be replaced by “%0*d” and pow(10, 6 - num_digits).
>>>>
>>>> Indeed, simplified.
>>>>
>>>>
>>>>>> +
>>>>>> +            fmt_new = av_asprintf(fmt_tmp, begin - fmt, fmt,
>>>>>> (int)(unow % 1000000) / div, begin + len);
>>>>>> +            if (!fmt_new)
>>>>>> +                return AVERROR(ENOMEM);
>>>>>> +            av_bprint_strftime(bp, fmt_new, &tm);
>>>>>> +            av_freep(&fmt_new);
>>>>>> +            return 0;
>>>>>> +        }
>>>>>> +        begin++;
>>>>>
>>>>> Progress faster by taking account of len.
>>>>
>>>> As well, also added to skip "%%".
>>>>
>>>>
>>>>>> +    }
>>>>>> +
>>>>>>        av_bprint_strftime(bp, fmt, &tm);
>>>>>>        return 0;
>>>>>>    }
>>>>>> -- 
>>>>
>>>> v8 attached.
>>>
>>> Fixed off-by-one bug.
>>> Allows for several occurrences of %N parameter now.
>>>
>>> v9 attached.
>>>
>>> Thanks,
>>> Thilo
>>>
>>>
>>> +    // manually parse format for %N (fractional seconds)
>>> +    begin = fmt;
>>> +    while ((begin = av_stristr(begin, "%"))) {
>>> +        tmp = begin + 1;
>>
>> begin = strchr(begin, '%')
> 
> Done.
> Also fixed buggy freep() for non user-supplied format string.
> 
> v10 attached.

Also going to apply soon if there are no more comments.

-Thilo
_______________________________________________
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".