diff mbox

[FFmpeg-devel] Using copyts with mpegts output fails after 26, 5 hours

Message ID CAMig2e6EAkYTCMWPu=i-yd6Zo=yj2AnUKwmx_7B4tXxjoXG-xw@mail.gmail.com
State New
Headers show

Commit Message

Panagiotis Malakoudis May 2, 2019, 6:45 p.m. UTC
> On Thu, May 02, 2019 at 08:13:10PM +0300, Panagiotis Malakoudis wrote:
> > When outputing to mpegts format, using -copyts option makes program
> > fail after the oveflow of mpegts PCR happens at around 26 hours and 30
> > minutes. When not using -copyts, only a warning is reported.
> > In order to reproduce the issue, you first have to create a 27 hour
> > mpegts video.
> > I have described the problem in thorough at ticket
> > https://trac.ffmpeg.org/ticket/7876
> >
> > The issue is resolved if you use a patch created originally by Arut
> > (Roman Arutyunyan) at
> > https://github.com/arut/ffmpeg-patches/blob/master/mpegts-33bit
> >
> > I have converted the patch in order to apply with current git and
> > confirmed working fine, fixing the above mentioned issue. Original
> > author describes the patch as: "Implements smooth 33-bit mpeg-ts
> > timestamp overflow in mpegts muxer."
> >
> > Here it is:
> > --- libavformat/mpegts.c 2019-04-30 16:11:49.000000000 +0300
> > +++ libavformat/mpegts.c 2019-04-30 18:55:12.736428766 +0300
>
> doesnt apply with git am
> Applying: Using copyts with mpegts output fails after 26, 5 hours
> error: mpegts.c: does not exist in index
> Patch failed at 0001 Using copyts with mpegts output fails after 26, 5 hours
>

Sorry, it was a simple diff, not a git am compatible diff. Hope I made
it correctly this time:

From 5273daaf8e568fc2a2b9a77d91cb8a834b9e14e7 Mon Sep 17 00:00:00 2001
From: Panagiotis Malakoudis <malakudi@gmail.com>
Date: Thu, 2 May 2019 21:34:44 +0300
Subject: [PATCH] Implement smooth 33bit mpegts timestamp overflow in mpegts
 muxer

---
 libavformat/mpegts.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

     uint8_t *sd;
@@ -1005,8 +1028,8 @@ static int new_pes_packet(PESContext *pes, AVPacket *pkt)
         pkt->stream_index = pes->sub_st->index;
     else
         pkt->stream_index = pes->st->index;
-    pkt->pts = pes->pts;
-    pkt->dts = pes->dts;
+    pkt->pts = unroll_timestamp(pes->ts, pes->pts);
+    pkt->dts = unroll_timestamp(pes->ts, pes->dts);
     /* store position of first TS packet of this PES packet */
     pkt->pos   = pes->ts_packet_pos;
     pkt->flags = pes->flags;

Comments

Carl Eugen Hoyos May 2, 2019, 6:55 p.m. UTC | #1
Am Do., 2. Mai 2019 um 20:52 Uhr schrieb Panagiotis Malakoudis
<malakudi@gmail.com>:
>
> > On Thu, May 02, 2019 at 08:13:10PM +0300, Panagiotis Malakoudis wrote:

> > > --- libavformat/mpegts.c 2019-04-30 16:11:49.000000000 +0300
> > > +++ libavformat/mpegts.c 2019-04-30 18:55:12.736428766 +0300
> >
> > doesnt apply with git am
> > Applying: Using copyts with mpegts output fails after 26, 5 hours
> > error: mpegts.c: does not exist in index
> > Patch failed at 0001 Using copyts with mpegts output fails after 26, 5 hours
> >
>
> Sorry, it was a simple diff, not a git am compatible diff. Hope I made
> it correctly this time:

You have to attach it to avoid linebreaks.

Carl Eugen
Panagiotis Malakoudis May 2, 2019, 7:09 p.m. UTC | #2
OK, here it is again, attached.


