[FFmpeg-devel] [PATCH] avformat/mpegts: unset DTS/PTS for subtitle PES packets if PCR not available
Michael Niedermayer
michael at niedermayer.cc
Tue Dec 18 12:18:24 EET 2018
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 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).
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 :(
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181218/a48596f0/attachment.sig>
More information about the ffmpeg-devel
mailing list