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

[Andreas Håkon]

Hi Andriy,
I'm glad to read you!

------- Original Message -------
On Saturday, 27 de July de 2019 9:05, Andriy Gelman <andriy.gelman at gmail.com> wrote:

> > 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.
>
> The PCR pid selection is probably fine.
> The issue is that pcr_packet_period is zero for N-1 out of N programs.
> That's why in your tests you see the insertion of the pcr adaptation field in each
> frame for 1/2 programs.

Yes, that's the root cause.


> > +   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];
> >
>
> How do you know that s->streams[0] belongs to this particular program?

First of all, note that most of the inserted code are just copy&paste
of previous code. I have needed to move some blocks within a loop to
process the data for "every" program. That's the objective of this patch.

In any case, it's good to discuss about your comments.

In this particular case "pcr_st = s->streams[0];" it seems that you're right.
I have only checked with services using VIDEO streams. And analyzing the code
Now I see that this case fails when there's no video stream. Any ideas?


> > +              ts_st            = pcr_st->priv_data;
> > +              service->pcr_pid = ts_st->pid;
> > +          } else
> > +              ts_st = pcr_st->priv_data;
> >
>
> pcr_st stream is set outside the for loop. No reason that it's part of this program.

I'm going to take a closer look, I may have missed something. Thank you!


> > +          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;
> >
>
> These assignments do not change for each service/program. They should probably
> be outside of the for loop.

Well, the code uses a "if" block and I prefer not to touch the original code much.
Also, the code uses the var "ts->pcr_period", which must be initialized. And the
initialization is only guaranteed within this loop, so even if the code seems
redundant I prefer to leave it here. Any other reason to move it?


> > +            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);
> > +                } else {
> > +                    service->pcr_packet_period =
> > +                        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);
> >
> > pcr_packet_period may be incorrect here because ts_st may be coming from a stream
> that's not part of this program.

I don't agree with you on this. The repetition rate of PCR marks is based
on the TS structure, and not based on the service. That's an MPTS and not a SPTS.
Therefore, the values are computed using the bitrate of the TS, and are EQUAL for
all services within the TS. So, the value is correct regardless the service used.
In fact, if you calculate it for one service, you can copy it for the rest. So
don't care about this.

> Andriy
Thank you for your comments! I'll write more later. Stay tuned.

Regards.
A.H.

---