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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
