diff mbox

[FFmpeg-devel] avformat/mpegts: skip subtitle PES packets if PCR not available

Message ID 20181215013144.21623-1-jeebjp@gmail.com
State New
Headers show

Commit Message

Jan Ekström Dec. 15, 2018, 1:31 a.m. UTC 
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.

In case someone actually wants to pass through all received packets,
the behavior can be controlled with an AVOption.
---
 libavformat/mpegts.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Carl Eugen Hoyos Dec. 15, 2018, 1:50 a.m. UTC | #1 
2018-12-15 2:31 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> 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.

For which sample does this make a difference?

Carl Eugen
Jan Ekström Dec. 15, 2018, 11:14 a.m. UTC | #2 
On Sat, Dec 15, 2018 at 3:50 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2018-12-15 2:31 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> > 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.
>
> For which sample does this make a difference?
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Hi,

This pops up unfortunately with various broadcast channels, but here's
one sample which shows it rather clearly:
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 - > with_skipping.json
2. ffprobe -skip_pes_packets_without_pcr none -of json -show_packets
-show_programs - > no_skipping.json
3. git diff --no-index no_skipping.json with_skipping.json

This shows that libavformat does receive the initial subtitle packet
with a timestamp that is wildly incorrect (due to the PID's mux being
bonkers). After PCR is received, the timestamp fixing logic that is
enabled by default kicks in, and the subtitle packets are matched
against the PCR (which is what many broadcasters just assume receivers
will do).

Technically I think waiting for PCR is mandatory for all tracks in
MPEG-TS, but since I don't generally see such discrepancies outside of
subtitle tracks I matched this up with the subtitle timestamp fixing
code for now, in order to see people's opinions on this.

Best regards,
Jan
Marton Balint Dec. 15, 2018, 2:57 p.m. UTC | #3 
On Sat, 15 Dec 2018, 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.
>
> In case someone actually wants to pass through all received packets,
> the behavior can be controlled with an AVOption.
> ---
> libavformat/mpegts.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)

If PCR is unknown, then it is better to return the packets with 
AV_NOPTS_VALUE pts and dts instead of skipping them entirely. Or is there 
a case which is not fixed by this approach?

IMHO you might even do this unconditionally as long as "fix_teletext_pts" 
option is set (which is the default).

Regards,
Marton
Jan Ekström Dec. 15, 2018, 3:13 p.m. UTC | #4 
On Sat, Dec 15, 2018 at 4:57 PM Marton Balint <cus@passwd.hu> wrote:
>
> On Sat, 15 Dec 2018, 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.
> >
> > In case someone actually wants to pass through all received packets,
> > the behavior can be controlled with an AVOption.
> > ---
> > libavformat/mpegts.c | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
>
> If PCR is unknown, then it is better to return the packets with
> AV_NOPTS_VALUE pts and dts instead of skipping them entirely. Or is there
> a case which is not fixed by this approach?
>
> IMHO you might even do this unconditionally as long as "fix_teletext_pts"
> option is set (which is the default).
>
> Regards,
> Marton

I did think about that, but I didn't remember the semantics of
AV_NOPTS_VALUE in various cases, and I hadn't tested it so the initial
idea was to just skip or buffer those packets (until you had PCR).
Latter sounded not too nice to implement with an unknown amount of
buffering occuring, so I chose this way for the initial patch.

I will have to give it a try and see how it goes.

Jan
Jan Ekström Dec. 15, 2018, 6:53 p.m. UTC | #5 
On Sat, Dec 15, 2018 at 5:13 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Sat, Dec 15, 2018 at 4:57 PM Marton Balint <cus@passwd.hu> wrote:
> >
> > On Sat, 15 Dec 2018, 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.
> > >
> > > In case someone actually wants to pass through all received packets,
> > > the behavior can be controlled with an AVOption.
> > > ---
> > > libavformat/mpegts.c | 22 ++++++++++++++++++++++
> > > 1 file changed, 22 insertions(+)
> >
> > If PCR is unknown, then it is better to return the packets with
> > AV_NOPTS_VALUE pts and dts instead of skipping them entirely. Or is there
> > a case which is not fixed by this approach?
> >
> > IMHO you might even do this unconditionally as long as "fix_teletext_pts"
> > option is set (which is the default).
> >
> > Regards,
> > Marton
>
> I did think about that, but I didn't remember the semantics of
> AV_NOPTS_VALUE in various cases, and I hadn't tested it so the initial
> idea was to just skip or buffer those packets (until you had PCR).
> Latter sounded not too nice to implement with an unknown amount of
> buffering occuring, so I chose this way for the initial patch.
>
> I will have to give it a try and see how it goes.
>
> Jan

Alright, this worked well enough so I posted a new patch that unsets
DTS/PTS from the subtitle packets by using AV_NOPTS_VALUE if PCR has
not yet been received.

Jan
diff mbox

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index edf6b5701d..50404e8272 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -59,6 +59,12 @@  enum MpegTSFilterType {
     MPEGTS_PCR,
 };
 
+enum MpegTSPESSkipMode {
+    MPEGTS_PES_SKIP_MODE_SUBTITLES = 0,
+    MPEGTS_PES_SKIP_MODE_NONE,
+    MPEGTS_PES_SKIP_MODE_MAX,
+};
+
 typedef struct MpegTSFilter MpegTSFilter;
 
 typedef int PESCallback (MpegTSFilter *f, const uint8_t *buf, int len,
@@ -150,6 +156,8 @@  struct MpegTSContext {
     int resync_size;
     int merge_pmt_versions;
 
+    enum MpegTSPESSkipMode pes_packet_skip_mode;
+
     /******************************************/
     /* private mpegts data */
     /* scan context */
@@ -182,6 +190,10 @@  static const AVOption options[] = {
      {.i64 = 0}, 0, 1, 0 },
     {"skip_clear", "skip clearing programs", offsetof(MpegTSContext, skip_clear), AV_OPT_TYPE_BOOL,
      {.i64 = 0}, 0, 1, 0 },
+    {"skip_pes_packets_without_pcr", "Skip PES packets without PCR matching to rule", offsetof(MpegTSContext, pes_packet_skip_mode), AV_OPT_TYPE_INT,
+     {.i64 = MPEGTS_PES_SKIP_MODE_SUBTITLES}, MPEGTS_PES_SKIP_MODE_SUBTITLES, MPEGTS_PES_SKIP_MODE_MAX - 1, AV_OPT_FLAG_DECODING_PARAM , "skip_pes_packets_without_pcr"},
+    { "subtitles", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MPEGTS_PES_SKIP_MODE_SUBTITLES }, INT_MIN, INT_MAX, AV_OPT_FLAG_DECODING_PARAM,  "skip_pes_packets_without_pcr" },
+    { "none",      NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MPEGTS_PES_SKIP_MODE_NONE      }, INT_MIN, INT_MAX, AV_OPT_FLAG_DECODING_PARAM,  "skip_pes_packets_without_pcr" },
     { NULL },
 };
 
@@ -1219,6 +1231,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 +1255,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 +1272,14 @@  skip:
                             }
                         }
                     }
+
+                    if (!pcr_found &&
+                        ts->pes_packet_skip_mode != MPEGTS_PES_SKIP_MODE_NONE) {
+                        av_log(pes->stream, AV_LOG_VERBOSE,
+                               "Skipping non-trustworthy PES packet for PID %d as PCR hasn't been received yet.\n",
+                               pes->pid);
+                        pes->state = MPEGTS_SKIP;
+                    }
                 }
             }
             break;