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

Michael Niedermayer michael at niedermayer.cc
Tue Apr 23 01:36:59 EEST 2019


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 at 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 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 ?


>      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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20190423/3459bd91/attachment.sig>


More information about the ffmpeg-devel mailing list