diff mbox series

[FFmpeg-devel] avformat/mpegts: only reset timestamps to NOPTS for DVB teletext

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström Aug. 12, 2020, 10:14 p.m. UTC
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. Thus while we can
adjust timestamps, we should exclude DVB subtitles from the timestamp
unsetting logic.

Fixes #8844
---
 libavformat/mpegts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marton Balint Aug. 13, 2020, 7:33 a.m. UTC | #1
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
Jan Ekström Aug. 13, 2020, 9:52 a.m. UTC | #2
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
Jan Ekström Aug. 18, 2020, 7:44 p.m. UTC | #3
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 mbox series

Patch

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 "