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

[Andreas Håkon]

Hi Marton,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, 4 de August de 2019 22:25, Marton Balint <cus at passwd.hu> wrote:

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

Sorry, a lot of typos... corrected example:

[...]
 -program st=0:st=1:st=2 \
 -program st=0:st=2 \
 -program st=1:st=2 \
 -program st=2 \
[...]

The idea with this example is quite simple: Share identical streams over
multiple services to check the correctness of the process.

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

But this is right anyway!

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

Sorry, sorry! Another typo: 0,045 seconds is the regular value
from ffmpeg (without patches).

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

0,045 seconds

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

OK.

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

And this is wrong. For MPTS-CBR the PCR interval needs to be calculated
individually for each PCR pid and count all packets.
So, I agree with you.

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

OK. I'll try https://patchwork.ffmpeg.org/patch/14233/


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

OK.

[...]

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

That's right. That's right. I assumed that the wrong intervals are for this. Excuse me!


Regards.
A.H.

---