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

[Marton Balint]

On Fri, 2 Aug 2019, Andreas Håkon wrote:

> Hi Marton,
>
>
>> > > > +
>> > > > +          /* 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.
>>
>
> Why you say that the condition (after the "if (program)") is executed exactly once?
> That statement is false! The inner **loop** (yes it's a loop) is executed:
>
> A) Just one time, only if no program is defined (that's the reason of the last
>   "if (!program) break)" plus the "program = program ? ... : NULL" and the
>   "while(program)" ).

Agreed.

> B) Just one time, if only one service is found for this stream.
> C) Multiple times, if the stream is inside multiple services.

I agree that if the outer (program) loop fires more than once if there are 
multiple programs, I meant that for each outer loop iteration the body of 
the inner loop is only executed once.

>
> So, because of (B) and (C) it's required to execute a loop. In fact, we iterate
> over all programs, except in one edge case: when no programs are defined. And only
> in this case the inner loop is executed once deterministically.
>
>
>> > > /* 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?
>
> To be frank too, I'm not responsible for the previous code.
> I think the original author created the code with the assumption that there was
> only one service. And after that, the support for multiple services was added.
> And now I have discovered this error, and I have fixed it with minimal modifications.
> Where's the problem, honestly?

It is natural that existing code needs to be cleaned up before adding a 
new feature.

>
>> 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.
>
> Sorry, too much work for me!

I will post a patch which does this, please test it and rebase your work 
upon it.

Thanks,
Marton