diff mbox

[FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v2]

Message ID 2QdF7N31CbHUlD9YkHYyMwBVkhYZHrxxbIY15xeqLr5hzz5bZRrCahnSvo_K3e20hhIYPbasWZgM-931n1yKAbo2Y7vP9n9rac1ZfFQmMQM=@protonmail.com
State Superseded
Headers show

Commit Message

Andreas Håkon July 27, 2019, 8:41 p.m. UTC 
Hi,

This newversion fixes all the problems commented in
https://patchwork.ffmpeg.org/patch/14036/

Now, it works with/without video streams.

Regards.
A.H.

---
From c9b295c49828f31bba3879de887c7b2ed0e2641c Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Sat, 27 Jul 2019 21:28:32 +0100
Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple
 programs [v2]

The MPEG-TS muxer has a serious bug related to the PCR pid selection.
This bug appears when more than one program is used. The root cause is because
the current code targets only one program when selecting the stream for the PCR.

This patch solves this problem and select correct streams for each program.

You can check it with this command:

$ ./ffmpeg -loglevel verbose -y -f data -i /dev/zero \
 -filter_complex "nullsrc=s=60x60,split=2[v0][v1],anullsrc[a]" \
 -map [v0] -c:v:0 rawvideo \
 -map [v1] -c:v:1 rawvideo \
 -map [a]  -c:a:0 pcm_s8 \
 -program st=0 -program st=1 -program st=2 -f mpegts out.ts

And you will see something like:

[mpegts @ 0x562f388cd800] service 1 using PCR in pid=256
[mpegts @ 0x562f388cd800] service 2 using PCR in pid=257
[mpegts @ 0x562f388cd800] service 3 using PCR in pid=258


Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
---
 libavformat/mpegtsenc.c |  111 ++++++++++++++++++++++++++---------------------
 1 file changed, 61 insertions(+), 50 deletions(-)

Comments

Michael Niedermayer July 28, 2019, 2:39 p.m. UTC | #1 
On Sat, Jul 27, 2019 at 08:41:02PM +0000, Andreas Håkon wrote:
> Hi,
> 
> This newversion fixes all the problems commented in
> https://patchwork.ffmpeg.org/patch/14036/
> 
> Now, it works with/without video streams.
> 
> Regards.
> A.H.
> 
> ---

> From c9b295c49828f31bba3879de887c7b2ed0e2641c Mon Sep 17 00:00:00 2001
> From: Andreas Hakon <andreas.hakon@protonmail.com>
> Date: Sat, 27 Jul 2019 21:28:32 +0100
> Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple
>  programs [v2]
> 
> The MPEG-TS muxer has a serious bug related to the PCR pid selection.
> This bug appears when more than one program is used. The root cause is because
> the current code targets only one program when selecting the stream for the PCR.
> 
> This patch solves this problem and select correct streams for each program.
> 
> You can check it with this command:
> 
> $ ./ffmpeg -loglevel verbose -y -f data -i /dev/zero \
>  -filter_complex "nullsrc=s=60x60,split=2[v0][v1],anullsrc[a]" \
>  -map [v0] -c:v:0 rawvideo \
>  -map [v1] -c:v:1 rawvideo \
>  -map [a]  -c:a:0 pcm_s8 \
>  -program st=0 -program st=1 -program st=2 -f mpegts out.ts
> 
> And you will see something like:
> 
> [mpegts @ 0x562f388cd800] service 1 using PCR in pid=256
> [mpegts @ 0x562f388cd800] service 2 using PCR in pid=257
> [mpegts @ 0x562f388cd800] service 3 using PCR in pid=258
> 
> 
> Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
> ---
>  libavformat/mpegtsenc.c |  111 ++++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 50 deletions(-)

This causes the stream from ticket/3714 to become noticably bigger

./ffmpeg -i 3714/FFmpeg\ Sample_cut.ts -vcodec copy -acodec copy -pat_period 1 -sdt_period 1 this_was_less_than_2560000.ts

-rw-r----- 1 michael michael 2700056 Jul 28 16:12 this_was_less_than_2560000.ts

before:
-rw-r----- 1 michael michael 2559808 Jul 28 16:13 this_was_less_than_2560000.ts

