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

[Andreas Rheinhardt]

Thilo Borgmann:
> Am 14.01.22 um 19:05 schrieb Andreas Rheinhardt:
>> Thilo Borgmann:
>>> Am 14.01.22 um 14:17 schrieb "zhilizhao(赵志立)":
>>>>
>>>>
>>>>> On Jan 14, 2022, at 8:14 PM, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>>>>
>>>>> Am 06.01.22 um 12:27 schrieb Thilo Borgmann:
>>>>>> Am 03.01.22 um 16:22 schrieb Thilo Borgmann:
>>>>>>> Am 29.12.21 um 12:46 schrieb Nicolas George:
>>>>>>>> "zhilizhao(赵志立)" (12021-12-29):
>>>>>>>>> How about add a restriction like this:
>>>>>>>>>
>>>>>>>>> if (format.endsWith(“%S"))
>>>>>>>>>    enable the feature
>>>>>>>>> else
>>>>>>>>>    warning message
>>>>>>>>>
>>>>>>>>> It’s a useful feature, it shouldn't create unexpected results, but
>>>>>>>>> doesn’t need to support every use case.
>>>>>>>>
>>>>>>>> I would not oppose it, but I find it inelegant, especially because it
>>>>>>>> requires a different expansion function, localtime_ms instead of
>>>>>>>> localtime.
>>>>>>>>
>>>>>>>> What about this: with the original function "localtime", if the format
>>>>>>>> ends in "%3N", then append the millisecond. It can later be expanded to
>>>>>>>> support %xN at any place in the format for any value of x.
>>>>>>>
>>>>>>> I think best will be to scan the format string for %S and extend it there with .ms part before expanding the rest of it, not? Shouldn't be too expensive for the filter.
>>>>>>>
>>>>>>> Just need to find time to actually implement it. 
>>>>>>
>>>>>> Like v5 as attached.
>>>>
>>>>
>>>>> +    if (tag == 'M' || tag == 'm') {
>>>>> +        char *seconds = av_stristr(fmt, "%S");
>>>>> +        if (seconds) {
>>>>> +            seconds += 2;
>>>>> +            int len = seconds - fmt + 1;
>>>>> +            char *tmp = av_malloc(len);
>>>>> +            av_strlcpy(tmp, fmt, len);
>>>>
>>>> Firstly, mixed variable declaration and statements.
>>>>
>>>> Secondly, I think you don’t need ’tmp’, something like
>>>>
>>>> av_asprintf(“%.*s.%03d%s", len, fmt, (int)(unow % 1000000) / 1000, seconds);
>>>
>>> You know your printf format-string :)
>>>
>>> Thanks, v6 attached.
>>> -Thilo
>>>
>>>
>>
>>>
>>> +            int len = seconds + 2 - fmt;
>>> +            char *fmt_new = av_asprintf("%.*s.%03d%s", len, fmt, (int)(unow % 1000000) / 1000, seconds + 2);
>>> +            av_bprint_strftime(bp, fmt_new, &tm);
>>> +            return 0;
>>> +        }
>>
>> I see an unchecked allocation and a leak. 
> 
> Ok fmt_new might be null, where is the leak?
> 

Where is fmt_new freed?

> 
>> And it seems you are using a
>> format string that comes from the user. This is undefined behaviour if
>> this string contains an invalid conversion specifier.
> 
> I think that was unfortunately true before the patch as well, was it not?

Seems so.

> And if true or not, do we have something in place to check a user string?
> 

Afaik no.

- Andreas