[FFmpeg-devel] [PATCH] avformat/mpegtsenc: fix PCR generation intervals

[Marton Balint]

On Mon, 5 Aug 2019, Andreas Håkon wrote:

> Hi Marton,
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Sunday, 4 de August de 2019 22:30, Marton Balint <cus at passwd.hu> wrote:
>
>> PCR generation was based on counting packets for both CBR and VBR streams.
>> Couting packets might have worked for CBR streams (when muxrate was specified)
>> but it only took into account the packets of a service (or the packets of the
>> PCR stream lately), so even that was problematic for multi program streams.
>>
>> The new code works on actual PCR for CBR and packet DTS values for VBR streams,
>> so the default 20ms PCR retransmission time is now respected for both CBR and
>> VBR.
>>
>
> I do some tests with this patch and the previous https://patchwork.ffmpeg.org/patch/14210/
>
> And the result is that the repetition rate of PCR is inconsistent: it's never
> constant and varies between different PCR streams.

Are you testing CRB or VBR? For VBR you get PCR at the start of every 
packet with the command line you provided, so PCR interval should be 
constant. DVB Inspector always shows the streams as if they were CBR, so 
the PCR/PTS/DTS view is not showing the intervals properly for VBR.

>> +        int64_t pcr = -1; /* avoid warning */
>> +
>>          retransmit_si_info(s, force_pat, dts);
>>          force_pat = 0;
>>
>>          write_pcr = 0;
>> -        if (ts_st->pcr_packet_period) {
>> -            if (ts->mux_rate > 1 || is_start) // VBR pcr period is based on frames
>> -                ts_st->pcr_packet_count++;
>> -            if (ts_st->pcr_packet_count >=
>> -                ts_st->pcr_packet_period) {
>> -                ts_st->pcr_packet_count = 0;
>> -                write_pcr = 1;
>> +        if (ts_st->pcr_period) {
>> +            if (ts->mux_rate > 1) {
>> +                pcr = get_pcr(ts, s->pb);
>> +                if (pcr - ts_st->last_pcr >= ts_st->pcr_period)
>> +                    write_pcr = 1;
>> +            } else if (dts != AV_NOPTS_VALUE) {
>> +                pcr = (dts - delay) * 300;
>> +                if (pcr - ts_st->last_pcr >= ts_st->pcr_period && is_start)
>> +                    write_pcr = 1;
>>              }
>> +            if (write_pcr)
>> +                ts_st->last_pcr = FFMAX(pcr - ts_st->pcr_period, ts_st->last_pcr + ts_st->pcr_period);
>>          }
>
> IMHO, here you return to make the same mistake of the previous code:
> only consider one PCR stream. Instead of the "if (ts_st->pcr_period)" it's
> required to iterate over all PCR streams and do the corresponding checks.
> And if some PCR stream has exceeded the pcr_period limit then enforce to
> write an empty TS packet with a PCR mark.

For CBR, I agree, even if the extra PCR packets increase the bitrate 
slightly. I can try implementing this.

I am not sure what are we trying to achieve for VBR though. Since you 
don't know the bitrate you can't sensibly calculate PCR for non-start 
packets. So I believe for VBR the code works as it is supposed to.

Regards,
Marton