diff mbox series

[FFmpeg-devel] Fix first_pcr initial update

Message ID 4b9b1e3d-3d4e-a353-d037-b9d924c808d6@m1stereo.tv
State New
Headers show
Series [FFmpeg-devel] Fix first_pcr initial update
Related show

Checks

Context Check Description
andriy/commit_msg_x86 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/commit_msg_ppc warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Maksym Veremeyenko Nov. 2, 2021, 10:47 a.m. UTC
One of latest commit 
https://source.ffmpeg.org/?p=ffmpeg.git;a=commitdiff;h=6f36eb0da71d22aadf8f056f0966bd86656ea57e 
claim it fixes endless loop on package generation if muxrate specified 
and copyts used. But actually it does not work properly if 
*-mpegts_copyts 1* specified:

ffmpeg -y -copyts -i loewe.ts -c:v libx264 -x264opts 
nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k 
-bufsize 1000k  -c:a mp2 -f mpegts -mpegts_copyts 1 -muxrate 4500k 
-vframes 1000 test.ts

ffmpeg generate huge file until it reach zero-based pcr value equal to 
first dts.

attached patch fix it.

Comments

Limin Wang Nov. 2, 2021, 2:59 p.m. UTC | #1
On Tue, Nov 02, 2021 at 12:47:57PM +0200, Maksym Veremeyenko wrote:
> One of latest commit https://source.ffmpeg.org/?p=ffmpeg.git;a=commitdiff;h=6f36eb0da71d22aadf8f056f0966bd86656ea57e
> claim it fixes endless loop on package generation if muxrate specified and
> copyts used. But actually it does not work properly if *-mpegts_copyts 1*
> specified:
> 
> ffmpeg -y -copyts -i loewe.ts -c:v libx264 -x264opts nal-hrd=cbr:force-cfr=1
> -b:v 3500k -minrate 3500k -maxrate 3500k -bufsize 1000k  -c:a mp2 -f mpegts
> -mpegts_copyts 1 -muxrate 4500k -vframes 1000 test.ts
> 

> ffmpeg generate huge file until it reach zero-based pcr value equal to first
> dts.
> 
> attached patch fix it.
> 
> 
> -- 
> Maksym Veremeyenko

> From fff0339c6d764e18fc4ecf111d035095309dc4d4 Mon Sep 17 00:00:00 2001
> From: Maksym Veremeyenko <verem@m1.tv>
> Date: Tue, 2 Nov 2021 12:33:54 +0200
> Subject: [PATCH] Fix first_pcr initial update
> 
> ---
>  libavformat/mpegtsenc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index 35c835c..32786be 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -1693,17 +1693,17 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>          stream_id = side_data[0];
>  
>      if (ts->copyts < 1) {
> -        if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
> -            ts->first_pcr += dts * 300;
> -            ts->first_dts_checked = 1;
> -        }
> -
>          if (pts != AV_NOPTS_VALUE)
>              pts += delay;
>          if (dts != AV_NOPTS_VALUE)
>              dts += delay;
>      }
>  
> +    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
> +        ts->first_pcr += dts * 300;
> +        ts->first_dts_checked = 1;
> +    }
> +

I think it's not same as the old code, the first_pcr will add extra delay if copyts is
0.

>      if (!ts_st->first_timestamp_checked && (pts == AV_NOPTS_VALUE || dts == AV_NOPTS_VALUE)) {
>          av_log(s, AV_LOG_ERROR, "first pts and dts value must be set\n");
>          return AVERROR_INVALIDDATA;
> -- 
> 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".
Maksym Veremeyenko Nov. 2, 2021, 3:36 p.m. UTC | #2
On 02.11.2021 16:59, lance.lmwang@gmail.com wrote:
[...]
>> +    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
>> +        ts->first_pcr += dts * 300;
>> +        ts->first_dts_checked = 1;
>> +    }
>> +
> 
> I think it's not same as the old code, the first_pcr will add extra delay if copyts is
> 0.
> 

proposed patch extend updating ts->first_pcr not only when copyts is 0 
but when copyts is 1 (*-mpegts_copyts 1* specified)

