[FFmpeg-devel] [PATCH] avformat/mpegts: unset DTS/PTS for subtitle PES packets if PCR not available

[Jan Ekström]

On Mon, Dec 17, 2018 at 8:26 PM Michael Niedermayer
<michael at 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