On Fri, 2 Mar 2018 22:48:07 +0100 Michael Niedermayer <mich...@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.r...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel