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 <andreas.ha...@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.ha...@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; > + } > + 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 == 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; > + } > + > + 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: /*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; } } 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; } > } > > 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; fail_at_end is not deallocating any memory. Just return AVERROR(EINVAL) instead of goto would be fine. > + } > + /* 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; > } This is outside the stream loop, so why the random check? > > 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; I don't think you need this goto option. > } > > -- > 1.7.10.4 > Andriy _______________________________________________ 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".