very similar issue i found at 
http://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242766.html
Limin Wang Nov. 3, 2021, 9:19 a.m. UTC | #3
On Tue, Nov 02, 2021 at 05:36:37PM +0200, Maksym Veremeyenko wrote:
> On 02.11.2021 16:59, lance.lmwang@gmail.com wrote:
> [...]
> > > +    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
> > > +        ts->first_pcr += dts * 300;
> > > +        ts->first_dts_checked = 1;
> > > +    }
> > > +
> > 
> > I think it's not same as the old code, the first_pcr will add extra delay if copyts is
> > 0.
> > 
> 
> proposed patch extend updating ts->first_pcr not only when copyts is 0 but
> when copyts is 1 (*-mpegts_copyts 1* specified)
> 
> very similar issue i found at
> http://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242766.html
> 
I mean you need move the code before if (ts->copyts < 1) { instead of after.

try with below command:
 ./ffmpeg -y -copyts -i http://samples.ffmpeg.org/MPEG2/foxksaz.ts -c:v libx264 -x264opts nal-hrd=cbr:force-cfr=1 \
-b:v 3500k -minrate 3500k -maxrate 3500k -bufsize 1000k  -c:a mp2 -f mpegts \
-mpegts_copyts 0 -muxrate 4500k -vframes 1000 test.ts

You'll get some "dts < pcr, TS is invalid" message.


> -- 
> Maksym Veremeyenko
> 
> _______________________________________________
> 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".
Maksym Veremeyenko Nov. 3, 2021, 10:50 a.m. UTC | #4
On 03.11.2021 11:19, lance.lmwang@gmail.com wrote:
> On Tue, Nov 02, 2021 at 05:36:37PM +0200, Maksym Veremeyenko wrote:
>> On 02.11.2021 16:59, lance.lmwang@gmail.com wrote:
>> [...]
>>>> +    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
>>>> +        ts->first_pcr += dts * 300;
>>>> +        ts->first_dts_checked = 1;
>>>> +    }
>>>> +
>>>
>>> I think it's not same as the old code, the first_pcr will add extra delay if copyts is
>>> 0.
[...]
> 
> try with below command:
>   ./ffmpeg -y -copyts -i http://samples.ffmpeg.org/MPEG2/foxksaz.ts -c:v libx264 -x264opts nal-hrd=cbr:force-cfr=1 \
> -b:v 3500k -minrate 3500k -maxrate 3500k -bufsize 1000k  -c:a mp2 -f mpegts \
> -mpegts_copyts 0 -muxrate 4500k -vframes 1000 test.ts
> 
> You'll get some "dts < pcr, TS is invalid" message.
> 
> 

yes, you are correct, in my tests s->max_delay was always 0 so i did not 
payed attention on order.
Maksym Veremeyenko Nov. 3, 2021, 11:01 a.m. UTC | #5
On 02.11.2021 12:47, Maksym Veremeyenko wrote:
> One of latest commit 
> https://source.ffmpeg.org/?p=ffmpeg.git;a=commitdiff;h=6f36eb0da71d22aadf8f056f0966bd86656ea57e 
> claim it fixes endless loop on package generation if muxrate specified 
> and copyts used. But actually it does not work properly if 
> *-mpegts_copyts 1* specified:
> 
> ffmpeg -y -copyts -i loewe.ts -c:v libx264 -x264opts 
> nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k 
> -bufsize 1000k  -c:a mp2 -f mpegts -mpegts_copyts 1 -muxrate 4500k 
> -vframes 1000 test.ts
> 
> ffmpeg generate huge file until it reach zero-based pcr value equal to 
> first dts.
> 

updated patch attached
Limin Wang Nov. 3, 2021, 1:43 p.m. UTC | #6
On Wed, Nov 03, 2021 at 01:01:29PM +0200, Maksym Veremeyenko wrote:
> On 02.11.2021 12:47, Maksym Veremeyenko wrote:
> > One of latest commit https://source.ffmpeg.org/?p=ffmpeg.git;a=commitdiff;h=6f36eb0da71d22aadf8f056f0966bd86656ea57e
> > claim it fixes endless loop on package generation if muxrate specified
> > and copyts used. But actually it does not work properly if
> > *-mpegts_copyts 1* specified:
> > 
> > ffmpeg -y -copyts -i loewe.ts -c:v libx264 -x264opts
> > nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k
> > -bufsize 1000k  -c:a mp2 -f mpegts -mpegts_copyts 1 -muxrate 4500k
> > -vframes 1000 test.ts
> > 
> > ffmpeg generate huge file until it reach zero-based pcr value equal to
> > first dts.
> > 
> 
> updated patch attached
> 
> -- 
> Maksym Veremeyenko

