| Message ID | kKG7q2gkiNGRhHDU5scRi1OGdR61amcf-pkujrYC_fCcvyqkWteDmwwVmVyx4Y0sx9sxtNv2RfB256CW4FkEgH00TzThQh458E53miWSHyk=@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Andreas, On Sun, 28. Jul 19:07, Andreas Håkon wrote: > Hi, > > This last version fixes the small bug discovered by Michael Niedermayer: > https://patchwork.ffmpeg.org/patch/14099/ > > This version is finally clean. > > Regards. > A.H. > > --- > From 08565b81aa2b6d75043e5e984da143480891c3b0 Mon Sep 17 00:00:00 2001 > From: Andreas Hakon <andreas.hakon@protonmail.com> > Date: Sun, 28 Jul 2019 19:59:51 +0100 > Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple > programs [v3] > > The MPEG-TS muxer has a serious bug related to the PCR pid selection. > This bug appears when more than one program is used. The root cause is because > the current code targets only one program when selecting the stream for the PCR. I'd suggest to update the commit message because pid selection is not the main issue. > > This patch solves this problem and select correct streams for each program. > Furthermore, it passes all the checks and doesn't generate any regression. > > You can check it with this 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 -program st=1 -program st=2 -f mpegts out.ts > > And you will see something like: > > [mpegts @ 0x562f388cd800] service 1 using PCR in pid=256 > [mpegts @ 0x562f388cd800] service 2 using PCR in pid=257 > [mpegts @ 0x562f388cd800] service 3 using PCR in pid=258 > > > Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com> > --- > libavformat/mpegtsenc.c | 103 ++++++++++++++++++++++++++++------------------- > 1 file changed, 62 insertions(+), 41 deletions(-) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index fc0ea22..fc340cd 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. */ > @@ -917,7 +923,12 @@ static int mpegts_init(AVFormatContext *s) > if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > service->pcr_pid == 0x1fff) { > service->pcr_pid = ts_st->pid; > - pcr_st = st; > + 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; > } > if (st->codecpar->codec_id == AV_CODEC_ID_AAC && > st->codecpar->extradata_size > 0) { > @@ -951,50 +962,62 @@ static int mpegts_init(AVFormatContext *s) > } > } > > - av_freep(&pids); > + for (i = 0; i < ts->nb_services; i++) { > + service = ts->services[i]; > > - /* if no video stream, use the first stream as PCR */ > - if (service->pcr_pid == 0x1fff && s->nb_streams > 0) { > - pcr_st = s->streams[0]; > - ts_st = pcr_st->priv_data; > - service->pcr_pid = ts_st->pid; > - } else > - ts_st = pcr_st->priv_data; > - > - if (ts->mux_rate > 1) { > - service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / > - (TS_PACKET_SIZE * 8 * 1000); > - ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / > - (TS_PACKET_SIZE * 8 * 1000); > - ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / > - (TS_PACKET_SIZE * 8 * 1000); > - > - if (ts->copyts < 1) > - ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); > - } else { > - /* Arbitrary values, PAT/PMT will also be written on video key frames */ > - ts->sdt_packet_period = 200; > - ts->pat_packet_period = 40; > - if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { > - int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0); > - if (!frame_size) { > - av_log(s, AV_LOG_WARNING, "frame size not set\n"); > - service->pcr_packet_period = > - pcr_st->codecpar->sample_rate / (10 * 512); > + /* get the PCR stream (real or candidate) */ > + if (!service->pcr_st) { > + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i", service->sid); > + ret = AVERROR(EINVAL); > + goto fail; > + } Do you have to consider an edge case where the streams are not unique to each program? For example the following will return an error, whereas it works before the patch: ./ffmpeg -loglevel verbose -y -f mpegts -i day_flight.mpg \ -map i:0x1e1 -c:v:0 copy \ -program st=0 -program st=0 -f mpegts out-error.ts Also, you are missing \n at the end of your error message. > + ts_st = service->pcr_st->priv_data; > + > + /* if no video stream, use the pid of the candidate PCR */ > + if (service->pcr_pid == 0x1fff) > + service->pcr_pid = ts_st->pid; > + > + if (ts->mux_rate > 1) { > + service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / > + (TS_PACKET_SIZE * 8 * 1000); > + ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / > + (TS_PACKET_SIZE * 8 * 1000); > + ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / > + (TS_PACKET_SIZE * 8 * 1000); > + if (ts->copyts < 1) > + ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); > + } else { > + /* Arbitrary values, PAT/PMT will also be written on video key frames */ > + ts->sdt_packet_period = 200; > + ts->pat_packet_period = 40; > + if (service->pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { > + int frame_size = av_get_audio_frame_duration2(service->pcr_st->codecpar, 0); > + if (!frame_size) { > + av_log(s, AV_LOG_WARNING, "frame size not set\n"); > + service->pcr_packet_period = > + service->pcr_st->codecpar->sample_rate / (10 * 512); > + } else { > + service->pcr_packet_period = > + service->pcr_st->codecpar->sample_rate / (10 * frame_size); > + } > } else { > + // max delta PCR 0.1s > + // TODO: should be avg_frame_rate > service->pcr_packet_period = > - pcr_st->codecpar->sample_rate / (10 * frame_size); > + ts_st->user_tb.den / (10 * ts_st->user_tb.num); > } > - } else { > - // max delta PCR 0.1s > - // TODO: should be avg_frame_rate > - service->pcr_packet_period = > - ts_st->user_tb.den / (10 * ts_st->user_tb.num); > + if (!service->pcr_packet_period) > + service->pcr_packet_period = 1; > } > - if (!service->pcr_packet_period) > - service->pcr_packet_period = 1; > + > + // output a PCR as soon as possible > + service->pcr_packet_count = service->pcr_packet_period; > + > + av_log(s, AV_LOG_VERBOSE, "service %i using PCR in pid=%i\n", service->sid, service->pcr_pid); > } > > + av_freep(&pids); > + > ts->last_pat_ts = AV_NOPTS_VALUE; > ts->last_sdt_ts = AV_NOPTS_VALUE; > // The user specified a period, use only it > @@ -1005,8 +1028,6 @@ static int mpegts_init(AVFormatContext *s) > ts->sdt_packet_period = INT_MAX; > } > > - // output a PCR as soon as possible > - service->pcr_packet_count = service->pcr_packet_period; > ts->pat_packet_count = ts->pat_packet_period - 1; > ts->sdt_packet_count = ts->sdt_packet_period - 1; > > -- > 1.7.10.4 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hi Andriy, A new updated version here: https://patchwork.ffmpeg.org/patch/14121/ ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Monday, 29 de July de 2019 8:46, Andriy Gelman <andriy.gelman@gmail.com> wrote: > > The MPEG-TS muxer has a serious bug related to the PCR pid selection. > > This bug appears when more than one program is used. The root cause is because > > the current code targets only one program when selecting the stream for the PCR. > > I'd suggest to update the commit message because pid selection is not the main > issue. Done! > > + if (!service->pcr_st) { > > + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i", service->sid); > > + ret = AVERROR(EINVAL); > > + goto fail; > > + } > > Do you have to consider an edge case where the streams are not unique to each > program? > For example the following will return an error, whereas it works before the > patch: > > ./ffmpeg -loglevel verbose -y -f mpegts -i day_flight.mpg \ > -map i:0x1e1 -c:v:0 copy \ > -program st=0 -program st=0 -f mpegts out-error.ts > Wow! A real edge case... Just solving it has involved a great rewriting. However, it is worth considering as this patch is the prelude to another (the one for real MPTS interleave muxing). > Also, you are missing \n at the end of your error message. Done! Thank you Andriy for pointing this edge case. Regards. A.H. ---
On Sun, 28 Jul 2019, Andreas Håkon wrote: > Hi, > > This last version fixes the small bug discovered by Michael Niedermayer: > https://patchwork.ffmpeg.org/patch/14099/ > > This version is finally clean. > From 8381febd0e881cfcd53583b0ccdd7eb2c580e422 Mon Sep 17 00:00:00 2001 > From: Andreas Hakon <andreas.hakon@protonmail.com> > 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 <andreas.hakon@protonmail.com> > --- > 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; > + } > + 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. > 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 (ts->services[j]->program == program) > + service = ts->services[j]; > + else > + continue; > + } 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: /* 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? > + > + 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? > + > + /* 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. > + } > + program = program ? av_find_program_from_stream(s, program, i) : NULL; > + } while (program); > } > > av_freep(&pids); > > - /* if no video stream, use the first stream as PCR */ > - if (service->pcr_pid == 0x1fff && s->nb_streams > 0) { > - pcr_st = s->streams[0]; > - ts_st = pcr_st->priv_data; > - service->pcr_pid = ts_st->pid; > - } else > - ts_st = pcr_st->priv_data; > - > - if (ts->mux_rate > 1) { > - service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / > - (TS_PACKET_SIZE * 8 * 1000); > - ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / > - (TS_PACKET_SIZE * 8 * 1000); > - ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / > - (TS_PACKET_SIZE * 8 * 1000); > - > - if (ts->copyts < 1) > - ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); > - } else { > - /* Arbitrary values, PAT/PMT will also be written on video key frames */ > - ts->sdt_packet_period = 200; > - ts->pat_packet_period = 40; > - if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { > - int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0); > - if (!frame_size) { > - av_log(s, AV_LOG_WARNING, "frame size not set\n"); > - service->pcr_packet_period = > - pcr_st->codecpar->sample_rate / (10 * 512); > + 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; > + } > + /* get the PCR stream (real or candidate) */ > + ts_st = service->pcr_st->priv_data; > + > + /* if no video stream then use the pid of the candidate PCR */ > + if (service->pcr_pid == 0x1fff) > + service->pcr_pid = ts_st->pid; > + > + if (ts->mux_rate > 1) { > + service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / > + (TS_PACKET_SIZE * 8 * 1000); > + ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / > + (TS_PACKET_SIZE * 8 * 1000); > + ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / > + (TS_PACKET_SIZE * 8 * 1000); > + if (ts->copyts < 1) > + ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); > + } else { > + /* Arbitrary values, PAT/PMT will also be written on video key frames */ > + ts->sdt_packet_period = 200; > + ts->pat_packet_period = 40; > + if (service->pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { > + int frame_size = av_get_audio_frame_duration2(service->pcr_st->codecpar, 0); > + if (!frame_size) { > + av_log(s, AV_LOG_WARNING, "frame size not set\n"); > + service->pcr_packet_period = > + service->pcr_st->codecpar->sample_rate / (10 * 512); > + } else { > + service->pcr_packet_period = > + service->pcr_st->codecpar->sample_rate / (10 * frame_size); > + } > } else { > + // max delta PCR 0.1s > + // TODO: should be avg_frame_rate > service->pcr_packet_period = > - pcr_st->codecpar->sample_rate / (10 * frame_size); > + ts_st->user_tb.den / (10 * ts_st->user_tb.num); > } > - } else { > - // max delta PCR 0.1s > - // TODO: should be avg_frame_rate > - service->pcr_packet_period = > - ts_st->user_tb.den / (10 * ts_st->user_tb.num); > + if (!service->pcr_packet_period) > + service->pcr_packet_period = 1; > } > - if (!service->pcr_packet_period) > - service->pcr_packet_period = 1; > + > + // output a PCR as soon as possible > + service->pcr_packet_count = service->pcr_packet_period; > + > + av_log(s, AV_LOG_VERBOSE, "service %i using PCR in pid=%i\n", service->sid, service->pcr_pid); > + } > + > + 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. > > ts->last_pat_ts = AV_NOPTS_VALUE; > @@ -1005,8 +1049,6 @@ static int mpegts_init(AVFormatContext *s) > ts->sdt_packet_period = INT_MAX; > } > > - // output a PCR as soon as possible > - service->pcr_packet_count = service->pcr_packet_period; > ts->pat_packet_count = ts->pat_packet_period - 1; > ts->sdt_packet_count = ts->sdt_packet_period - 1; > > @@ -1016,7 +1058,7 @@ static int mpegts_init(AVFormatContext *s) > av_log(s, AV_LOG_VERBOSE, "muxrate %d, ", ts->mux_rate); > av_log(s, AV_LOG_VERBOSE, > "pcr every %d pkts, sdt every %d, pat/pmt every %d pkts\n", > - service->pcr_packet_period, > + ts_st->service->pcr_packet_period, > ts->sdt_packet_period, ts->pat_packet_period); > > if (ts->m2ts_mode == -1) { > @@ -1031,6 +1073,7 @@ static int mpegts_init(AVFormatContext *s) > > fail: > av_freep(&pids); > +fail_at_end: > return ret; > } > Regards, Marton
Hi Marton, First of all, a new version [v4] is posted here: https://patchwork.ffmpeg.org/patch/14121/ However, I'll comment on your suggestions, as they are reasonable. ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Wednesday, 31 de July de 2019 23:58, Marton Balint <cus@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. > > + > > + /* 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. > /* 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. > > > - > > - 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. > > Regards, > Marton Thank you for your comments! Regards. A.H. ---
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 <cus@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
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 1 de August de 2019 23:09, Marton Balint <cus@passwd.hu> wrote: > 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. OK, no problem! > > However, I'll comment on your suggestions, as they are reasonable. > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > > On Wednesday, 31 de July de 2019 23:58, Marton Balint cus@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. OK. I'll remove it. I think is a good practice to initializa all vars, but if you prefer it that way, I don't criticize it. > > > > + > > > > + /* 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)" ). B) Just one time, if only one service is found for this stream. C) Multiple times, if the stream is inside multiple services. 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? > 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! > > > > + 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. OK. I agree to change it to an "av_assert". Thank you for the tip! > Thanks, > Marton I'll prepare a new version. Stay on hold. Regards. A.H. ---
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
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday, 2 de August de 2019 10:09, Marton Balint <cus@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. ---
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index fc0ea22..fc340cd 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. */ @@ -917,7 +923,12 @@ static int mpegts_init(AVFormatContext *s) if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && service->pcr_pid == 0x1fff) { service->pcr_pid = ts_st->pid; - pcr_st = st; + 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; } if (st->codecpar->codec_id == AV_CODEC_ID_AAC && st->codecpar->extradata_size > 0) { @@ -951,50 +962,62 @@ static int mpegts_init(AVFormatContext *s) } } - av_freep(&pids); + for (i = 0; i < ts->nb_services; i++) { + service = ts->services[i]; - /* if no video stream, use the first stream as PCR */ - if (service->pcr_pid == 0x1fff && s->nb_streams > 0) { - pcr_st = s->streams[0]; - ts_st = pcr_st->priv_data; - service->pcr_pid = ts_st->pid; - } else - ts_st = pcr_st->priv_data; - - if (ts->mux_rate > 1) { - service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / - (TS_PACKET_SIZE * 8 * 1000); - ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / - (TS_PACKET_SIZE * 8 * 1000); - ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / - (TS_PACKET_SIZE * 8 * 1000); - - if (ts->copyts < 1) - ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); - } else { - /* Arbitrary values, PAT/PMT will also be written on video key frames */ - ts->sdt_packet_period = 200; - ts->pat_packet_period = 40; - if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { - int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0); - if (!frame_size) { - av_log(s, AV_LOG_WARNING, "frame size not set\n"); - service->pcr_packet_period = - pcr_st->codecpar->sample_rate / (10 * 512); + /* get the PCR stream (real or candidate) */ + if (!service->pcr_st) { + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i", service->sid); + ret = AVERROR(EINVAL); + goto fail; + } + ts_st = service->pcr_st->priv_data; + + /* if no video stream, use the pid of the candidate PCR */ + if (service->pcr_pid == 0x1fff) + service->pcr_pid = ts_st->pid; + + if (ts->mux_rate > 1) { + service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / + (TS_PACKET_SIZE * 8 * 1000); + ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / + (TS_PACKET_SIZE * 8 * 1000); + ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / + (TS_PACKET_SIZE * 8 * 1000); + if (ts->copyts < 1) + ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); + } else { + /* Arbitrary values, PAT/PMT will also be written on video key frames */ + ts->sdt_packet_period = 200; + ts->pat_packet_period = 40; + if (service->pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { + int frame_size = av_get_audio_frame_duration2(service->pcr_st->codecpar, 0); + if (!frame_size) { + av_log(s, AV_LOG_WARNING, "frame size not set\n"); + service->pcr_packet_period = + service->pcr_st->codecpar->sample_rate / (10 * 512); + } else { + service->pcr_packet_period = + service->pcr_st->codecpar->sample_rate / (10 * frame_size); + } } else { + // max delta PCR 0.1s + // TODO: should be avg_frame_rate service->pcr_packet_period = - pcr_st->codecpar->sample_rate / (10 * frame_size); + ts_st->user_tb.den / (10 * ts_st->user_tb.num); } - } else { - // max delta PCR 0.1s - // TODO: should be avg_frame_rate - service->pcr_packet_period = - ts_st->user_tb.den / (10 * ts_st->user_tb.num); + if (!service->pcr_packet_period) + service->pcr_packet_period = 1; } - if (!service->pcr_packet_period) - service->pcr_packet_period = 1; + + // output a PCR as soon as possible + service->pcr_packet_count = service->pcr_packet_period; + + av_log(s, AV_LOG_VERBOSE, "service %i using PCR in pid=%i\n", service->sid, service->pcr_pid); } + av_freep(&pids); + ts->last_pat_ts = AV_NOPTS_VALUE; ts->last_sdt_ts = AV_NOPTS_VALUE; // The user specified a period, use only it @@ -1005,8 +1028,6 @@ static int mpegts_init(AVFormatContext *s) ts->sdt_packet_period = INT_MAX; } - // output a PCR as soon as possible - service->pcr_packet_count = service->pcr_packet_period; ts->pat_packet_count = ts->pat_packet_period - 1; ts->sdt_packet_count = ts->sdt_packet_period - 1;