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

[Andreas Håkon]

Hi Marton,

First of all, a new version [v4] is posted here:
https://patchwork.ffmpeg.org/patch/14121/

However, I'll comment on your suggestions, as they are reasonable.


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 31 de July de 2019 23:58, Marton Balint <cus at passwd.hu> wrote:

> > +    for (i = 0; i < ts->nb_services; i++) {
> > +        service = ts->services[i];
> > +        service->pcr_st = NULL;
> > +    }
> > +
>
> This loop is not needed as far as I see. service is already initialized
> and service->pcr_st is zero initialized so you don't need to set it to
> NULL explicitly.

I prefer to initilize any variable. Futhermore, Andriy has commented to
do it inside the mpegts_add_service function. I prefer his approach.


> > +
> > +        /* program service checks */
> > +        program = av_find_program_from_stream(s, NULL, i);
> > +        do {
> > +            for (j = 0; j < ts->nb_services; j++) {
> > +                if (program) {
> > +                    /* search for the services corresponding to this program */
> > +                    if (ts->services[j]->program == program)
> > +                        service = ts->services[j];
> > +                    else
> > +                        continue;
> > +                }
> > +
> > +                ts_st->service = service;
> > +
> > +                /* check for PMT pid conflicts */
> > +                if (ts_st->pid == service->pmt.pid) {
> > +                    av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid);
> > +                    ret = AVERROR(EINVAL);
> > +                    goto fail;
> > +                }
> > +
> > +                /* update PCR pid by using the first video stream */
> > +                if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> > +                    service->pcr_pid == 0x1fff) {
> > +                    service->pcr_pid = ts_st->pid;
> > +                    service->pcr_st  = st;
> > +                }
> > +
> > +                /* store this stream as a candidate PCR if the service doesn't have any */
> > +                if (service->pcr_pid == 0x1fff &&
> > +                    !service->pcr_st) {
> > +                    service->pcr_st  = st;
> > +                }
> > +
> > +                /* exit when just one single service */
> > +                if (!program)
> > +                    break;
> > +            }
> > +            program = program ? av_find_program_from_stream(s, program, i) : NULL;
> > +        } while (program);
>
>
> Maybe I miss something but as far as I see only this block needs to be in
> the loop, the rest is not. So you can actually write this:
>

False! All checks require to be completed inside the loop.
Please, note that one stream (aka pid) can be assigned to multiple programs.
So we need to iterate over all programs and services.


> /* program service checks */
> program = av_find_program_from_stream(s, NULL, i);
> do {
> if (program) {
> for (j = 0; j < ts->nb_services; j++) {
>
>                      if (ts->services[j]->program == program) {
>
>                          service = ts->services[j];
>
>                          break;
>                      }
>                  }
>              }
>
>
> ts_st->service = service;
>
> /* check for PMT pid conflicts */
> ...
>
> This way you also ensure that ts_st->service is always set for every
> stream which is kind of how the old code worked, right?

Not really. The "service" can be assigned on two ways: or using the iteration
over all programs, or in the creation of a new service with the call
to the "mpegts_add_service()" function when no programs are defined.


>
> > -
> > -                  ts_st->service = service;
> >
> >
>
> You are setting this for all programs of the stream, so effectively the
> stream's service will be the last program (service) the stream is part of.
> Maybe you should aim for the first instead?

No. That's doesn't have sense. The objective of the code (old and new) is:

- Assign a pid for the PCR.
- Check for conflicts with the pid number.

To achieve this you need to compare with all PMTs. And futhermore, you need
to iterate over all services as the pid can be inside one or more of them.

And note this: the "ts_st->service = service" will be assigned to the
"last" service only when no programs are defined. So in this case only one
service exists, then first==last. In any other case (when one or programs
exist) any "ts_st->service = service" is assigned after the iteration over
the program list.

As a summary: the code is fine.


> > +
> > +                /* check for PMT pid conflicts */
> > +                if (ts_st->pid == service->pmt.pid) {
> > +                    av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid);
> > +                    ret = AVERROR(EINVAL);
> > +                    goto fail;
> > +                }
> > +
> > +                /* update PCR pid by using the first video stream */
> > +                if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> > +                    service->pcr_pid == 0x1fff) {
> > +                    service->pcr_pid = ts_st->pid;
> > +                    service->pcr_st  = st;
> > +                }
> > +
> > +                /* store this stream as a candidate PCR if the service doesn't have any */
> > +                if (service->pcr_pid == 0x1fff &&
> > +                    !service->pcr_st) {
> > +                    service->pcr_st  = st;
> > +                }
> > +
> > +                /* exit when just one single service */
> > +                if (!program)
> > +                    break;
>
> With the change above this check should be no longer needed.

As I have said this is not so.


> > +   if (!ts_st->service) {
> > +          av_log(s, AV_LOG_ERROR, "empty services!\\n");
> > +          ret = AVERROR(EINVAL);
> > +          goto fail_at_end;
> >     }
> >
>
> I guess this check will no longer be needed if ts_st->service is always
> set.

This block is like an ASSERT. It's only here to provent to continue without
empty services (impossible inside MPEG-TS). It's a simple check to avoid
extreme cases, which in the future could occur if changes are made to the code.


>
> Regards,
> Marton

Thank you for your comments!
Regards.
A.H.

---