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

wm4 nfxjfg at googlemail.com
Sat Mar 3 00:14:51 EET 2018


On Fri, 2 Mar 2018 22:48:07 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Mar 02, 2018 at 09:07:06AM +0100, Tobias Rapp wrote:
> > 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.  
> 
> You could first make the formatting the same and then factorize it.
> not saying that should be done, just that this should be the easy was to
> factor it out
> 
> > 
> > So please either consider this v3 patch or the original one posted in
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225775.html  
> 
> iam not against this, but it seems others where

My comments were meant as minor suggestion for improvement (as I
stated, unfortunately only in my second reply), not a demand to rewrite
everything. If he just wants to push that first patch, fine with me.


More information about the ffmpeg-devel mailing list