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

[Andriy Gelman]

Andreas,

On Thu, 01. Aug 08:24, Andreas Håkon wrote:
> 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.

Yes, I believe the code works. 
But, readability is also very important. Think about someone who is not familiar
with the code. It's tough to see straightaway how the two cases s->nb_programs == 0 and
and s->nb_programs > 0 are handled differently. Also, I believe it's easier for bugs to be
appear if someone modifies this code. 

I think Marton or Michael should have the final say. 

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

The break refers to the "for" loop and not the "while" loop. 
You'll find the next service in the next iteration of the while loop.

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

I suppose the checks could be outside the function. 

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

I did a grep for goto in ffmpeg. I cannot see any cases where there is a branch to
simply return the error. We should probably be consistent with the rest of the
code. 

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

The check is outside of the services loop. ts_st can belong to any of the
streams. Did you mean for the check to be inside the loop? 

If it's an assert, then an appropriate assert function would be better? 

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

Andriy