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

[Andreas Håkon]

Hi Marton,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 1 de August de 2019 23:09, Marton Balint <cus at passwd.hu> wrote:

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

OK, no problem!


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

OK. I'll remove it. I think is a good practice to initializa all vars, but
if you prefer it that way, I don't criticize it.


> > > > +
> > > > +          /* 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)" ).
B) Just one time, if only one service is found for this stream.
C) Multiple times, if the stream is inside multiple services.

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?

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


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

OK. I agree to change it to an "av_assert". Thank you for the tip!


> Thanks,
> Marton

I'll prepare a new version. Stay on hold.
Regards.
A.H.

---