| Message ID | lY1kaveQf3Z8jVa1ReVr4gvqKI-wbto1Z2BFtHkN4qWZvm86q-n51ryJfhelTUA9QSHKbVz98Ya-X2mgT1-kyFS9cFGvBTjIu_3nuofTSjA=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday, 19 de April de 2019 10:47, Andreas Håkon via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Thursday, 18 de April de 2019 11:01, Andreas Håkon via ffmpeg-devel ffmpeg-devel@ffmpeg.org wrote: > > > Hi, > > This patch resolves one very specific use case: > > > > - When you use the mpegts muxer; > > > > - And use the global parameter “-copyts”; > > > > - And use the parameter “-muxrate” for the mpegts muxer; > > > > - And use too the parameter “-mpegts_copyts”. > > The problem is created because the member “first_pcr” of the MpegTSWrite struct isn’t initialized with a correct timestamp (so when copying the timestamps the initial value is 0). And in this case an infinite loop is created because the code never writes PES packets when the “mux_rate” isn’t VBR (equals to 1). See the block that creates the loop here (note the "continue" command at end): > > https://github.com/FFmpeg/FFmpeg/blob/a0559fcd81f42f446c93357a943699f9d44eeb79/libavformat/mpegtsenc.c#L1211 > > So, this patch fixes the problem initializing the “first_pcr” with the first DTS value that comes when using the incoming timestamps. > > Regards. > > A.H. > > > > Hi, > > Here a new version of the patch. > > The "fist_pcr" value is now derived from DTS in a more consistent way. > > Regards, > A.H. > Hi Michael Niedermayer, Please review this patch. It resolves a **serious** bug when using "copyts" and "muxrate" with the mpegts muxer. Testcase: $ ffmpeg -loglevel debug \ -copyts -f mpegts -i 01c56b0dc1.ts \ -filter_complex "[i:0xfb]fps=fps=25[out]" \ -map "[out]" -c:v libx264 \ -f mpegts -muxrate 10M -mpegts_copyts 1 out.ts File "01c56b0dc1.ts" is "http://samples.ffmpeg.org/ts/01c56b0dc1.ts". Without this patch the ffmpeg enters in a INFINITE LOOP !!! With the patch all is processed as excepted. Please, apply it. Regards. A.H. ---
On Fri, Apr 19, 2019 at 08:47:34AM +0000, Andreas Håkon via ffmpeg-devel wrote: > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Thursday, 18 de April de 2019 11:01, Andreas Håkon via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > > > Hi, > > > > This patch resolves one very specific use case: > > > > - When you use the mpegts muxer; > > - And use the global parameter “-copyts”; > > - And use the parameter “-muxrate” for the mpegts muxer; > > - And use too the parameter “-mpegts_copyts”. > > > > The problem is created because the member “first_pcr” of the MpegTSWrite struct isn’t initialized with a correct timestamp (so when copying the timestamps the initial value is 0). And in this case an infinite loop is created because the code never writes PES packets when the “mux_rate” isn’t VBR (equals to 1). See the block that creates the loop here (note the "continue" command at end): > > https://github.com/FFmpeg/FFmpeg/blob/a0559fcd81f42f446c93357a943699f9d44eeb79/libavformat/mpegtsenc.c#L1211 > > > > So, this patch fixes the problem initializing the “first_pcr” with the first DTS value that comes when using the incoming timestamps. > > > > Regards. > > A.H. > > > > Hi, > > Here a new version of the patch. > > The "fist_pcr" value is now derived from DTS in a more consistent way. > > Regards, > A.H. > > > --- > > From c59569ca9426fef455edabfa648cb2ff678c1640 Mon Sep 17 00:00:00 2001 > From: Andreas Hakon <andreas.hakon@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@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 ? > 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 thanks [...]
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, 23 de April de 2019 0:36, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > From c59569ca9426fef455edabfa648cb2ff678c1640 Mon Sep 17 00:00:00 2001 > > From: Andreas Hakon andreas.hakon@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@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. ---
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); + 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; /* recalculate write_pcr and possibly retransmit si_info */ continue; }