-rw-r----- 1 michael michael 2560000 Jun 11  2014 3714/FFmpeg Sample_cut.ts

sample should be here: https://trac.ffmpeg.org/raw-attachment/ticket/3714/FFmpeg%20Sample_cut.ts

[...]
Andreas Håkon July 28, 2019, 7:14 p.m. UTC | #2 
Hi Michael,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, 28 de July de 2019 16:39, Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Sat, Jul 27, 2019 at 08:41:02PM +0000, Andreas Håkon wrote:
>
> > Hi,
> > This newversion fixes all the problems commented in
> > https://patchwork.ffmpeg.org/patch/14036/
> > Now, it works with/without video streams.
> > Regards.
> > A.H.
>
> This causes the stream from ticket/3714 to become noticably bigger
>
> ./ffmpeg -i 3714/FFmpeg\ Sample_cut.ts -vcodec copy -acodec copy -pat_period 1 -sdt_period 1 this_was_less_than_2560000.ts
>
> -rw-r----- 1 michael michael 2700056 Jul 28 16:12 this_was_less_than_2560000.ts
>
> before:
> -rw-r----- 1 michael michael 2559808 Jul 28 16:13 this_was_less_than_2560000.ts
> -rw-r----- 1 michael michael 2560000 Jun 11 2014 3714/FFmpeg Sample_cut.ts
>
> sample should be here: https://trac.ffmpeg.org/raw-attachment/ticket/3714/FFmpeg Sample_cut.ts
>
> [...]

OMG!
Thank you Michael for finding this mistake... I missed a previous code block.

A new fixed version is here:
https://patchwork.ffmpeg.org/patch/14109/

And this new version pass the check with the file "FFmpeg Sample_cut.ts". Identical file size output.

Regards.
A.H.

---
diff mbox

Patch

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index fc0ea22..6cd7d11 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -60,6 +60,7 @@  typedef struct MpegTSService {
     int pcr_packet_count;
     int pcr_packet_period;
     AVProgram *program;
+    AVStream *pcr_st;
 } MpegTSService;
 
 // service_type values as defined in ETSI 300 468
@@ -774,7 +775,7 @@  static int mpegts_init(AVFormatContext *s)
     MpegTSWrite *ts = s->priv_data;
     MpegTSWriteStream *ts_st;
     MpegTSService *service;
-    AVStream *st, *pcr_st = NULL;
+    AVStream *st;
     AVDictionaryEntry *title, *provider;
     int i, j;
     const char *service_name;
@@ -831,6 +832,11 @@  static int mpegts_init(AVFormatContext *s)
         }
     }
 
+    for (i = 0; i < ts->nb_services; i++) {
+        service = ts->services[i];
+        service->pcr_st = NULL;
+    }
+
     ts->pat.pid          = PAT_PID;
     /* Initialize at 15 so that it wraps and is equal to 0 for the
      * first packet we write. */
@@ -917,7 +923,12 @@  static int mpegts_init(AVFormatContext *s)
         if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
             service->pcr_pid == 0x1fff) {
             service->pcr_pid = ts_st->pid;
-            pcr_st           = st;
+            service->pcr_st  = st;
+        }
+        /* store this stream as a candidate PCR if the service doesn't have any */
+        if (service->pcr_pid == 0x1fff &&
+            !service->pcr_st) {
+            service->pcr_st  = st;
         }
         if (st->codecpar->codec_id == AV_CODEC_ID_AAC &&
             st->codecpar->extradata_size > 0) {
@@ -951,62 +962,62 @@  static int mpegts_init(AVFormatContext *s)
         }
     }
 
