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.
However, I'll comment on your suggestions, as they are reasonable.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 31 de July de 2019 23:58, Marton Balint <c...@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.
> +
> + /* 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.
/* 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?
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.
> -
> - ts_st->service = service;
>
>
You are setting this for all programs of the stream, so effectively the
stream's service will be the last program (service) the stream is part of.
Maybe you should aim for the first instead?
No. That's doesn't have sense. The objective of the code (old and new) is:
- Assign a pid for the PCR.
- Check for conflicts with the pid number.
To achieve this you need to compare with all PMTs. And futhermore, you need
to iterate over all services as the pid can be inside one or more of them.
And note this: the "ts_st->service = service" will be assigned to the
"last" service only when no programs are defined. So in this case only one
service exists, then first==last. In any other case (when one or programs
exist) any "ts_st->service = service" is assigned after the iteration over
the program list.
As a summary: the code is fine.
> +
> + /* 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;
With the change above this check should be no longer needed.
As I have said this is not so.
> + 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.
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".
