Re: [FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v4]
Hi Andriy,
‐‐‐ Original Message ‐‐‐
On Thursday, 1 de August de 2019 16:15, Andriy Gelman
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
Re: [FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v4]
Andreas,
On Thu, 01. Aug 08:24, Andreas Håkon wrote:
> Hi Andriy,
>
>
> ‐‐‐ Original Message ‐‐‐
> On Thursday, 1 de August de 2019 0:23, Andriy Gelman
> wrote:
>
> > > +for (i = 0; i < ts->nb_services; i++) {
> > > +service = ts->services[i];
> > > +service->pcr_st = NULL;
> > > +}
> > > +
> >
> > 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!
>
>
> > > +
> > > +/* 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);
> >
> > 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.
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 Marton or Michael should have the final say.
>
> > /*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.
>
> >
> > ret = update_service_and_set_pcr(s, st, service);
> > if (ret < 0)
> > goto fail:
> >
> > /*find next program that this stream belongs to */
> > program = av_find_program_from_stream(s, program, i);
> > }
> >
> > /*we need to define the function update_service_and_set_pcr */
> > static int update_service_and_set_pcr(AVFormatContext *s, AVStream *st,
> > MpegTSService *service)
> > {
> >
> >
Re: [FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v4]
Hi Andriy,
‐‐‐ Original Message ‐‐‐
On Thursday, 1 de August de 2019 0:23, Andriy Gelman
wrote:
> > +for (i = 0; i < ts->nb_services; i++) {
> > +service = ts->services[i];
> > +service->pcr_st = NULL;
> > +}
> > +
>
> 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!
> > +
> > +/* 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);
>
> 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.
> /*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.
>
> ret = update_service_and_set_pcr(s, st, service);
> if (ret < 0)
> goto fail:
>
> /*find next program that this stream belongs to */
> program = av_find_program_from_stream(s, program, i);
> }
>
> /*we need to define the function update_service_and_set_pcr */
> static int update_service_and_set_pcr(AVFormatContext *s, AVStream *st,
> MpegTSService *service)
> {
>
> MpegTSWriteStream *ts_st = st->priv_data;
>
> 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);
>
> return AVERROR(EINVAL);
> }
>
>
> /* 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 = st;
>
>
> return 0;
> }
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"
Re: [FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v4]
Andreas,
On Mon, 29. Jul 11:11, Andreas Håkon wrote:
> Hi,
>
> An updated version that fixes all previous bugs:
> https://patchwork.ffmpeg.org/patch/14109/
> https://patchwork.ffmpeg.org/patch/14099/
> https://patchwork.ffmpeg.org/patch/14036/
>
> It passes the tests pointed by Michael Niedermayer (Sample_cut.ts) and
> Andriy Gelman (day_flight.mpg).
>
> I hope this time the patch will be accepted.
> Regards.
> A.H.
>
> ---
> From 8381febd0e881cfcd53583b0ccdd7eb2c580e422 Mon Sep 17 00:00:00 2001
> From: Andreas Hakon
> Date: Mon, 29 Jul 2019 12:02:06 +0100
> Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple
> programs [v4]
>
> The MPEG-TS muxer has a serious bug related to the use of multiple programs:
> in that case, the PCR pid selection is incomplete for all services except one.
> This patch solves this problem and select correct streams for each program.
> Furthermore, it passes all the checks and doesn't generate any regression.
> Also fully compatible with shared pids over services.
>
> You can check it with this example command:
>
> $ ./ffmpeg -loglevel verbose -y -f data -i /dev/zero \
> -filter_complex "nullsrc=s=60x60,split=2[v0][v1],anullsrc[a]" \
> -map [v0] -c:v:0 rawvideo \
> -map [v1] -c:v:1 rawvideo \
> -map [a] -c:a:0 pcm_s8 \
> -program st=0,2 -program st=1,2 -program st=2 -program st=0 -f mpegts out.ts
>
> And you will see something like:
>
> [mpegts @ 0x55f8452797c0] service 1 using PCR in pid=256
> [mpegts @ 0x55f8452797c0] service 2 using PCR in pid=257
> [mpegts @ 0x55f8452797c0] service 3 using PCR in pid=258
> [mpegts @ 0x55f8452797c0] service 4 using PCR in pid=256
>
>
> Signed-off-by: Andreas Hakon
> ---
> libavformat/mpegtsenc.c | 167
> +--
> 1 file changed, 105 insertions(+), 62 deletions(-)
>
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index fc0ea22..6a0dd55 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -60,6 +60,7 @@ typedef struct MpegTSService {
> int pcr_packet_count;
> int pcr_packet_period;
> AVProgram *program;
> +AVStream *pcr_st;
> } MpegTSService;
>
> // service_type values as defined in ETSI 300 468
> @@ -774,7 +775,7 @@ static int mpegts_init(AVFormatContext *s)
> MpegTSWrite *ts = s->priv_data;
> MpegTSWriteStream *ts_st;
> MpegTSService *service;
> -AVStream *st, *pcr_st = NULL;
> +AVStream *st;
> AVDictionaryEntry *title, *provider;
> int i, j;
> const char *service_name;
> @@ -831,6 +832,11 @@ static int mpegts_init(AVFormatContext *s)
> }
> }
>
> +for (i = 0; i < ts->nb_services; i++) {
> +service = ts->services[i];
> +service->pcr_st = NULL;
> +}
> +
If you are going to add pcr_st to MpegTSService then it would make sense to
move the
initialization to mpegts_add_service function.
> ts->pat.pid = PAT_PID;
> /* Initialize at 15 so that it wraps and is equal to 0 for the
> * first packet we write. */
> @@ -872,17 +878,6 @@ static int mpegts_init(AVFormatContext *s)
> goto fail;
> }
>
> -program = av_find_program_from_stream(s, NULL, i);
> -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;
> /* MPEG pid values < 16 are reserved. Applications which set st->id
> in
> * this range are assigned a calculated pid. */
> if (st->id < 16) {
> @@ -895,11 +890,6 @@ static int mpegts_init(AVFormatContext *s)
> ret = AVERROR(EINVAL);
> goto fail;
> }
> -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;
> -}
> for (j = 0; j < i; j++) {
> if (pids[j] == ts_st->pid) {
> av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n",
> ts_st->pid);
> @@ -913,12 +903,7 @@ static int mpegts_init(AVFormatContext *s)
> ts_st->first_pts_check = 1;
> ts_st->cc = 15;
> ts_st->discontinuity = ts->flags & MPEGTS_FLAG_DISCONT;
> -/* 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;
> -pcr_st = st;
> -}
> +
> if (st->codecpar->codec_id == AV_CODEC_ID_AAC &&
> st->codecpar->extradata_size > 0) {
> AVStream *ast;
> @@ -949,50 +934,109 @@ static int mpegts_init(AVFormatContext *s)
> if (st->codecpar->codec_id ==
[FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v4]
Hi,
An updated version that fixes all previous bugs:
https://patchwork.ffmpeg.org/patch/14109/
https://patchwork.ffmpeg.org/patch/14099/
https://patchwork.ffmpeg.org/patch/14036/
It passes the tests pointed by Michael Niedermayer (Sample_cut.ts) and
Andriy Gelman (day_flight.mpg).
I hope this time the patch will be accepted.
Regards.
A.H.
---From 8381febd0e881cfcd53583b0ccdd7eb2c580e422 Mon Sep 17 00:00:00 2001
From: Andreas Hakon
Date: Mon, 29 Jul 2019 12:02:06 +0100
Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple
programs [v4]
The MPEG-TS muxer has a serious bug related to the use of multiple programs:
in that case, the PCR pid selection is incomplete for all services except one.
This patch solves this problem and select correct streams for each program.
Furthermore, it passes all the checks and doesn't generate any regression.
Also fully compatible with shared pids over services.
You can check it with this example command:
$ ./ffmpeg -loglevel verbose -y -f data -i /dev/zero \
-filter_complex "nullsrc=s=60x60,split=2[v0][v1],anullsrc[a]" \
-map [v0] -c:v:0 rawvideo \
-map [v1] -c:v:1 rawvideo \
-map [a] -c:a:0 pcm_s8 \
-program st=0,2 -program st=1,2 -program st=2 -program st=0 -f mpegts out.ts
And you will see something like:
[mpegts @ 0x55f8452797c0] service 1 using PCR in pid=256
[mpegts @ 0x55f8452797c0] service 2 using PCR in pid=257
[mpegts @ 0x55f8452797c0] service 3 using PCR in pid=258
[mpegts @ 0x55f8452797c0] service 4 using PCR in pid=256
Signed-off-by: Andreas Hakon
---
libavformat/mpegtsenc.c | 167 +--
1 file changed, 105 insertions(+), 62 deletions(-)
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index fc0ea22..6a0dd55 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -60,6 +60,7 @@ typedef struct MpegTSService {
int pcr_packet_count;
int pcr_packet_period;
AVProgram *program;
+AVStream *pcr_st;
} MpegTSService;
// service_type values as defined in ETSI 300 468
@@ -774,7 +775,7 @@ static int mpegts_init(AVFormatContext *s)
MpegTSWrite *ts = s->priv_data;
MpegTSWriteStream *ts_st;
MpegTSService *service;
-AVStream *st, *pcr_st = NULL;
+AVStream *st;
AVDictionaryEntry *title, *provider;
int i, j;
const char *service_name;
@@ -831,6 +832,11 @@ static int mpegts_init(AVFormatContext *s)
}
}
+for (i = 0; i < ts->nb_services; i++) {
+service = ts->services[i];
+service->pcr_st = NULL;
+}
+
ts->pat.pid = PAT_PID;
/* Initialize at 15 so that it wraps and is equal to 0 for the
* first packet we write. */
@@ -872,17 +878,6 @@ static int mpegts_init(AVFormatContext *s)
goto fail;
}
-program = av_find_program_from_stream(s, NULL, i);
-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;
/* MPEG pid values < 16 are reserved. Applications which set st->id in
* this range are assigned a calculated pid. */
if (st->id < 16) {
@@ -895,11 +890,6 @@ static int mpegts_init(AVFormatContext *s)
ret = AVERROR(EINVAL);
goto fail;
}
-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;
-}
for (j = 0; j < i; j++) {
if (pids[j] == ts_st->pid) {
av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n",
ts_st->pid);
@@ -913,12 +903,7 @@ static int mpegts_init(AVFormatContext *s)
ts_st->first_pts_check = 1;
ts_st->cc = 15;
ts_st->discontinuity = ts->flags & MPEGTS_FLAG_DISCONT;
-/* 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;
-pcr_st = st;
-}
+
if (st->codecpar->codec_id == AV_CODEC_ID_AAC &&
st->codecpar->extradata_size > 0) {
AVStream *ast;
@@ -949,50 +934,109 @@ static int mpegts_init(AVFormatContext *s)
if (st->codecpar->codec_id == AV_CODEC_ID_OPUS) {
ts_st->opus_pending_trim_start = st->codecpar->initial_padding *
48000 / st->codecpar->sample_rate;
}
+
+/* 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
