| Message ID | 20200812221405.25604-1-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Commit | c820c2d4bfb7e2573f7bf8e2a3544a98e5a11343 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mpegts: only reset timestamps to NOPTS for DVB teletext | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Thu, 13 Aug 2020, Jan Ekström wrote: > While having the possibility of non-NOPTS values that can suddenly > jump in time due to adjustments to match PCR is not nice for DVB > subtitles, apparently the parser for this format bases its behavior on > whether the packets' timestamps are NOPTS or not. Actually what matters is that the parser separates packets which have different PTS values. Having a DVB subtitle packet with no PTS is not even valid based on what I read from the specs: "Each PES header shall carry a PTS, associated with all the subtitle data contained within that PES packet. " So I guess current code assumes that if a packet has no PTS then it is also part of the previous packet, but this also seems like a workaround for bad streams... > Thus while we can adjust timestamps, we should exclude DVB subtitles > from the timestamp unsetting logic. Ok, but to be frank the timestamp setting logic (even when it is used with PCR) also breaks the parser, because it might assign different PTS to PES packets which have the same PTS... > > Fixes #8844 > --- > libavformat/mpegts.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index f71f18a57d..50d4d5e9bc 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -1343,7 +1343,8 @@ skip: > } > } > > - if (!pcr_found) { > + if (pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_TELETEXT && > + !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 " Patch LGTM, I am just saying that the problems lies more deep... Regards, Marton
On Thu, Aug 13, 2020 at 10:34 AM Marton Balint <cus@passwd.hu> wrote: > > > > On Thu, 13 Aug 2020, Jan Ekström wrote: > > > While having the possibility of non-NOPTS values that can suddenly > > jump in time due to adjustments to match PCR is not nice for DVB > > subtitles, apparently the parser for this format bases its behavior on > > whether the packets' timestamps are NOPTS or not. > > Actually what matters is that the parser separates packets which have > different PTS values. Having a DVB subtitle packet with no PTS is not even > valid based on what I read from the specs: > > "Each PES header shall carry a PTS, associated with all the subtitle data > contained within that PES packet. " > > So I guess current code assumes that if a packet has no PTS then it is > also part of the previous packet, but this also seems like a workaround > for bad streams... > Yup, it's layers upon layers of fun :) Just like a box of chocolates or an onion. I posted this patch to get a discussion rolling, as I was highlighted on the parser being broken with NOPTS. > > Thus while we can adjust timestamps, we should exclude DVB subtitles > > from the timestamp unsetting logic. > > Ok, but to be frank the timestamp setting logic (even when it is > used with PCR) also breaks the parser, because it might assign different > PTS to PES packets which have the same PTS... > Yup. Cascades of things and all that jazz are fun. I just addressed the most obvious bit which I caused by my unsetting of timestamps (which in turn was to make sure that the time line doesn't suddenly jump X hours back or so). On trac reverting http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=42aa02418e43802b4ebcca373d2413ab63a0307e was mentioned, but that seems to have been a case fixing https://trac.ffmpeg.org/ticket/4200 . The ticket lacks exact details, but I expect there to be a bad mux with the subtitle timestamps not matching the general time line according to PCR. > > > > Fixes #8844 > > --- > > libavformat/mpegts.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > index f71f18a57d..50d4d5e9bc 100644 > > --- a/libavformat/mpegts.c > > +++ b/libavformat/mpegts.c > > @@ -1343,7 +1343,8 @@ skip: > > } > > } > > > > - if (!pcr_found) { > > + if (pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_TELETEXT && > > + !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 " > > Patch LGTM, I am just saying that the problems lies more deep... > Thanks. Unless people have a better idea of how to go forward with this, I'll apply this patch. Jan
On Thu, Aug 13, 2020 at 12:52 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Thu, Aug 13, 2020 at 10:34 AM Marton Balint <cus@passwd.hu> wrote: > > > > > > > > On Thu, 13 Aug 2020, Jan Ekström wrote: > > > > > While having the possibility of non-NOPTS values that can suddenly > > > jump in time due to adjustments to match PCR is not nice for DVB > > > subtitles, apparently the parser for this format bases its behavior on > > > whether the packets' timestamps are NOPTS or not. > > > > Actually what matters is that the parser separates packets which have > > different PTS values. Having a DVB subtitle packet with no PTS is not even > > valid based on what I read from the specs: > > > > "Each PES header shall carry a PTS, associated with all the subtitle data > > contained within that PES packet. " > > > > So I guess current code assumes that if a packet has no PTS then it is > > also part of the previous packet, but this also seems like a workaround > > for bad streams... > > > > Yup, it's layers upon layers of fun :) Just like a box of chocolates > or an onion. > > I posted this patch to get a discussion rolling, as I was highlighted > on the parser being broken with NOPTS. > > > > Thus while we can adjust timestamps, we should exclude DVB subtitles > > > from the timestamp unsetting logic. > > > > Ok, but to be frank the timestamp setting logic (even when it is > > used with PCR) also breaks the parser, because it might assign different > > PTS to PES packets which have the same PTS... > > > > Yup. Cascades of things and all that jazz are fun. I just addressed > the most obvious bit which I caused by my unsetting of timestamps > (which in turn was to make sure that the time line doesn't suddenly > jump X hours back or so). > > On trac reverting > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=42aa02418e43802b4ebcca373d2413ab63a0307e > was mentioned, but that seems to have been a case fixing > https://trac.ffmpeg.org/ticket/4200 . The ticket lacks exact details, > but I expect there to be a bad mux with the subtitle timestamps not > matching the general time line according to PCR. > > > > > > > Fixes #8844 > > > --- > > > libavformat/mpegts.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > > index f71f18a57d..50d4d5e9bc 100644 > > > --- a/libavformat/mpegts.c > > > +++ b/libavformat/mpegts.c > > > @@ -1343,7 +1343,8 @@ skip: > > > } > > > } > > > > > > - if (!pcr_found) { > > > + if (pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_TELETEXT && > > > + !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 " > > > > Patch LGTM, I am just saying that the problems lies more deep... > > > > Thanks. Unless people have a better idea of how to go forward with > this, I'll apply this patch. > As no better ideas were brought up, applied this as c820c2d4bfb7e2573f7bf8e2a3544a98e5a11343 . Jan
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index f71f18a57d..50d4d5e9bc 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -1343,7 +1343,8 @@ skip: } } - if (!pcr_found) { + if (pes->st->codecpar->codec_id == AV_CODEC_ID_DVB_TELETEXT && + !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 "