[FFmpeg-devel] [PATCH] libavutil/timestamp.h: Fix loss of precision in timestamps for silencedetect on long files

[Allan Cady]

Greetings Martin et al,

I've been trying to resubmit this patch based on your earlier suggestions. Most of the tools in the toolchain for this are new to me, so it's been awkward going. 

I did finally get the email sent with the new patch, but for some reason I haven't been able to figure out yet, it's going to the wrong thread. Here's the link to the message with the patch.

https://ffmpeg.org/pipermail/ffmpeg-devel/2024-March/323170.html

Looking forward to comments and to hopefully getting this approved and into the codebase.

Allan







On Friday, February 23, 2024 at 02:47:27 PM PST, Marton Balint <cus at passwd.hu> wrote: 







On Fri, 23 Feb 2024, Allan Cady via ffmpeg-devel wrote:

> [Apologies for the awful mess in the previous email. Trying again with straight text.]
>
> On Thursday, February 22, 2024 at 01:16:19 AM PST, Marton Balint <cus at passwd.hu> wrote:
>
>>> For starters, I'm curious why there are two functions & macros:
>>>
>>> av_ts2str/av_ts_make_string (which used "%" format specifier)
>>  
>> That takes a 64-bit integer timestamp and is actually using "%"PRId64 
>> because that is the correct (portable) format string for an int64_t 
>> variable.
>>  
>>> av_ts2timestr/av_ts_make_time_string (which used "%6g")
>>  
>> That takes an integer timestamp and a rational time base. Float timestamps 
>> (in seconds) is calculated by multiplying the two, that is what is 
>> printed.
>>  
>>>
>>> Do you know the rationale for that? I see that only av_ts2timestr is used in silencedetect.c.
>>>
>>> And are you suggesting we should fold those two functions into one?
>>  
>> No, they have different purpose. The first prints out a timestamps which 
>> can be in any time base. The second prints out a timestamp in seconds.
>
>
> Understood, I think I'm up to speed now. av_ts2str prints a 64-bit integer timestamp;
> av_ts2timestr prints a floating point timestamp in seconds with the timebase applied.
>
>
> In your previous email, you said:
>
>
>> I'd rather just to fix av_ts_make_string to not limit the number of 
>> significant digits. Something like:
>>
>> 1) Print the number in decimal notation with at most 6 fractional digits. 
>> 2) Use less fractional digits if the first format would not fit into 
>> AV_TS_MAX_STRING_SIZE.
>> 3) Use scientific notation if the second format would not fit into 
>> AV_TS_MAX_STRINT_SIZE.
>
>
> I think you probably meant to say "I'd rather just to fix
> av_ts_make_time_string" (not av_ts_make_string)?
> Since it's av_ts_make_time_string() that's formatting floating point.

Yes, indeed.

>
> So it makes sense to me to make the change to av_ts_make_time_string()
> for all timestamps, as you suggest. As for how specifically to format them,
> I'm fine with whatever you think is best, and I'm happy to work on this, but the
> implementation has me a bit stumped for the moment, and I may need some
> help with it. My C language skills are rusty to say the least.

The simplest way is to try snprintf() with 6 fractional digits, and check 
the return value to see how long the string would become.
Based on this you can either accept snprintf result and truncate 
trailing zeros and dots, or try snprintf() with less fractional digits and 
truncate, or fall back to scientific format.

>
> It also occurs to me to wonder, would this warrant a formal problem ticket?
> I haven't looked into how that works for ffmpeg.

Opening a ticket for the problem is not required, but if your patch 
fixes an existing ticket, then you should mention the ticket number
in the commit message.

Regards,

Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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