[FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available

[Tobias Rapp]

On 01.03.2018 22:08, Michael Niedermayer wrote:
> On Wed, Feb 28, 2018 at 09:47:15AM +0100, Tobias Rapp wrote:
>> On 27.02.2018 19:03, Michael Niedermayer wrote:
>>> On Tue, Feb 27, 2018 at 08:49:19AM +0100, Tobias Rapp wrote:
>>>> On 27.02.2018 01:12, Michael Niedermayer wrote:
>>>>> On Mon, Feb 26, 2018 at 05:09:04PM +0100, Tobias Rapp wrote:
>>>>>> Move time string formatting into inline function. Also fixes out_time
>>>>>> sign prefix for progress report.
>>>>>>
>>>>>> Signed-off-by: Tobias Rapp <t.rapp at noa-archive.com>
>>>>>> ---
>>>>>>   fftools/ffmpeg.c | 48 +++++++++++++++++++++++++++++++-----------------
>>>>>>   1 file changed, 31 insertions(+), 17 deletions(-)
>>>>>>
>>>>> [...]
>>>>>> +{
>>>>>> +    const char *hours_sign;
>>>>>> +    int hours, mins;
>>>>>> +    double secs;
>>>>>> +
>>>>>> +    if (pts == AV_NOPTS_VALUE) {
>>>>>> +        snprintf(buf, AV_TS_MAX_STRING_SIZE, "N/A");
>>>>>> +    } else {
>>>>>> +        hours_sign = (pts < 0) ? "-" : "";
>>>>>> +        secs = (double)FFABS(pts) / AV_TIME_BASE;
>>>>>> +        mins = (int)secs / 60;
>>>>>> +        secs = secs - mins * 60;
>>>>>> +        hours = mins / 60;
>>>>>> +        mins %= 60;
>>>>>
>>>>> This is not the same code, also with double it can produce inexact
>>>>> results and results differing between platforms
>>>>
>>>> I changed secs to double to handle the cases with different number of
>>>> sub-second digits more easily. Would it be OK to output two digits after the
>>>> decimal point in both cases? The progress report contains the precise
>>>> out_time_ms value anyway.
>>>
>>> iam not sure iam guessing correctly what you mean by "both cases"
>>> you mean if its unneeded as in .00 ?
>>> I guess that would be ok
>>
>> There are two places within print_report() that output
>> hour/minute/seconds-formatted time. One is using HH:MM:SS.ZZ format and the
>> other one is using HH:MM:SS.ZZZZZZ format. Would it be OK to output
>> HH:MM:SS.ZZ (two digits after the decimal separator) in both places like in
>> the attached patch version?
> 
> iam not sure if the reduction of precission is a problem for some use case or not
> But such a change doesnt belong in a patch factorizing the code.
> It should be done seperately, if its changed

Factorizing the code *and* keeping the exact same behavior in both cases 
is pointless in my eyes as it just increases the amount and complexity 
of code for low benefit.

So please either consider this v3 patch or the original one posted in 
https://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225775.html

All I wanted to get fixed was the large negative value printed at the 
start of transcoding ... *sigh*

Regards,
Tobias