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

[Andreas Håkon]

Hi Andriy,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 1 de August de 2019 0:23, Andriy Gelman <andriy.gelman at gmail.com> wrote:

> > +    for (i = 0; i < ts->nb_services; i++) {
> > +        service = ts->services[i];
> > +        service->pcr_st = NULL;
> > +    }
> > +
>
> If you are going to add pcr_st to MpegTSService then it would make sense to move the
> initialization to mpegts_add_service function.

Good idea!


> > +
> > +        /* 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);
>
> This may be easier to digest if you separate the cases when
> s->nb_programs == 0 and s->nb_programs > 0. Maybe something like this could
> work:

Well, the code works, right?
So let me to comment some things previous to discuss your suggestion:

1. I prefer to leave the code close to the original one. If someone needs to do
   more changes, it's preferable to do small changes. Don't you think so?

2. The difference between "s->nb_programs == 0" and "s->nb_programs > 0" is because
   the old code. From my point of view, the special case when "s->nb_programs == 0"
   is a little sloppy. However, I need to consider it, so for this reason the code
   handles it.

> /*update service when no programs defined. use default service */
> if (s->nb_programs == 0) {
>
>     ret = update_service_and_set_pcr(s, st, service);
>     if (ret < 0)
>         goto fail:
>
>
> }
>
> /*find program for i-th stream */
> program = av_find_program_from_stream(s, NULL, i);
> while (s->nb_programs > 0 && program) {
>
> /*find the service that this program belongs to */
> for (j = 0; j < ts->nb_services; j++) {
>
>         if (ts->services[j]->program == program) {
>
>             service = ts->services[j];
>
>             break;
>         }
>     }
>

No, no! The loop requires to complete it for all services!
Every service requires a PCR, and the PCR can be inside a
shared stream. So if we break for the first service, then
other services will not be processed.
Please, see that the code is inside a loop and uses a continue
when the program doesn't match.

>
> ret = update_service_and_set_pcr(s, st, service);
> if (ret < 0)
> goto fail:
>
> /*find next program that this stream belongs to */
> program = av_find_program_from_stream(s, program, i);
> }
>
> /*we need to define the function update_service_and_set_pcr */
> static int update_service_and_set_pcr(AVFormatContext *s, AVStream *st, MpegTSService *service)
> {
>
> MpegTSWriteStream *ts_st = st->priv_data;
>
>     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);
>
>         return AVERROR(EINVAL);
>     }
>
>
> /* 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  = st;
>
>
> return 0;
> }

As commented before IMHO the code is more clear if we leave the service
checks here and not in a new function. When others update the code will
see that "all" checks requires to be completed here.


> +    for (i = 0; i < ts->nb_services; i++) {
> +        service = ts->services[i];
> +
> +        if (!service->pcr_st) {
> +            av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i\n", service->sid);
> +            ret = AVERROR(EINVAL);
> +            goto fail_at_end;
>
> fail_at_end is not deallocating any memory.
> Just return AVERROR(EINVAL) instead of goto would be fine.

Well, in the future perhaps some other change will require to do more processing
at end. So, I prefer to exit using the same behaviour as the rest of the code.
It's good coding practice, so I won't change it.


> +    if (!ts_st->service) {
> +        av_log(s, AV_LOG_ERROR, "empty services!\n");
> +        ret = AVERROR(EINVAL);
> +        goto fail_at_end;
>      }
>
> This is outside the stream loop, so why the random check?

This block is like an ASSERT. The code NEVER needs to continue from this
point if no service is created for the Transport Stream. So it checks it.
It's an insurance policy for future changes.


> >
> > fail:
> > av_freep(&pids);
>
> > +fail_at_end:
> > return ret;
>
> I don't think you need this goto option.

As I've already said, I'd rather leave it this way.


Regards.
A.H.

---