[FFmpeg-devel] [PATCHv2 1/4] avformat/mpegtsenc: fix incorrect PCR selection with multiple programs

[Andreas Håkon]

Hi Marton,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

> v2: a video is stream is preferred if there are no programs, just like before
> the patch.

Thank you for your effort!
However, something is wrong with this patch regarding PCR intervals.

First, try this example (using the file shared by Michael
https://trac.ffmpeg.org/raw-attachment/ticket/3714/FFmpeg%20Sample_cut.ts ):


% ffmpeg -loglevel info -y -i Sample_cut.ts \
 -map '0:v:0' -c:v:0 copy \
 -map '0:v:0' -c:v:1 copy \
 -map '0:a:0' -c:a:0 copy \
 -pat_period 1 \
 -program st=0,1,2 \
 -program st=0,2 \
 -program st=1,2 \
 -program st=3 \
 -sdt_period 1 \
 this_was_less_than_2560000-marton.ts

Then you get:
[mpegts @ 0x3046880] service 1 using PCR in pid=256
[mpegts @ 0x3046880] service 2 using PCR in pid=256
[mpegts @ 0x3046880] service 3 using PCR in pid=257
[mpegts @ 0x3046880] service 4 using PCR in pid=258

Until here all seems correct.

However, if you analyze the PCR intervals:
- 256: PCR_count: 0x3 (3) => repetition rate: 0,299 seconds
- 257: PCR_count: 0x3 (3) => repetition rate: 0,305 seconds
- 258: PCR_count: 0x4 (4) => repetition rate: 0,139 seconds

(You can check it with the DVB Inspector tool or any other)

And regular repetition rate are 0,450 seconds (with ffmpeg).

So...

> @@ -57,8 +57,6 @@ typedef struct MpegTSService {
> uint8_t name[256];
> uint8_t provider_name[256];
> int pcr_pid;
>
> -   int pcr_packet_count;
> -   int pcr_packet_period;
>     AVProgram *program;
>     } MpegTSService;

Why you remove the "pcr_packet_period" from services?
I know that the value can be the same for all services inside the
same TS, but the PCR interval is per service, and not per TS.


> @@ -1015,8 +1032,7 @@ static int mpegts_init(AVFormatContext *s)
> else
> av_log(s, AV_LOG_VERBOSE, "muxrate %d, ", ts->mux_rate);
>      av_log(s, AV_LOG_VERBOSE,
> -             "pcr every %d pkts, sdt every %d, pat/pmt every %d pkts\\n",
> -             service->pcr_packet_period,
> +             "sdt every %d, pat/pmt every %d pkts\\n",
>               ts->sdt_packet_period, ts->pat_packet_period);

The information about the PCR interval is a must have. Please, don't remove it!


> @@ -1198,12 +1214,12 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> force_pat = 0;
> write_pcr = 0;
> -          if (ts_st->pid == ts_st->service->pcr_pid) {
> +          if (ts_st->pcr_packet_period) {
>                if (ts->mux_rate > 1 || is_start) // VBR pcr period is based on frames
> -                  ts_st->service->pcr_packet_count++;
> -              if (ts_st->service->pcr_packet_count >=
> -                  ts_st->service->pcr_packet_period) {
> -                  ts_st->service->pcr_packet_count = 0;
> +                  ts_st->pcr_packet_count++;
> +              if (ts_st->pcr_packet_count >=
> +                  ts_st->pcr_packet_period) {
> +                  ts_st->pcr_packet_count = 0;
>                }
>            }

In this way, with multiple services (the reason for this patch) you're generating a
Transport Stream with PCR streams sharing the interval period. That's a new serious BUG!

For each service it's one PCR. And the interval of each PCR for one stream is
computed from the last PCR of the same stream. It can't be calculated from the last
PCR of any other stream.

Please, fix this bug before merging this patch.


> @@ -1236,7 +1252,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> }
> if (key && is_start && pts != AV_NOPTS_VALUE) {
> // set Random Access for key frames
>
> -              if (ts_st->pid == ts_st->service->pcr_pid)
> +              if (ts_st->pcr_packet_period)
>                    write_pcr = 1;
>                set_af_flag(buf, 0x40);
>                q = get_ts_payload_start(buf);

Just the same problem.


Regards.
A.H.

---