Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
On 02.03.2018 23:14, wm4 wrote: On Fri, 2 Mar 2018 22:48:07 +0100 Michael Niedermayer 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 --- 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.ZZ 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. I agree that factorization would make sense but it seems no agreement could be found on how exactly it should be implemented. So just pushed v1 of the patch which fixes the progress output strings followed by the AVBPrint clean-up patch. Thanks, Tobias ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
On Fri, 2 Mar 2018 22:48:07 +0100 Michael Niedermayer 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 > > >>--- > > >> 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.ZZ 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
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 > >>--- > >> 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.ZZ 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 --- 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.ZZ 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 > --- > 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.ZZ 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 [...] thx -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct answer. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 --- 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.ZZ 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? Regards, Tobias From d2141a259d733e9ff8e76f0e33b3e2e449adde14 Mon Sep 17 00:00:00 2001 From: Tobias Rapp Date: Mon, 26 Feb 2018 16:58:24 +0100 Subject: [PATCH v3] fftools/ffmpeg: fix progress log message in case pts is not available Move time string formatting into inline function. Also fixes out_time sign prefix for progress report. Signed-off-by: Tobias Rapp --- fftools/ffmpeg.c | 48 +++- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 3a45f43..f3598ff 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -1518,6 +1518,27 @@ static int reap_filters(int flush) return 0; } +static inline char *pts_to_hms_str(char *buf, size_t buf_size, int64_t pts) +{ +const char *hours_sign; +int hours, mins, secs, usecs; + +if (pts == AV_NOPTS_VALUE) { +snprintf(buf, buf_size, "N/A"); +} else { +hours_sign = (pts < 0) ? "-" : ""; +secs = FFABS(pts) / AV_TIME_BASE; +usecs = FFABS(pts) % AV_TIME_BASE; +mins = secs / 60; +secs %= 60; +hours = mins / 60; +mins %= 60; +snprintf(buf, buf_size, "%s%02d:%02d:%02d.%02d", + hours_sign, hours, mins, secs, (100 * usecs) / AV_TIME_BASE); +} +return buf; +} + static void print_final_stats(int64_t total_size) { uint64_t video_size = 0, audio_size = 0, extra_size = 0, other_size = 0; @@ -1649,7 +1670,7 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti int64_t pts = INT64_MIN + 1; static int64_t last_time = -1; static int qp_histogram[52]; -int hours, mins, secs, us; +char buf_pts[AV_TS_MAX_STRING_SIZE] = {0}; int ret; float t; @@ -1751,25 +1772,15 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti nb_frames_drop += ost->last_dropped; } -secs = FFABS(pts) / AV_TIME_BASE; -us = FFABS(pts) % AV_TIME_BASE; -mins = secs / 60; -secs %= 60; -hours = mins / 60; -mins %= 60; - bitrate = pts && total_size >= 0 ? total_size * 8 / (pts / 1000.0) : -1; speed = t != 0.0 ? (double)pts / AV_TIME_BASE / t : -1; if (total_size < 0) snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), - "size=N/A time="); + "size=N/A "); elsesnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), - "size=%8.0fkB time=", total_size / 1024.0); -if (pts < 0) -snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "-"); + "size=%8.0fkB ", total_size / 1024.0); snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), - "%02d:%02d:%02d.%02d ", hours, mins, secs, - (100 * us) / AV_TIME_BASE); + "time=%s ", pts_to_hms_str(buf_pts, sizeof(buf_pts), pts)); if (bitrate < 0) { snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf),"bitrate=N/A"); @@ -1781,9 +1792,12 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti if (total_size < 0) av_bprintf(&buf_script, "total_size=N/A\n"); elseav_bprintf(&buf_script, "total_s
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 > >>--- > >> fftools/ffmpeg.c | 48 +++- > >> 1 file changed, 31 insertions(+), 17 deletions(-) > >> > >>diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > >>index 32caa4b..0097a7d 100644 > >>--- a/fftools/ffmpeg.c > >>+++ b/fftools/ffmpeg.c > >>@@ -1518,6 +1518,27 @@ static int reap_filters(int flush) > >> return 0; > >> } > >>+static inline char *pts_to_hms_str(char *buf, int64_t pts, unsigned int > >>digits) > > > >char buf[AV_TS_MAX_STRING_SIZE] > > > >or the buf size should be passed too, in fact this might be better anyway > > Will change. > > >>+{ > >>+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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Avoid a single point of failure, be that a person or equipment. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 --- fftools/ffmpeg.c | 48 +++- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 32caa4b..0097a7d 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -1518,6 +1518,27 @@ static int reap_filters(int flush) return 0; } +static inline char *pts_to_hms_str(char *buf, int64_t pts, unsigned int digits) char buf[AV_TS_MAX_STRING_SIZE] or the buf size should be passed too, in fact this might be better anyway Will change. +{ +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. Regards, Tobias ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
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 > --- > fftools/ffmpeg.c | 48 +++- > 1 file changed, 31 insertions(+), 17 deletions(-) > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > index 32caa4b..0097a7d 100644 > --- a/fftools/ffmpeg.c > +++ b/fftools/ffmpeg.c > @@ -1518,6 +1518,27 @@ static int reap_filters(int flush) > return 0; > } > > +static inline char *pts_to_hms_str(char *buf, int64_t pts, unsigned int > digits) char buf[AV_TS_MAX_STRING_SIZE] or the buf size should be passed too, in fact this might be better anyway > +{ > +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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH v2 1/2] fftools/ffmpeg: fix progress log message in case pts is not available
Move time string formatting into inline function. Also fixes out_time sign prefix for progress report. Signed-off-by: Tobias Rapp --- fftools/ffmpeg.c | 48 +++- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 32caa4b..0097a7d 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -1518,6 +1518,27 @@ static int reap_filters(int flush) return 0; } +static inline char *pts_to_hms_str(char *buf, int64_t pts, unsigned int digits) +{ +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; +snprintf(buf, AV_TS_MAX_STRING_SIZE, "%s%02d:%02d:%0*.*f", + hours_sign, hours, mins, digits+3, digits, secs); +} +return buf; +} + static void print_final_stats(int64_t total_size) { uint64_t video_size = 0, audio_size = 0, extra_size = 0, other_size = 0; @@ -1649,7 +1670,7 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti int64_t pts = INT64_MIN + 1; static int64_t last_time = -1; static int qp_histogram[52]; -int hours, mins, secs, us; +char buf_pts[AV_TS_MAX_STRING_SIZE] = {0}; int ret; float t; @@ -1751,25 +1772,15 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti nb_frames_drop += ost->last_dropped; } -secs = FFABS(pts) / AV_TIME_BASE; -us = FFABS(pts) % AV_TIME_BASE; -mins = secs / 60; -secs %= 60; -hours = mins / 60; -mins %= 60; - bitrate = pts && total_size >= 0 ? total_size * 8 / (pts / 1000.0) : -1; speed = t != 0.0 ? (double)pts / AV_TIME_BASE / t : -1; if (total_size < 0) snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), - "size=N/A time="); + "size=N/A "); elsesnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), - "size=%8.0fkB time=", total_size / 1024.0); -if (pts < 0) -snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "-"); + "size=%8.0fkB ", total_size / 1024.0); snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), - "%02d:%02d:%02d.%02d ", hours, mins, secs, - (100 * us) / AV_TIME_BASE); + "time=%s ", pts_to_hms_str(buf_pts, pts, 2)); if (bitrate < 0) { snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf),"bitrate=N/A"); @@ -1781,9 +1792,12 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti if (total_size < 0) av_bprintf(&buf_script, "total_size=N/A\n"); elseav_bprintf(&buf_script, "total_size=%"PRId64"\n", total_size); -av_bprintf(&buf_script, "out_time_ms=%"PRId64"\n", pts); -av_bprintf(&buf_script, "out_time=%02d:%02d:%02d.%06d\n", - hours, mins, secs, us); +if (pts == AV_NOPTS_VALUE) { +av_bprintf(&buf_script, "out_time_ms=N/A\n"); +} else { +av_bprintf(&buf_script, "out_time_ms=%"PRId64"\n", pts); +} +av_bprintf(&buf_script, "out_time=%s\n", pts_to_hms_str(buf_pts, pts, 6)); if (nb_frames_dup || nb_frames_drop) snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), " dup=%d drop=%d", -- 2.7.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel