[FFmpeg-devel] [PATCH] libavformat: fix copyts and muxrate in mpegts muxer

Andreas Håkon andreas.hakon at protonmail.com
Tue Apr 23 10:04:23 EEST 2019


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 23 de April de 2019 0:36, Michael Niedermayer <michael at niedermayer.cc> wrote:
>
> > From c59569ca9426fef455edabfa648cb2ff678c1640 Mon Sep 17 00:00:00 2001
> > From: Andreas Hakon andreas.hakon at protonmail.com
> > Date: Fri, 19 Apr 2019 09:32:33 +0100
> > Subject: [PATCH] libavformat: fix copyts and muxrate in mpegts muxer v2
> > When using "-copyts" and "-muxrate" with the mpegts muxer the muxing fails
> > because the member "first_pcr" of the MpegTSWrite struct isn't initialized
> > with a correct timestamp.
> > The behaviour of the error is an infinite loop created in the function
> > mpegts_write_pes() because the code never writes PES packets.
> > This patch resolves the problem initializing the "first_pcr" with a value
> > derived from the first DTS value seen.
> >
> > Signed-off-by: Andreas Hakon andreas.hakon at protonmail.com
> >
> > ----------------------------------------------------------
> >
> > libavformat/mpegtsenc.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> > index fc0ea22..858b0d7 100644
> > --- a/libavformat/mpegtsenc.c
> > +++ b/libavformat/mpegtsenc.c
> > @@ -971,6 +971,9 @@ static int mpegts_init(AVFormatContext *s)
> >
> >          if (ts->copyts < 1)
> >              ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
> > +        else
> > +            ts->first_pcr = AV_NOPTS_VALUE;
> > +   } else {
> >          /* Arbitrary values, PAT/PMT will also be written on video key frames */
> >          ts->sdt_packet_period = 200;
> > @@ -1186,12 +1189,16 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream st,
> >     int64_t pcr = -1; / avoid warning */int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE);
> >     int force_pat = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && key && !ts_st->prev_payload_key;
> > +   int last_payload_size = 0;
> >     av_assert0(ts_st->payload != buf || st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO);
> >     if (ts->flags & MPEGTS_FLAG_PAT_PMT_AT_FRAMES && st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) {
> >     force_pat = 1;
> >     }
> >
> > +   if (ts->mux_rate > 1 && dts != AV_NOPTS_VALUE && ts->first_pcr == AV_NOPTS_VALUE)
> > +       ts->first_pcr = (dts * 300) - av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
> > +
>
> if this doesnt execute first_pcr could remain AV_NOPTS_VALUE, this looks
> a bit fragile to me as there is code using first_pcr with implicit assumtation
> that it is not AV_NOPTS_VALUE in get_pcr().
> is something preventing this combination ?

After reading it, I don't see any reason to this assumption...
But, you need to understand that the function "get_pcr()" ***uses*** the "first_pcr" value.
Check it at: https://github.com/FFmpeg/FFmpeg/blob/694d9d53685333771823285457bdd1ef1480eafc/libavformat/mpegtsenc.c#L747

So, you can't use "get_pcr()" to calculate "first_pcr" or you'll go to a loop!

So the code is correct here.

>
> >      is_start = 1;
> >      while (payload_size > 0) {
> >          retransmit_si_info(s, force_pat, dts);
> >
>
> > @@ -1209,12 +1216,13 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> > }
> >
> >          if (ts->mux_rate > 1 && dts != AV_NOPTS_VALUE &&
> > -              (dts - get_pcr(ts, s->pb) / 300) > delay) {
> > +              last_payload_size != payload_size && (dts - get_pcr(ts, s->pb) / 300) > delay) {
> >                /* pcr insert gets priority over null packet insert */
> >                if (write_pcr)
> >                    mpegts_insert_pcr_only(s, st);
> >                else
> >                    mpegts_insert_null_packet(s);
> > +              last_payload_size = payload_size;
> >
> >
>
> why is the check on payload_size needed ? (it is not for the testcase)
> the commit message also seems not to explain this part

This is required because the loop needs to break (the cause of the infinite loop: the *bug* is here).

Let me to explain in more detail:

- The loop is created because the code enters in the block because (don't care about the rest):
"(dts - get_pcr(ts, s->pb) / 300) > delay)". And "get_pcr()" uses the "first_pcr". So, for this
reason we need to properly initialize it in the previous block. And this is the first part of the
solution for the bug.

- But the second part is how to break the loop. The code really needs to be executed if the
difference between the DTS and the PCR is greater than the fixed delay. However, nothing prevents to
re-enter in the code another time, generating the loop. The solution to break it is to check if
the "last_payload_size" is different from the current one. If this is true, then it's true that
something is writed in the stream. And in this case is then valid to re-enter in the block.
But in the opposite case when nothing is writen, then we don't need to enter in the block.
So, for this reason it's the "last_payload_size != payload_size" in the test of the if condition.

> thanks
>

As conclusion. The bug is here. And the patch solves it as a whole.
Please, commit.

Best!
A.H.

---



More information about the ffmpeg-devel mailing list