diff mbox series

[FFmpeg-devel] ffmpeg: make timestamp discontinuity logging a warning

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

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Jan Ekström March 24, 2023, 2:19 p.m. UTC 
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(-)

Comments

Stefano Sabatini March 25, 2023, 3:43 p.m. UTC | #1 
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.
James Almer March 25, 2023, 3:46 p.m. UTC | #2 
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.
Jan Ekström March 25, 2023, 4:11 p.m. UTC | #3 
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
Jan Ekström April 3, 2023, 9:05 a.m. UTC | #4 
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
Anton Khirnov April 12, 2023, 8:15 a.m. UTC | #5 
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
Jan Ekström April 14, 2023, 12:15 p.m. UTC | #6 
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 mbox series

Patch

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,