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

[Marton Balint]

On Sun, 4 Aug 2019, Andreas Håkon wrote:

> 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

This command line does not seem correct, there is no stream 3, also in 
order to specify multiple streams per program you have to use this syntax: 
-program st=0:st=2.

>
> 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).

450m PCR seems wrong. ISO/IEC13818-1 specifies 
an interval of 100 ms, while DVB recommends 40 ms PCR-s.

What was the PCR interval before the patch? 450 ms seems like the 
keyframe interval of your video, because for keyframes you always get 
a PCR with the current code.

Keep in mind that according to this
https://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/192040.html
PCR in VBR streams never worked correctly.

CBR with multiple programs was also problematic, because instead of 
counting all packets the counter only counted the packets of a service.

The way I see it PCR generation needs a serious rework, I will post a 
patch for that, you will have to apply it on top of this series.

>
> 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.

It is per PID now instead of per service.

>
>
>> @@ -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!

It is written out in select_pcr_streams.

>
>
>> @@ -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!

No, ts_st is different for each stream (each PID).

>
> 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.

ts_st is different for each stream, it is not common.

>
> 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,
Marton