[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 16:15, Andriy Gelman <andriy.gelman at gmail.com> wrote:

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

Marton recommends eliminating the unnecessary initialization.


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

Thank you!

> 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 so too!
But, the cases are (in reverse order):

A) One stream in multiple services.
B) One stream in one service.
C) No programs, so just one service.

Because (A) and (B) we need to iterate over all services. Futhermore, for each stream we
need to search for a program associated to it. But, an extreme case still remains: (C);
when no program is defined. And only for this special case can the code be somewhat confusing.
I will try to add some comments.


> I think Marton or Michael should have the final say.

Sure! And the comments from Marton are nice.


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

Sorry?
In the "next" iteration of the while loop the "j" will return to start at 0,
so you don't iterate over ALL services in where this stream lives. The "j"
iterator requires to go over all services, and execute the inner block for
all cases. That's the core of this patch to solve the current bug!


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

OK. If you read the entire source file after applying the patch, you can see
that several checks have been made for the stream. I make some rearrangements
to separate PID number checks from SERVICE checks. So instead of a separated
function called from the outher loop of the function "mpegts_init()" I prefer
this approach of inline code.


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

OK.


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

Marton suggested that. I'll do it that way.


> Andriy

Thank you for your comments!
Regards.
A.H.

---