Message ID | 20230324141927.69095-1-jeebjp@gmail.com |
---|---|
State | Accepted |
Commit | eb96cfbf57822f7ce0af508d4a9ecd2224cbd565 |
Headers | show |
Series | [FFmpeg-devel] ffmpeg: make timestamp discontinuity logging a warning | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
On date Friday 2023-03-24 16:19:27 +0200, Jan Ekström wrote: > From: Nongji Chen <nchen@aminocom.com> > > In most cases this should only occur once or so per stream in an > input, and in case the logic ends up in an eternal loop, it should > be visible to the end user instead of being completely invisible. > > Signed-off-by: Jan Ekström <jan.ekstrom@24i.com> > --- > fftools/ffmpeg.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > index 438bee8fef..1428988f9d 100644 > --- a/fftools/ffmpeg.c > +++ b/fftools/ffmpeg.c > @@ -3696,7 +3696,7 @@ static void ts_discontinuity_detect(InputFile *ifile, InputStream *ist, > if (FFABS(delta) > 1LL * dts_delta_threshold * AV_TIME_BASE || > pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)) { > ifile->ts_offset_discont -= delta; > - av_log(NULL, AV_LOG_DEBUG, > + av_log(NULL, AV_LOG_WARNING, > "timestamp discontinuity for stream #%d:%d " > "(id=%d, type=%s): %"PRId64", new offset= %"PRId64"\n", > ist->file_index, ist->st->index, ist->st->id, LGTM.
On 3/25/2023 12:43 PM, Stefano Sabatini wrote: > On date Friday 2023-03-24 16:19:27 +0200, Jan Ekström wrote: >> From: Nongji Chen <nchen@aminocom.com> >> >> In most cases this should only occur once or so per stream in an >> input, and in case the logic ends up in an eternal loop, it should >> be visible to the end user instead of being completely invisible. >> >> Signed-off-by: Jan Ekström <jan.ekstrom@24i.com> >> --- >> fftools/ffmpeg.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c >> index 438bee8fef..1428988f9d 100644 >> --- a/fftools/ffmpeg.c >> +++ b/fftools/ffmpeg.c >> @@ -3696,7 +3696,7 @@ static void ts_discontinuity_detect(InputFile *ifile, InputStream *ist, >> if (FFABS(delta) > 1LL * dts_delta_threshold * AV_TIME_BASE || >> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)) { >> ifile->ts_offset_discont -= delta; >> - av_log(NULL, AV_LOG_DEBUG, >> + av_log(NULL, AV_LOG_WARNING, >> "timestamp discontinuity for stream #%d:%d " >> "(id=%d, type=%s): %"PRId64", new offset= %"PRId64"\n", >> ist->file_index, ist->st->index, ist->st->id, > > LGTM. Wont this be a bit noisy? Maybe it could be VERBOSE instead? If it was DEBUG until now, then it was not deemed that important to become a log message shown by default when it was added.
On Sat, Mar 25, 2023 at 5:46 PM James Almer <jamrial@gmail.com> wrote: > > On 3/25/2023 12:43 PM, Stefano Sabatini wrote: > > On date Friday 2023-03-24 16:19:27 +0200, Jan Ekström wrote: > >> From: Nongji Chen <nchen@aminocom.com> > >> > >> In most cases this should only occur once or so per stream in an > >> input, and in case the logic ends up in an eternal loop, it should > >> be visible to the end user instead of being completely invisible. > >> > >> Signed-off-by: Jan Ekström <jan.ekstrom@24i.com> > >> --- > >> fftools/ffmpeg.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > >> index 438bee8fef..1428988f9d 100644 > >> --- a/fftools/ffmpeg.c > >> +++ b/fftools/ffmpeg.c > >> @@ -3696,7 +3696,7 @@ static void ts_discontinuity_detect(InputFile *ifile, InputStream *ist, > >> if (FFABS(delta) > 1LL * dts_delta_threshold * AV_TIME_BASE || > >> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)) { > >> ifile->ts_offset_discont -= delta; > >> - av_log(NULL, AV_LOG_DEBUG, > >> + av_log(NULL, AV_LOG_WARNING, > >> "timestamp discontinuity for stream #%d:%d " > >> "(id=%d, type=%s): %"PRId64", new offset= %"PRId64"\n", > >> ist->file_index, ist->st->index, ist->st->id, > > > > LGTM. > > Wont this be a bit noisy? Maybe it could be VERBOSE instead? > If it was DEBUG until now, then it was not deemed that important to > become a log message shown by default when it was added. In my experience this will log for each stream having a different offset until all streams have gone past the discontinuity point. If it spams, it means that something has gone badly wrong (usually one stream gone to either past or future). And in my opinion that should be shown instead of hiding it by default. Jan
On Sat, Mar 25, 2023 at 6:11 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Sat, Mar 25, 2023 at 5:46 PM James Almer <jamrial@gmail.com> wrote: > > > > On 3/25/2023 12:43 PM, Stefano Sabatini wrote: > > > On date Friday 2023-03-24 16:19:27 +0200, Jan Ekström wrote: > > >> From: Nongji Chen <nchen@aminocom.com> > > >> > > >> In most cases this should only occur once or so per stream in an > > >> input, and in case the logic ends up in an eternal loop, it should > > >> be visible to the end user instead of being completely invisible. > > >> > > >> Signed-off-by: Jan Ekström <jan.ekstrom@24i.com> > > >> --- > > >> fftools/ffmpeg.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > > >> index 438bee8fef..1428988f9d 100644 > > >> --- a/fftools/ffmpeg.c > > >> +++ b/fftools/ffmpeg.c > > >> @@ -3696,7 +3696,7 @@ static void ts_discontinuity_detect(InputFile *ifile, InputStream *ist, > > >> if (FFABS(delta) > 1LL * dts_delta_threshold * AV_TIME_BASE || > > >> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)) { > > >> ifile->ts_offset_discont -= delta; > > >> - av_log(NULL, AV_LOG_DEBUG, > > >> + av_log(NULL, AV_LOG_WARNING, > > >> "timestamp discontinuity for stream #%d:%d " > > >> "(id=%d, type=%s): %"PRId64", new offset= %"PRId64"\n", > > >> ist->file_index, ist->st->index, ist->st->id, > > > > > > LGTM. > > > > Wont this be a bit noisy? Maybe it could be VERBOSE instead? > > If it was DEBUG until now, then it was not deemed that important to > > become a log message shown by default when it was added. > > In my experience this will log for each stream having a different > offset until all streams have gone past the discontinuity point. > > If it spams, it means that something has gone badly wrong (usually one > stream gone to either past or future). And in my opinion that should > be shown instead of hiding it by default. Ping. Are you OK with this explanation/reasoning, or are you still blocking this? Jan
Quoting Jan Ekström (2023-04-03 11:05:24) > > Ping. Are you OK with this explanation/reasoning, or are you still > blocking this? Just push IMO
On Wed, Apr 12, 2023 at 11:15 AM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Jan Ekström (2023-04-03 11:05:24) > > > > Ping. Are you OK with this explanation/reasoning, or are you still > > blocking this? > > Just push IMO > Alright, applied as eb96cfbf57822f7ce0af508d4a9ecd2224cbd565 . Jan
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 438bee8fef..1428988f9d 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -3696,7 +3696,7 @@ static void ts_discontinuity_detect(InputFile *ifile, InputStream *ist, if (FFABS(delta) > 1LL * dts_delta_threshold * AV_TIME_BASE || pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)) { ifile->ts_offset_discont -= delta; - av_log(NULL, AV_LOG_DEBUG, + av_log(NULL, AV_LOG_WARNING, "timestamp discontinuity for stream #%d:%d " "(id=%d, type=%s): %"PRId64", new offset= %"PRId64"\n", ist->file_index, ist->st->index, ist->st->id,