> From 5d6265e84417d7169d9c225be7305c116c33042f Mon Sep 17 00:00:00 2001
> From: Maksym Veremeyenko <verem@m1.tv>
> Date: Wed, 3 Nov 2021 12:57:45 +0200
> Subject: [PATCH] Fix first_pcr initial update
> 
> ---
>  libavformat/mpegtsenc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index 35c835c..dd01628 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -1692,12 +1692,12 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>      if (side_data)
>          stream_id = side_data[0];
>  
> -    if (ts->copyts < 1) {
> -        if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
> -            ts->first_pcr += dts * 300;
> -            ts->first_dts_checked = 1;
> -        }
> +    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
> +        ts->first_pcr += dts * 300;
> +        ts->first_dts_checked = 1;
> +    }
>  
> +    if (ts->copyts < 1) {
>          if (pts != AV_NOPTS_VALUE)
>              pts += delay;
>          if (dts != AV_NOPTS_VALUE)
> -- 
> 1.8.3.1
> 

LGTM


> _______________________________________________
> 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 Nov. 6, 2021, 7:26 p.m. UTC | #7
On Wed, 3 Nov 2021, lance.lmwang@gmail.com wrote:

> On Wed, Nov 03, 2021 at 01:01:29PM +0200, Maksym Veremeyenko wrote:
>> On 02.11.2021 12:47, Maksym Veremeyenko wrote:
>> > One of latest commit https://source.ffmpeg.org/?p=ffmpeg.git;a=commitdiff;h=6f36eb0da71d22aadf8f056f0966bd86656ea57e
>> > claim it fixes endless loop on package generation if muxrate specified
>> > and copyts used. But actually it does not work properly if
>> > *-mpegts_copyts 1* specified:
>> > 
>> > ffmpeg -y -copyts -i loewe.ts -c:v libx264 -x264opts
>> > nal-hrd=cbr:force-cfr=1 -b:v 3500k -minrate 3500k -maxrate 3500k
>> > -bufsize 1000k  -c:a mp2 -f mpegts -mpegts_copyts 1 -muxrate 4500k
>> > -vframes 1000 test.ts
>> > 
>> > ffmpeg generate huge file until it reach zero-based pcr value equal to
>> > first dts.
>> > 
>> 
>> updated patch attached
>> 
>> -- 
>> Maksym Veremeyenko
>
>> From 5d6265e84417d7169d9c225be7305c116c33042f Mon Sep 17 00:00:00 2001
>> From: Maksym Veremeyenko <verem@m1.tv>
>> Date: Wed, 3 Nov 2021 12:57:45 +0200
>> Subject: [PATCH] Fix first_pcr initial update
>> 
>> ---
>>  libavformat/mpegtsenc.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
>> index 35c835c..dd01628 100644
>> --- a/libavformat/mpegtsenc.c
>> +++ b/libavformat/mpegtsenc.c
>> @@ -1692,12 +1692,12 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>>      if (side_data)
>>          stream_id = side_data[0];
>> 
>> -    if (ts->copyts < 1) {
>> -        if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
>> -            ts->first_pcr += dts * 300;
>> -            ts->first_dts_checked = 1;
>> -        }
>> +    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
>> +        ts->first_pcr += dts * 300;
>> +        ts->first_dts_checked = 1;
>> +    }
>> 
>> +    if (ts->copyts < 1) {
>>          if (pts != AV_NOPTS_VALUE)
>>              pts += delay;
>>          if (dts != AV_NOPTS_VALUE)
>> -- 
>> 1.8.3.1
>> 
>
> LGTM

Thanks, will apply.

Regards,
Marton
diff mbox series

Patch

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index 35c835c..32786be 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -1693,17 +1693,17 @@  static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
         stream_id = side_data[0];
 
     if (ts->copyts < 1) {
-        if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
-            ts->first_pcr += dts * 300;
-            ts->first_dts_checked = 1;
-        }
-
         if (pts != AV_NOPTS_VALUE)
             pts += delay;
         if (dts != AV_NOPTS_VALUE)
             dts += delay;
     }
 
+    if (!ts->first_dts_checked && dts != AV_NOPTS_VALUE) {
+        ts->first_pcr += dts * 300;
+        ts->first_dts_checked = 1;
+    }
+
     if (!ts_st->first_timestamp_checked && (pts == AV_NOPTS_VALUE || dts == AV_NOPTS_VALUE)) {
         av_log(s, AV_LOG_ERROR, "first pts and dts value must be set\n");
         return AVERROR_INVALIDDATA;