diff mbox series

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

Message ID 1603379867-6325-2-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2,1/3] avformat/mpegtsenc: use total_size instead of avio_tell() | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Lance Wang Oct. 22, 2020, 3:17 p.m. UTC 
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(+)

Comments

Marton Balint Oct. 25, 2020, 12:04 p.m. UTC | #1 
On Thu, 22 Oct 2020, 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 5c97d63..415a64e 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;

It is minor nitpicking, but can you rename this to first_dts_checked and 
reverse the logic? So this can be 0 by default (so it does not have to be 
explicitly initialized), and you set it to 1 after it is checked.

Thanks,
Marton

>     int64_t next_pcr;
>     int mux_rate; ///< set to 1 when VBR
>     int pes_payload_size;
> @@ -1134,6 +1135,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);
> 
> @@ -1688,6 +1690,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;
> +        }
> +
>         if (pts != AV_NOPTS_VALUE)
>             pts += delay;
>         if (dts != AV_NOPTS_VALUE)
> -- 
> 1.8.3.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lance Wang Oct. 25, 2020, 12:55 p.m. UTC | #2 
On Sun, Oct 25, 2020 at 01:04:11PM +0100, Marton Balint wrote:
> 
> 
> On Thu, 22 Oct 2020, 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 5c97d63..415a64e 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;
> 
> It is minor nitpicking, but can you rename this to first_dts_checked and
> reverse the logic? So this can be 0 by default (so it does not have to be
> explicitly initialized), and you set it to 1 after it is checked.

I'm OK for your suggestion for the name, but MpegTSWriteStream have defined a 
first_pts_check for every stream to check first pts for stream, do you think 
it's necessary to keep the same style?

> 
> Thanks,
> Marton
> 
> >     int64_t next_pcr;
> >     int mux_rate; ///< set to 1 when VBR
> >     int pes_payload_size;
> > @@ -1134,6 +1135,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);
> > 
> > @@ -1688,6 +1690,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;
> > +        }
> > +
> >         if (pts != AV_NOPTS_VALUE)
> >             pts += delay;
> >         if (dts != AV_NOPTS_VALUE)
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Oct. 25, 2020, 5:47 p.m. UTC | #3 
On Sun, 25 Oct 2020, lance.lmwang@gmail.com wrote:

> On Sun, Oct 25, 2020 at 01:04:11PM +0100, Marton Balint wrote:
>> 
>> 
>> On Thu, 22 Oct 2020, 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 5c97d63..415a64e 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;
>> 
>> It is minor nitpicking, but can you rename this to first_dts_checked and
>> reverse the logic? So this can be 0 by default (so it does not have to be
>> explicitly initialized), and you set it to 1 after it is checked.
>
> I'm OK for your suggestion for the name, but MpegTSWriteStream have defined a 
> first_pts_check for every stream to check first pts for stream, do you think 
> it's necessary to keep the same style?

Ok, I see what you mean. I'd rather change that to use reversed logic (and 
past tense) there later. And we should expand that to check if not only 
pts, but the first dts in every stream is also valid.

Regards,
Marton

>
>> 
>> Thanks,
>> Marton
>> 
>> >     int64_t next_pcr;
>> >     int mux_rate; ///< set to 1 when VBR
>> >     int pes_payload_size;
>> > @@ -1134,6 +1135,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);
>> > 
>> > @@ -1688,6 +1690,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;
>> > +        }
>> > +
>> >         if (pts != AV_NOPTS_VALUE)
>> >             pts += delay;
>> >         if (dts != AV_NOPTS_VALUE)
>> > -- 
>> > 1.8.3.1
>> > 
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > 
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> -- 
> Thanks,
> Limin Wang
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lance Wang Oct. 26, 2020, 1:16 a.m. UTC | #4 
On Sun, Oct 25, 2020 at 06:47:27PM +0100, Marton Balint wrote:
> 
> 
> On Sun, 25 Oct 2020, lance.lmwang@gmail.com wrote:
> 
> > On Sun, Oct 25, 2020 at 01:04:11PM +0100, Marton Balint wrote:
> > > 
> > > 
> > > On Thu, 22 Oct 2020, 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 5c97d63..415a64e 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;
> > > 
> > > It is minor nitpicking, but can you rename this to first_dts_checked and
> > > reverse the logic? So this can be 0 by default (so it does not have to be
> > > explicitly initialized), and you set it to 1 after it is checked.
> > 
> > I'm OK for your suggestion for the name, but MpegTSWriteStream have
> > defined a first_pts_check for every stream to check first pts for
> > stream, do you think it's necessary to keep the same style?
> 
> Ok, I see what you mean. I'd rather change that to use reversed logic (and
> past tense) there later. And we should expand that to check if not only pts,
> but the first dts in every stream is also valid.

thanks, will update the patch to fix the name to first_dts_checked and reverse
the logic.

> 
> Regards,
> Marton
> 
> > 
> > > 
> > > Thanks,
> > > Marton
> > > 
> > > >     int64_t next_pcr;
> > > >     int mux_rate; ///< set to 1 when VBR
> > > >     int pes_payload_size;
> > > > @@ -1134,6 +1135,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);
> > > > > @@ -1688,6 +1690,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;
> > > > +        }
> > > > +
> > > >         if (pts != AV_NOPTS_VALUE)
> > > >             pts += delay;
> > > >         if (dts != AV_NOPTS_VALUE)
> > > > -- > 1.8.3.1
> > > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> > -- 
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index 5c97d63..415a64e 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;
@@ -1134,6 +1135,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);
 
@@ -1688,6 +1690,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;
+        }
+
         if (pts != AV_NOPTS_VALUE)
             pts += delay;
         if (dts != AV_NOPTS_VALUE)