-    av_freep(&pids);
+    for (i = 0; i < ts->nb_services; i++) {
+        service = ts->services[i];
 
-    /* if no video stream, use the first stream as PCR */
-    if (service->pcr_pid == 0x1fff && s->nb_streams > 0) {
-        pcr_st           = s->streams[0];
-        ts_st            = pcr_st->priv_data;
-        service->pcr_pid = ts_st->pid;
-    } else
-        ts_st = pcr_st->priv_data;
-
-    if (ts->mux_rate > 1) {
-        service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period /
-                                     (TS_PACKET_SIZE * 8 * 1000);
-        ts->sdt_packet_period      = (int64_t)ts->mux_rate * SDT_RETRANS_TIME /
-                                     (TS_PACKET_SIZE * 8 * 1000);
-        ts->pat_packet_period      = (int64_t)ts->mux_rate * PAT_RETRANS_TIME /
-                                     (TS_PACKET_SIZE * 8 * 1000);
-
-        if (ts->copyts < 1)
-            ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
-    } else {
-        /* Arbitrary values, PAT/PMT will also be written on video key frames */
-        ts->sdt_packet_period = 200;
-        ts->pat_packet_period = 40;
-        if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
-            int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0);
-            if (!frame_size) {
-                av_log(s, AV_LOG_WARNING, "frame size not set\n");
-                service->pcr_packet_period =
-                    pcr_st->codecpar->sample_rate / (10 * 512);
+        /* get the PCR stream (real or candidate) */
+        if (!service->pcr_st) {
+            av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i", service->sid);
+            ret = AVERROR(EINVAL);
+            goto fail;
+        }
+        ts_st = service->pcr_st->priv_data;
+
+        /* if no video stream, use the pid of the candidate PCR */
+        if (service->pcr_pid == 0x1fff)
+            service->pcr_pid = ts_st->pid;
+
+        if (ts->mux_rate > 1) {
+            service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period /
+                                        (TS_PACKET_SIZE * 8 * 1000);
+            ts->sdt_packet_period      = (int64_t)ts->mux_rate * SDT_RETRANS_TIME /
+                                        (TS_PACKET_SIZE * 8 * 1000);
+            ts->pat_packet_period      = (int64_t)ts->mux_rate * PAT_RETRANS_TIME /
+                                        (TS_PACKET_SIZE * 8 * 1000);
+            if (ts->copyts < 1)
+                ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
+        } else {
+            /* Arbitrary values, PAT/PMT will also be written on video key frames */
+            ts->sdt_packet_period = 200;
+            ts->pat_packet_period = 40;
+            if (service->pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
+                int frame_size = av_get_audio_frame_duration2(service->pcr_st->codecpar, 0);
+                if (!frame_size) {
+                    av_log(s, AV_LOG_WARNING, "frame size not set\n");
+                    service->pcr_packet_period =
+                        service->pcr_st->codecpar->sample_rate / (10 * 512);
+                } else {
+                    service->pcr_packet_period =
+                        service->pcr_st->codecpar->sample_rate / (10 * frame_size);
+                }
             } else {
+                // max delta PCR 0.1s
+                // TODO: should be avg_frame_rate
                 service->pcr_packet_period =
-                    pcr_st->codecpar->sample_rate / (10 * frame_size);
+                    ts_st->user_tb.den / (10 * ts_st->user_tb.num);
             }
-        } else {
-            // max delta PCR 0.1s
-            // TODO: should be avg_frame_rate
-            service->pcr_packet_period =
-                ts_st->user_tb.den / (10 * ts_st->user_tb.num);
+            if (!service->pcr_packet_period)
+                service->pcr_packet_period = 1;
         }
-        if (!service->pcr_packet_period)
-            service->pcr_packet_period = 1;
-    }
 
-    ts->last_pat_ts = AV_NOPTS_VALUE;
-    ts->last_sdt_ts = AV_NOPTS_VALUE;
-    // The user specified a period, use only it
-    if (ts->pat_period < INT_MAX/2) {
-        ts->pat_packet_period = INT_MAX;
-    }
-    if (ts->sdt_period < INT_MAX/2) {
-        ts->sdt_packet_period = INT_MAX;
+        // output a PCR as soon as possible
+        service->pcr_packet_count = service->pcr_packet_period;
+
+        av_log(s, AV_LOG_VERBOSE, "service %i using PCR in pid=%i\n", service->sid, service->pcr_pid);
     }
 
-    // output a PCR as soon as possible
-    service->pcr_packet_count = service->pcr_packet_period;
+    av_freep(&pids);
+
     ts->pat_packet_count      = ts->pat_packet_period - 1;
     ts->sdt_packet_count      = ts->sdt_packet_period - 1;