[FFmpeg-devel] ffmpeg: correct units for raw pts in -progress report

Submitted by Gyan on Aug. 5, 2018, 7:10 a.m.

Details

Message ID 63a22eb9-ce4a-c204-19fe-4704e03bbeb6@gmail.com
State New
Headers show

Commit Message

Gyan Aug. 5, 2018, 7:10 a.m.
From a45b0ade691816d8037c846f2b773d0ddab74cbe Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Sun, 5 Aug 2018 12:34:21 +0530
Subject: [PATCH] ffmpeg: correct units for raw pts in -progress report

PTS is in microseconds.

Fixes #7345
---
 fftools/ffmpeg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Aug. 5, 2018, 8:16 a.m.
> Am 05.08.2018 um 09:10 schrieb Gyan Doshi <gyandoshi@gmail.com>:
> 
> <0001-ffmpeg-correct-units-for-raw-pts-in-progress-report.patch>

>  av_bprintf(&buf_script, "out_time_ms=N/A\n");

If the patch is a good idea you should also change this line.

Thank you, Carl Eugen
Gyan Aug. 5, 2018, 8:43 a.m.
On 05-08-2018 01:46 PM, Carl Eugen Hoyos wrote:
> 
>>   av_bprintf(&buf_script, "out_time_ms=N/A\n");
> 
> If the patch is a good idea you should also change this line.

Fixed locally.

Will push soon, if nothing else.

Thanks,
Gyan
Nicolas George Aug. 5, 2018, 8:47 a.m.
Gyan Doshi (2018-08-05):
> Will push soon, if nothing else.

Have you considered you may be breaking some user's scripts?

Carl Eugen's comment was in no way an approval of your patch. Pushing a
patch without a second positive opinion in less than half a day...

Regards,
Gyan Aug. 5, 2018, 8:58 a.m.
On 05-08-2018 02:17 PM, Nicolas George wrote:
> Gyan Doshi (2018-08-05):
>> Will push soon, if nothing else.
> 
> Have you considered you may be breaking some user's scripts?

Ah yes, Sorry, this is used for parsing. What protocol do you suggest 
for correcting this?

Regards,
Gyan
Gyan Aug. 6, 2018, 4:55 a.m.
On 05-08-2018 02:28 PM, Gyan Doshi wrote:

> Ah yes, Sorry, this is used for parsing. What protocol do you suggest 
> for correcting this?

Any thoughts on this?

I see that there were two changes made to value fields of the progress 
report earlier this year - a194e9c4159 and 1322b00060

Regards,
Gyan
Nicolas George Aug. 6, 2018, 2:30 p.m.
Gyan Doshi (2018-08-06):
> >Ah yes, Sorry, this is used for parsing. What protocol do you suggest for
> >correcting this?
> 
> Any thoughts on this?

Not really. I suppose you could add the field with the correct name and
remove the bogus one later. But your thoughts on this are probably
better than mine.

> I see that there were two changes made to value fields of the progress
> report earlier this year - a194e9c4159 and 1322b00060

It is true, but past mistakes cannot be used to justify new ones.

Also, it seems to me theses changes are much less serious: 1322b00060
only changes the precision in a compatible way, while the second only
changes anything when the required value is unavailable.

Regards,
Gyan Aug. 7, 2018, 7:15 p.m.
On 06-08-2018 08:00 PM, Nicolas George wrote:

> Not really. I suppose you could add the field with the correct name and
> remove the bogus one later. But your thoughts on this are probably
> better than mine.

That will just defer the breaking change. The options, as I see it, are

wontfix, or

push with a easily grepped intuitive commit msg for those who like to 
use nightly builds and want to know why their script is (partially) 
broken. Best done at the time of a new release.

Regards,
Gyan
Nicolas George Aug. 7, 2018, 7:17 p.m.
Gyan Doshi (2018-08-08):
> That will just defer the breaking change.

That will leave people time to notice the change and allow old and new
scripts to work during the transition.

Regards,
Gyan Aug. 9, 2018, 4:39 a.m.
On 08-08-2018 12:47 AM, Nicolas George wrote:
> Gyan Doshi (2018-08-08):
>> That will just defer the breaking change.
> 
> That will leave people time to notice the change and allow old and new
> scripts to work during the transition.

Will do it this way.

Gyan

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 55faec8ede..2bc550dfe8 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -1797,7 +1797,7 @@  static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti
         av_bprintf(&buf_script, "out_time_ms=N/A\n");
         av_bprintf(&buf_script, "out_time=N/A\n");
     } else {
-        av_bprintf(&buf_script, "out_time_ms=%"PRId64"\n", pts);
+        av_bprintf(&buf_script, "out_time_us=%"PRId64"\n", pts);
         av_bprintf(&buf_script, "out_time=%s%02d:%02d:%02d.%06d\n",
                    hours_sign, hours, mins, secs, us);
     }