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

[Andreas Håkon]

Hi Marton,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 2 de August de 2019 10:09, Marton Balint <cus at passwd.hu> wrote:

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

Sorry, I disagree:

- The outer (program) loop (the "do-while" block) iterates over every program
  associated to the stream.
- And the inner (service) loop iterates over every SERVICE associated to the stream.

And take note that your target are SERVICES, so you really need to process ALL
services, and that's the reason to not break when the first service it's found.


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

Yes. And my "added new feature" is the "interleaving mux":
https://patchwork.ffmpeg.org/patch/13745/

So, this patch is only a small part that solves a BUG (a serious bug).
IMHO I'm doing the best I can.


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

OK, I'll wait for your patch then.


> Thanks,
> Marton

You're welcome!
Regards.
A.H.

---