Στις Πέμ, 2 Μαΐ 2019 στις 9:56 μ.μ., ο/η Carl Eugen Hoyos
<ceffmpeg@gmail.com> έγραψε:
>
> Am Do., 2. Mai 2019 um 20:52 Uhr schrieb Panagiotis Malakoudis
> <malakudi@gmail.com>:
> >
> > > On Thu, May 02, 2019 at 08:13:10PM +0300, Panagiotis Malakoudis wrote:
>
> > > > --- libavformat/mpegts.c 2019-04-30 16:11:49.000000000 +0300
> > > > +++ libavformat/mpegts.c 2019-04-30 18:55:12.736428766 +0300
> > >
> > > doesnt apply with git am
> > > Applying: Using copyts with mpegts output fails after 26, 5 hours
> > > error: mpegts.c: does not exist in index
> > > Patch failed at 0001 Using copyts with mpegts output fails after 26, 5 hours
> > >
> >
> > Sorry, it was a simple diff, not a git am compatible diff. Hope I made
> > it correctly this time:
>
> You have to attach it to avoid linebreaks.
>
> Carl Eugen
> _______________________________________________
> 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".
Michael Niedermayer May 3, 2019, 8:36 a.m. UTC | #3
On Thu, May 02, 2019 at 10:09:23PM +0300, Panagiotis Malakoudis wrote:
> OK, here it is again, attached.
> 
> 
> Στις Πέμ, 2 Μαΐ 2019 στις 9:56 μ.μ., ο/η Carl Eugen Hoyos
> <ceffmpeg@gmail.com> έγραψε:
> >
> > Am Do., 2. Mai 2019 um 20:52 Uhr schrieb Panagiotis Malakoudis
> > <malakudi@gmail.com>:
> > >
> > > > On Thu, May 02, 2019 at 08:13:10PM +0300, Panagiotis Malakoudis wrote:
> >
> > > > > --- libavformat/mpegts.c 2019-04-30 16:11:49.000000000 +0300
> > > > > +++ libavformat/mpegts.c 2019-04-30 18:55:12.736428766 +0300
> > > >
> > > > doesnt apply with git am
> > > > Applying: Using copyts with mpegts output fails after 26, 5 hours
> > > > error: mpegts.c: does not exist in index
> > > > Patch failed at 0001 Using copyts with mpegts output fails after 26, 5 hours
> > > >
> > >
> > > Sorry, it was a simple diff, not a git am compatible diff. Hope I made
> > > it correctly this time:
> >
> > You have to attach it to avoid linebreaks.
> >
> > Carl Eugen
> > _______________________________________________
> > 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".

>  mpegts.c |   27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 4bef9770af2125d8b3eb5f48bda0fab9e04c7b36  mpegts-33bit-git.patch
> From 5273daaf8e568fc2a2b9a77d91cb8a834b9e14e7 Mon Sep 17 00:00:00 2001
> From: Panagiotis Malakoudis <malakudi@gmail.com>
> Date: Thu, 2 May 2019 21:34:44 +0300
> Subject: [PATCH] Implement smooth 33bit mpegts timestamp overflow in mpegts
>  muxer
> 
> ---
>  libavformat/mpegts.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)

this will break timestamp handling
not sure what you are trying to do exactly but the way its done is wrong.
There is alot of code that depends on the timestamps from the demuxer
not being mangled like this.

also its the wrong place, timestamp discontinuities are not a thing limited
to mpegts. And there are already layers of attempts to try to do what this
seems to try. Simple rule, if you dont need to allocate memory proportional
to duration in some cases then your code is wrong. Also if you dont intercept
seeks chapters and so on, again the code would be incomplete.

If you want a specific testcase that this breaks. It breaks tbr values
amongth other things for
tickets/2186/multiple_programs.ts

thanks

