| Message ID | 20181215185041.12703-1-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Commit | a1b4f120c031e6697bac9fd8c725d9c37ee36d13 |
| Headers | show |
On Sat, Dec 15, 2018, 20:50 Jan Ekström <jeebjp@gmail.com wrote: > Fixes issues when a subtitle packet is received before PCR for the > program has been received, leading to wildly jumping timestamps > on the lavf client side as well as in the re-ordering logic. > > This usually happens in case of multiplexes where the PCR of a > program is not taken into account with subtitle tracks' DTS/PTS. > -- Friendly ping. Sample where this improves the result is linked in the first attempt @ https://patchwork.ffmpeg.org/patch/11419/ Jan
On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > Fixes issues when a subtitle packet is received before PCR for the > program has been received, leading to wildly jumping timestamps > on the lavf client side as well as in the re-ordering logic. > > This usually happens in case of multiplexes where the PCR of a > program is not taken into account with subtitle tracks' DTS/PTS. > --- > libavformat/mpegts.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index edf6b5701d..8f6ee81cda 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -1219,6 +1219,7 @@ skip: > || pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_SUBTITLE) > ) { > AVProgram *p = NULL; > + int pcr_found = 0; > while ((p = av_find_program_from_stream(pes->stream, p, pes->st->index))) { > if (p->pcr_pid != -1 && p->discard != AVDISCARD_ALL) { > MpegTSFilter *f = pes->ts->pids[p->pcr_pid]; > @@ -1242,6 +1243,7 @@ skip: > // and the pcr error to this packet should be no more than 100 ms. > // TODO: we should interpolate the PCR, not just use the last one > int64_t pcr = f->last_pcr / 300; > + pcr_found = 1; > pes->st->pts_wrap_reference = st->pts_wrap_reference; > pes->st->pts_wrap_behavior = st->pts_wrap_behavior; > if (pes->dts == AV_NOPTS_VALUE || pes->dts < pcr) { > @@ -1258,6 +1260,15 @@ skip: > } > } > } > + > + if (!pcr_found) { > + av_log(pes->stream, AV_LOG_VERBOSE, > + "Forcing DTS/PTS to be unset for a " > + "non-trustworthy PES packet for PID %d as " > + "PCR hasn't been received yet.\n", > + pes->pid); > + pes->dts = pes->pts = AV_NOPTS_VALUE; > + } > } > } > break; Assuming i understand the intend correctly ... could the packet be put in a buffer and once a PCR has been encountered be returned based on that? This seems better as no timestamp would be lost thx [...]
On 12/17/18, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: >> Fixes issues when a subtitle packet is received before PCR for the >> program has been received, leading to wildly jumping timestamps >> on the lavf client side as well as in the re-ordering logic. >> >> This usually happens in case of multiplexes where the PCR of a >> program is not taken into account with subtitle tracks' DTS/PTS. >> --- >> libavformat/mpegts.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c >> index edf6b5701d..8f6ee81cda 100644 >> --- a/libavformat/mpegts.c >> +++ b/libavformat/mpegts.c >> @@ -1219,6 +1219,7 @@ skip: >> || pes->st->codecpar->codec_id == >> AV_CODEC_ID_DVB_SUBTITLE) >> ) { >> AVProgram *p = NULL; >> + int pcr_found = 0; >> while ((p = av_find_program_from_stream(pes->stream, >> p, pes->st->index))) { >> if (p->pcr_pid != -1 && p->discard != >> AVDISCARD_ALL) { >> MpegTSFilter *f = pes->ts->pids[p->pcr_pid]; >> @@ -1242,6 +1243,7 @@ skip: >> // and the pcr error to this packet >> should be no more than 100 ms. >> // TODO: we should interpolate the >> PCR, not just use the last one >> int64_t pcr = f->last_pcr / 300; >> + pcr_found = 1; >> pes->st->pts_wrap_reference = >> st->pts_wrap_reference; >> pes->st->pts_wrap_behavior = >> st->pts_wrap_behavior; >> if (pes->dts == AV_NOPTS_VALUE || >> pes->dts < pcr) { >> @@ -1258,6 +1260,15 @@ skip: >> } >> } >> } >> + >> + if (!pcr_found) { >> + av_log(pes->stream, AV_LOG_VERBOSE, >> + "Forcing DTS/PTS to be unset for a " >> + "non-trustworthy PES packet for PID %d as >> " >> + "PCR hasn't been received yet.\n", >> + pes->pid); >> + pes->dts = pes->pts = AV_NOPTS_VALUE; >> + } >> } >> } >> break; > > Assuming i understand the intend correctly ... > could the packet be put in a buffer and once a PCR has been encountered be > returned based on that? > This seems better as no timestamp would be lost > > thx No, that is awful idea. You do not waste memory on that.
On Mon, Dec 17, 2018 at 8:26 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > > Fixes issues when a subtitle packet is received before PCR for the > > program has been received, leading to wildly jumping timestamps > > on the lavf client side as well as in the re-ordering logic. > > > > This usually happens in case of multiplexes where the PCR of a > > program is not taken into account with subtitle tracks' DTS/PTS. > > --- > > libavformat/mpegts.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > index edf6b5701d..8f6ee81cda 100644 > > --- a/libavformat/mpegts.c > > +++ b/libavformat/mpegts.c > > @@ -1219,6 +1219,7 @@ skip: > > || pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_SUBTITLE) > > ) { > > AVProgram *p = NULL; > > + int pcr_found = 0; > > while ((p = av_find_program_from_stream(pes->stream, p, pes->st->index))) { > > if (p->pcr_pid != -1 && p->discard != AVDISCARD_ALL) { > > MpegTSFilter *f = pes->ts->pids[p->pcr_pid]; > > @@ -1242,6 +1243,7 @@ skip: > > // and the pcr error to this packet should be no more than 100 ms. > > // TODO: we should interpolate the PCR, not just use the last one > > int64_t pcr = f->last_pcr / 300; > > + pcr_found = 1; > > pes->st->pts_wrap_reference = st->pts_wrap_reference; > > pes->st->pts_wrap_behavior = st->pts_wrap_behavior; > > if (pes->dts == AV_NOPTS_VALUE || pes->dts < pcr) { > > @@ -1258,6 +1260,15 @@ skip: > > } > > } > > } > > + > > + if (!pcr_found) { > > + av_log(pes->stream, AV_LOG_VERBOSE, > > + "Forcing DTS/PTS to be unset for a " > > + "non-trustworthy PES packet for PID %d as " > > + "PCR hasn't been received yet.\n", > > + pes->pid); > > + pes->dts = pes->pts = AV_NOPTS_VALUE; > > + } > > } > > } > > break; > > Assuming i understand the intend correctly ... > could the packet be put in a buffer and once a PCR has been encountered be > returned based on that? > This seems better as no timestamp would be lost > > thx > That was one of the initial ideas I had on this (the other was the "just drop/skip the PES packet(s)", for which I posted a patch in order to receive comments). The problems in such a case are: - There technically is no upper bound for the buffering (not 100% sure but in theory I think the demuxer can go on without PCR - of course this kind of stream would be completely against the spec, but so would be files without PMT/PAT which we still support). - How do you set the timestamp at that point... do you try to keep count on which packets with what sort of timestamps had already passed, and adjust according to those - or would you just set it to the PCR? - Do you buffer only the subtitle packets, or also all the other ones? If you only buffer the subtitle packets, you might end up returning the subtitle packet after its intended time of presentation. Additionally to keep in mind: - You already are effectively losing original timestamp information with the timestamp fixing logic. You can disable this logic with `fix_teletext_pts` being set to zero (enabled by default). - AV_NOPTS_VALUE can effectively be thought as "utilize ASAP", and if the comments are correct that should be generally correct. Jan
On Tue, Dec 18, 2018 at 01:24:34AM +0200, Jan Ekström wrote: > On Mon, Dec 17, 2018 at 8:26 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > > > Fixes issues when a subtitle packet is received before PCR for the > > > program has been received, leading to wildly jumping timestamps > > > on the lavf client side as well as in the re-ordering logic. > > > > > > This usually happens in case of multiplexes where the PCR of a > > > program is not taken into account with subtitle tracks' DTS/PTS. > > > --- > > > libavformat/mpegts.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > > index edf6b5701d..8f6ee81cda 100644 > > > --- a/libavformat/mpegts.c > > > +++ b/libavformat/mpegts.c > > > @@ -1219,6 +1219,7 @@ skip: > > > || pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_SUBTITLE) > > > ) { > > > AVProgram *p = NULL; > > > + int pcr_found = 0; > > > while ((p = av_find_program_from_stream(pes->stream, p, pes->st->index))) { > > > if (p->pcr_pid != -1 && p->discard != AVDISCARD_ALL) { > > > MpegTSFilter *f = pes->ts->pids[p->pcr_pid]; > > > @@ -1242,6 +1243,7 @@ skip: > > > // and the pcr error to this packet should be no more than 100 ms. > > > // TODO: we should interpolate the PCR, not just use the last one > > > int64_t pcr = f->last_pcr / 300; > > > + pcr_found = 1; > > > pes->st->pts_wrap_reference = st->pts_wrap_reference; > > > pes->st->pts_wrap_behavior = st->pts_wrap_behavior; > > > if (pes->dts == AV_NOPTS_VALUE || pes->dts < pcr) { > > > @@ -1258,6 +1260,15 @@ skip: > > > } > > > } > > > } > > > + > > > + if (!pcr_found) { > > > + av_log(pes->stream, AV_LOG_VERBOSE, > > > + "Forcing DTS/PTS to be unset for a " > > > + "non-trustworthy PES packet for PID %d as " > > > + "PCR hasn't been received yet.\n", > > > + pes->pid); > > > + pes->dts = pes->pts = AV_NOPTS_VALUE; > > > + } > > > } > > > } > > > break; > > > > Assuming i understand the intend correctly ... > > could the packet be put in a buffer and once a PCR has been encountered be > > returned based on that? > > This seems better as no timestamp would be lost > > > > thx > > > > That was one of the initial ideas I had on this (the other was the > "just drop/skip the PES packet(s)", for which I posted a patch in > order to receive comments). > > The problems in such a case are: > - There technically is no upper bound for the buffering (not 100% sure > but in theory I think the demuxer can go on without PCR - of course > this kind of stream would be completely against the spec, but so would > be files without PMT/PAT which we still support). yes the spec requires a PCR every 100ms but, no, you do not need unlimited buffering, you need to just buffer 100ms or make that rather 400ms to allow for some droped data. > - How do you set the timestamp at that point... do you try to keep > count on which packets with what sort of timestamps had already > passed, and adjust according to those - or would you just set it to > the PCR? I think backward extrapolation based on spec with compensation for timestamp discontinuities is the correct way but just using the timestamp with the duration from the buffer should be fine. You already need the duration so you know when to stop buffering because there are fewer PCR than the spec requires > - Do you buffer only the subtitle packets, or also all the other ones? > If you only buffer the subtitle packets, you might end up returning > the subtitle packet after its intended time of presentation. The buffer would be limited to a few hundread milli seconds (if it works that way, which needs to be tested ...) so i dont think this matters much in either case > > Additionally to keep in mind: > - You already are effectively losing original timestamp information > with the timestamp fixing logic. the existing logic is supposed to just adjusts invalid timestamps or interpolate according to spec. Valid timestamps should theoretically be untouched > You can disable this logic with > `fix_teletext_pts` being set to zero (enabled by default). yes but The user has no way of knowing that a particular file which appears to be missing subtitles at the begin would need this specific flag. In fact the user might not even know that there are subtitles in the file which are not displayed. Or that subtitles displayed at the wrong time are not a flaw in the file but a configuration issue. And there might not even be a user, it might be a automated process converting thousands of files. Worse yet, a file with no PCR would loose every subtitle timestamp in the whole file > - AV_NOPTS_VALUE can effectively be thought as "utilize ASAP", and if > the comments are correct that should be generally correct. This works in a video player, but not in other use cases where the output is not immedeate display but a file requireing timestamps. So i think, this is not a simple bug to fix :( [...]
On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > Fixes issues when a subtitle packet is received before PCR for the > program has been received, leading to wildly jumping timestamps > on the lavf client side as well as in the re-ordering logic. > > This usually happens in case of multiplexes where the PCR of a > program is not taken into account with subtitle tracks' DTS/PTS. > --- > libavformat/mpegts.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) For the record, so this is not lost on IRC and causes misunderstandings <michaelni> JEEB, if you prefer the simple solution iam fine with that too but i might (if i find time) rewrite it to do the buffering so the timestamps arent lost in the cases this affects [...]
On Wed, Dec 19, 2018 at 8:56 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > > Fixes issues when a subtitle packet is received before PCR for the > > program has been received, leading to wildly jumping timestamps > > on the lavf client side as well as in the re-ordering logic. > > > > This usually happens in case of multiplexes where the PCR of a > > program is not taken into account with subtitle tracks' DTS/PTS. > > --- > > libavformat/mpegts.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > For the record, so this is not lost on IRC and causes misunderstandings > <michaelni> JEEB, if you prefer the simple solution iam fine with that too but i might (if i find time) rewrite it to do the buffering so the timestamps arent lost in the cases this affects > My opinion generally is that anything before PCR is received is unsure/non-trustable. So if values are received before PCR that cause issues, I would prefer not to pass on those values - which API clients would then take at face value. Additionally, if you currently attempt to transcode a sample - a la the one I linked - with ffmpeg.c, the initial subtitle timestamp received leads to zero video frames getting encoded (due to the vsync logic thinking that there was an 8-hour jump in the timestamps or so). That could be its own bug, but the timestamp jump is caused by us returning the original timestamp before PCR is available - while all following packets are adjusted to be within margin of the PCR. In other words, this seems to improve handling of such samples, and generally speaking in a spec-compliant MPEG-TS file you should receive PCR quite quickly, and thus the possible negative effects of this change should not be protracted. Thus I would like for this patch to proceed unless someone has noticed issues with such handling. I requested for someone to pass this patch through FATE and according to a helpful soul it passes. I do understand the wish to keep a timestamp that is received, of course. Cases like this always remind me of how upipe has three different timestamps in its packets. One for what was there originally in the input, second that is adjusted for wrap-arounds and such to always be rising - and third that contains the receival timestamp. A lot of applications would only utilize the second one, but having the first and the last one for specific use cases could be quite helpful. Best regards, Jan
On Wed, Dec 19, 2018 at 8:56 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > > Fixes issues when a subtitle packet is received before PCR for the > > program has been received, leading to wildly jumping timestamps > > on the lavf client side as well as in the re-ordering logic. > > > > This usually happens in case of multiplexes where the PCR of a > > program is not taken into account with subtitle tracks' DTS/PTS. > > --- > > libavformat/mpegts.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > For the record, so this is not lost on IRC and causes misunderstandings > <michaelni> JEEB, if you prefer the simple solution iam fine with that too but i might (if i find time) rewrite it to do the buffering so the timestamps arent lost in the cases this affects > As Michael is OK with this, If there are no further comments, I will push this tomorrow morning. As a reminder of what this improves: Sample: https://kuroko.fushizen.eu/samples/2018-04-04-funky_teletext_mux.cut.ts Steps to show difference: 1. ffprobe -of json -show_packets -show_programs - > before.json 2. ffprobe -of json -show_packets -show_programs - > after.json 3. git diff --no-index before.json after.json Example of a failing ffmpeg.c command line without these changes (requires the libzvbi teletext decoder to be enabled): ffmpeg -fix_sub_duration -txt_format text -v verbose -i 2018-04-04-funky_teletext_mux.cut.ts -c:v mpeg4 -c:a aac -c:s ass out.mkv This works after this patch is applied and the incorrect timestamp is no longer there in the first teletext subtitle packet. Jan
On Fri, Dec 21, 2018 at 9:04 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Wed, Dec 19, 2018 at 8:56 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > > > Fixes issues when a subtitle packet is received before PCR for the > > > program has been received, leading to wildly jumping timestamps > > > on the lavf client side as well as in the re-ordering logic. > > > > > > This usually happens in case of multiplexes where the PCR of a > > > program is not taken into account with subtitle tracks' DTS/PTS. > > > --- > > > libavformat/mpegts.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > For the record, so this is not lost on IRC and causes misunderstandings > > <michaelni> JEEB, if you prefer the simple solution iam fine with that too but i might (if i find time) rewrite it to do the buffering so the timestamps arent lost in the cases this affects > > > > As Michael is OK with this, If there are no further comments, I will > push this tomorrow morning. > > As a reminder of what this improves: > Sample: https://kuroko.fushizen.eu/samples/2018-04-04-funky_teletext_mux.cut.ts > > Steps to show difference: > 1. ffprobe -of json -show_packets -show_programs - > before.json > 2. ffprobe -of json -show_packets -show_programs - > after.json > 3. git diff --no-index before.json after.json > > Example of a failing ffmpeg.c command line without these changes > (requires the libzvbi teletext decoder to be enabled): > ffmpeg -fix_sub_duration -txt_format text -v verbose -i > 2018-04-04-funky_teletext_mux.cut.ts -c:v mpeg4 -c:a aac -c:s ass > out.mkv > Actually now that I give it another bit of thought, no need for libzvbi, just -c copy'ing into .ts is enough. Jan
On Fri, Dec 21, 2018 at 9:04 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Wed, Dec 19, 2018 at 8:56 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sat, Dec 15, 2018 at 08:50:41PM +0200, Jan Ekström wrote: > > > Fixes issues when a subtitle packet is received before PCR for the > > > program has been received, leading to wildly jumping timestamps > > > on the lavf client side as well as in the re-ordering logic. > > > > > > This usually happens in case of multiplexes where the PCR of a > > > program is not taken into account with subtitle tracks' DTS/PTS. > > > --- > > > libavformat/mpegts.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > For the record, so this is not lost on IRC and causes misunderstandings > > <michaelni> JEEB, if you prefer the simple solution iam fine with that too but i might (if i find time) rewrite it to do the buffering so the timestamps arent lost in the cases this affects > > > > As Michael is OK with this, If there are no further comments, I will > push this tomorrow morning. > > As a reminder of what this improves: > Sample: https://kuroko.fushizen.eu/samples/2018-04-04-funky_teletext_mux.cut.ts > > Steps to show difference: > 1. ffprobe -of json -show_packets -show_programs - > before.json > 2. ffprobe -of json -show_packets -show_programs - > after.json > 3. git diff --no-index before.json after.json > > Example of a failing ffmpeg.c command line without these changes > (requires the libzvbi teletext decoder to be enabled): > ffmpeg -fix_sub_duration -txt_format text -v verbose -i > 2018-04-04-funky_teletext_mux.cut.ts -c:v mpeg4 -c:a aac -c:s ass > out.mkv > > This works after this patch is applied and the incorrect timestamp is > no longer there in the first teletext subtitle packet. > > Jan Applied to master. Jan
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index edf6b5701d..8f6ee81cda 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -1219,6 +1219,7 @@ skip: || pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_SUBTITLE) ) { AVProgram *p = NULL; + int pcr_found = 0; while ((p = av_find_program_from_stream(pes->stream, p, pes->st->index))) { if (p->pcr_pid != -1 && p->discard != AVDISCARD_ALL) { MpegTSFilter *f = pes->ts->pids[p->pcr_pid]; @@ -1242,6 +1243,7 @@ skip: // and the pcr error to this packet should be no more than 100 ms. // TODO: we should interpolate the PCR, not just use the last one int64_t pcr = f->last_pcr / 300; + pcr_found = 1; pes->st->pts_wrap_reference = st->pts_wrap_reference; pes->st->pts_wrap_behavior = st->pts_wrap_behavior; if (pes->dts == AV_NOPTS_VALUE || pes->dts < pcr) { @@ -1258,6 +1260,15 @@ skip: } } } + + if (!pcr_found) { + av_log(pes->stream, AV_LOG_VERBOSE, + "Forcing DTS/PTS to be unset for a " + "non-trustworthy PES packet for PID %d as " + "PCR hasn't been received yet.\n", + pes->pid); + pes->dts = pes->pts = AV_NOPTS_VALUE; + } } } break;