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

[Marton Balint]

On Thu, 1 Aug 2019, Andreas Håkon wrote:

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

I replied to the wrong (v3) mail, but I commented the v4 version.

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

You allocate the MpegTSService struct with av_mallocz therefore pcr_st it 
is already initialized to NULL, re-initializing it to NULL is pointless.

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

My problem is that the body of the inner loop, after the if (program) 
condition is executed exactly once. Therefore it does not belong to a 
loop.

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

To be frank, the whole ts_st->service is bogus, because a stream can 
belong to multiple services, so on what grounds do we select one?

I'd suggest to remove service from MpegTSWriteStream and move
pcr_packet_count and pcr_packet_period from MpegTSService to 
MpegTSWriteStream. Of course this should be yet another, separate patch, a 
prerequisite to this one.

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

If it cannot happen now then make it an av_assert. If it can happen after 
a later code change then change av_assert to "if" with that change.

Thanks,
Marton