[FFmpeg-devel,2/3] avformat/mpegtsenc: make first_pcr sync with the first valid dts

From: Limin Wang <lance.lmwang@gmail.com>

now first_pts assume dts will start from zero, if it's not true(copyts is enable),
too many null packet will be inserted for cbr output.

Please test with below command, you'll get huge test.ts without the patch:
./ffmpeg -y -copyts -i ../fate-suite/mpegts/loewe.ts  -c:v libx264 -x264opts \
   nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k -bufsize \
   1000k  -c:a mp2 -muxrate 4500k  -vframes 1000 test.ts

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/mpegtsenc.c | 7 +++++++
 1 file changed, 7 insertions(+)

On Wed, Oct 21, 2020 at 9:48 AM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> now first_pts assume dts will start from zero, if it's not true(copyts is enable),
> too many null packet will be inserted for cbr output.
>
> Please test with below command, you'll get huge test.ts without the patch:
> ./ffmpeg -y -copyts -i ../fate-suite/mpegts/loewe.ts  -c:v libx264 -x264opts \
>    nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k -bufsize \
>    1000k  -c:a mp2 -muxrate 4500k  -vframes 1000 test.ts
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/mpegtsenc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index e05d0ce..baea3b4 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -81,6 +81,7 @@ typedef struct MpegTSWrite {
>      int64_t pat_period; /* PAT/PMT period in PCR time base */
>      int nb_services;
>      int64_t first_pcr;
> +    int first_dts_check;
>      int64_t next_pcr;
>      int mux_rate; ///< set to 1 when VBR
>      int pes_payload_size;
> @@ -1136,6 +1137,7 @@ static int mpegts_init(AVFormatContext *s)
>
>      if (ts->copyts < 1)
>          ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
> +    ts->first_dts_check = 1;
>
>      select_pcr_streams(s);
>
> @@ -1690,6 +1692,11 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>          stream_id = side_data[0];
>
>      if (ts->copyts < 1) {
> +        if (ts->first_dts_check && dts != AV_NOPTS_VALUE) {
> +            ts->first_pcr += dts * 300;
> +        }
> +        ts->first_dts_check = 0;
> +

Shouldn't the line "ts->first_dts_check = 0;" be inside the if()
statement? Otherwise if the very first packet has a DTS of
AV_NOPTS_VALUE then the first_pcr will never get incremented by the
dts (since after the first run the check for first_dts_check will
always fail).

FWIW, the basic change is definitely useful, as I've had a nearly
identical patch in my tree for some time:

https://github.com/LTNGlobal-opensource/FFmpeg-ltn/commit/b974e431f7de39401a04d2c6e9bbdcc21e2982da

Devin

On Wed, Oct 21, 2020 at 03:41:00PM -0400, Devin Heitmueller wrote:
> On Wed, Oct 21, 2020 at 9:48 AM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > now first_pts assume dts will start from zero, if it's not true(copyts is enable),
> > too many null packet will be inserted for cbr output.
> >
> > Please test with below command, you'll get huge test.ts without the patch:
> > ./ffmpeg -y -copyts -i ../fate-suite/mpegts/loewe.ts  -c:v libx264 -x264opts \
> >    nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k -bufsize \
> >    1000k  -c:a mp2 -muxrate 4500k  -vframes 1000 test.ts
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/mpegtsenc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> > index e05d0ce..baea3b4 100644
> > --- a/libavformat/mpegtsenc.c
> > +++ b/libavformat/mpegtsenc.c
> > @@ -81,6 +81,7 @@ typedef struct MpegTSWrite {
> >      int64_t pat_period; /* PAT/PMT period in PCR time base */
> >      int nb_services;
> >      int64_t first_pcr;
> > +    int first_dts_check;
> >      int64_t next_pcr;
> >      int mux_rate; ///< set to 1 when VBR
> >      int pes_payload_size;
> > @@ -1136,6 +1137,7 @@ static int mpegts_init(AVFormatContext *s)
> >
> >      if (ts->copyts < 1)
> >          ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
> > +    ts->first_dts_check = 1;
> >
> >      select_pcr_streams(s);
> >
> > @@ -1690,6 +1692,11 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
> >          stream_id = side_data[0];
> >
> >      if (ts->copyts < 1) {
> > +        if (ts->first_dts_check && dts != AV_NOPTS_VALUE) {
> > +            ts->first_pcr += dts * 300;
> > +        }
> > +        ts->first_dts_check = 0;
> > +
> 
> Shouldn't the line "ts->first_dts_check = 0;" be inside the if()
> statement? Otherwise if the very first packet has a DTS of
> AV_NOPTS_VALUE then the first_pcr will never get incremented by the
> dts (since after the first run the check for first_dts_check will
> always fail).

Yes, you're right, will fix it.

> 
> FWIW, the basic change is definitely useful, as I've had a nearly
> identical patch in my tree for some time:
> 
> https://github.com/LTNGlobal-opensource/FFmpeg-ltn/commit/b974e431f7de39401a04d2c6e9bbdcc21e2982da
> 

make fate will fail if I set first_pcr to AV_NOPTS_VALUE and add later like
your changes. So I add extra flag for the check.

> Devin
> 
> -- 
> Devin Heitmueller, Senior Software Engineer
> LTN Global Communications
> o: +1 (301) 363-1001
> w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com