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
> <mich...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