[...]
Panagiotis Malakoudis May 3, 2019, 8:50 a.m. UTC | #4
Στις Παρ, 3 Μαΐ 2019 στις 11:36 π.μ., ο/η Michael Niedermayer
<michael@niedermayer.cc> έγραψε:
>
> this will break timestamp handling
> not sure what you are trying to do exactly but the way its done is wrong.
> There is alot of code that depends on the timestamps from the demuxer
> not being mangled like this.
>
> also its the wrong place, timestamp discontinuities are not a thing limited
> to mpegts. And there are already layers of attempts to try to do what this
> seems to try. Simple rule, if you dont need to allocate memory proportional
> to duration in some cases then your code is wrong. Also if you dont intercept
> seeks chapters and so on, again the code would be incomplete.
>
> If you want a specific testcase that this breaks. It breaks tbr values
> amongth other things for
> tickets/2186/multiple_programs.ts
>
> thanks

As I mentioned in ticket http://trac.ffmpeg.org/ticket/7876 this is
not my code, I just adapted it for current git. It is also very old
code (back from 2012), probably it fitted OK back then. This code
fixes this specific issue reported in the ticket. If it breaks other
things, then of course it can't be commited. Still there is a need for
a fix for the reported issue in the ticket, as it is you can't
transcode with -copyts above 26,5 hours, it fails after that.

>
> [...]
>
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.
> _______________________________________________
> 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".
Devin Heitmueller May 3, 2019, 1:23 p.m. UTC | #5
Hello Panagiotis,

> On May 3, 2019, at 4:50 AM, Panagiotis Malakoudis <malakudi@gmail.com> wrote:
> 
> As I mentioned in ticket http://trac.ffmpeg.org/ticket/7876 this is
> not my code, I just adapted it for current git. It is also very old
> code (back from 2012), probably it fitted OK back then. This code
> fixes this specific issue reported in the ticket. If it breaks other
> things, then of course it can't be commited. Still there is a need for
> a fix for the reported issue in the ticket, as it is you can't
> transcode with -copyts above 26,5 hours, it fails after that.
> 

For what it’s worth, this is a pretty difficult problem, and not one exclusive to -copyts.  I had to put a bunch of code in the demux to support the TS wraparound for use with the decklink output, which supports a 90KHz clock but the value is 64-bits, so the output doesn’t expect it to jump back to zero after hitting the 33-bit limit.

Also, at least in my case, the existing wraparound code that was there works exactly once - even in cases where it worked as expected it would properly handle the wraparound after 26.5 hours, but then fail after 53 hours.

The approach I took was to track the PTS values as they approached 2^33, and then have a separate wraparound count such that the values coming out of the demux keep incrementing past the 33-bit limit.  This works well for the TS output case as well, since the output will simply truncate the lower 33-bits and it will continue working as it was before.  But, as Michael suggested, it almost certainly breaks seeking.  While my use cases don’t rely on seeking since I’m doing 24x7 decoding of realtime TS streams, such prevents my patches from being accepted upstream as well.

I’ve got no easy answers to this one - having TS streams longer than 26.5 hours and expecting seeking to work properly seem like they are not compatible concepts.

Devin

---
Devin Heitmueller - LTN Global Communications
dheitmueller@ltnglobal.com
diff mbox

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 8a84e5cc19..b7f5d8ea4e 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -163,6 +163,9 @@  struct MpegTSContext {
     /** structure to keep track of Program->pids mapping */
     unsigned int nb_prg;
     struct Program *prg;
+
+    /** base timestamp for smooth rolling over 33bits */
+    int64_t base_ts;

     int8_t crc_validity[NB_PID_MAX];
     /** filters for various streams specified by PMT + for the PAT and PMT */
@@ -982,6 +985,26 @@  static void new_data_packet(const uint8_t
*buffer, int len, AVPacket *pkt)
     pkt->size = len;
 }

+static int64_t unroll_timestamp(MpegTSContext *ts, int64_t t)
+{
+    int64_t dt;
+
+    if (t == AV_NOPTS_VALUE)
+        return t;
+
+    if (!ts->base_ts)
+        ts->base_ts = t;
+
+    dt = (t - ts->base_ts) & 0x01ffffffffll;
+    if (dt & 0x0100000000ll) {
+        dt |= 0xffffffff00000000ll;
+    }
+
+    ts->base_ts += dt;
+
+    return ts->base_ts;
+}
+
 static int new_pes_packet(PESContext *pes, AVPacket *pkt)
 {