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.ha...@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.ha...@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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to