Andreas, On Mon, 22. Jul 12:22, Andreas Håkon wrote: > Hi, > > Based on the discussion of my previous patch > https://patchwork.ffmpeg.org/patch/13487/ > Here I publish a first part of the patch that only addresses the PCR problem.
Thanks for splitting the patch set. > > This supersedes too: https://patchwork.ffmpeg.org/patch/13745/ > > Regards. > A.H. > > --- > From a9d51e71c546986c8ea3bd111d16fc81354ffa3d Mon Sep 17 00:00:00 2001 > From: Andreas Hakon <andreas.ha...@protonmail.com> > Date: Mon, 22 Jul 2019 13:10:20 +0100 > Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple > programs > > 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. The PCR pid selection is probably fine. The issue is that pcr_packet_period is zero for N-1 out of N programs. That's why in your tests you see the insertion of the pcr adaptation field in each frame for 1/2 programs. > > This patch solves this problem. And you can check it with these commands: > > - First, make sure you have "ffmpeg-old" (a binary without the patch), > "ffmpeg-new" (a binary with the patch), and the test file "Day_Flight.mpg" > from > https://samples.ffmpeg.org/MPEG2/mpegts-klv/Day%20Flight.mpg > > - Execute these commands: > > $ ffmpeg-old -loglevel verbose -y -f mpegts -i Day_Flight.mpg \ > -map i:0x1e1 -c:v:0 copy -map i:0x1e1 -c:v:1 copy \ > -program st=0 -program st=1 -f mpegts out-error.ts > > $ ffmpeg-new -loglevel verbose -y -f mpegts -i Day_Flight.mpg \ > -map i:0x1e1 -c:v:0 copy -map i:0x1e1 -c:v:1 copy \ > -program st=0 -program st=1 -f mpegts out-ok.ts > > - As a result, you have two files "out-error.ts" and "out-ok.ts". Both files > with the same content (but not identical): two services, and each one with one > video stream. However, the first file has errors in the PCR timestamps. > > - If you analyze the files, you can see that the file "out-error.ts" has PCR > marks in every packet of the pid 256 (a total of 561862 packets with PCR > marks). > However, the pid 257 has 783 packets only with PCR marks. On the other hand, > the file "out-ok.ts" has correct PCR marks, and both pids (256 & 257) only > have > 783 packets with PCR timestamps. > > - You can do additional checks with the tool "tsreport" from the tstools > package. > Try this to see the difference: > > $ tsreport -b -v out-ok.ts | more > $ tsreport -b -v out-error.ts | more > > > Signed-off-by: Andreas Hakon <andreas.ha...@protonmail.com> > --- > libavformat/mpegtsenc.c | 82 > +++++++++++++++++++++++++---------------------- > 1 file changed, 44 insertions(+), 38 deletions(-) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index fc0ea22..4a978ed 100644 > --- a/libavformat/mpegtsenc.c > +++ b/libavformat/mpegtsenc.c > @@ -953,46 +953,54 @@ static int mpegts_init(AVFormatContext *s) > > 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 no video stream, use the first stream as PCR */ > + if (service->pcr_pid == 0x1fff && s->nb_streams > 0) { > + pcr_st = s->streams[0]; How do you know that s->streams[0] belongs to this particular program? > + ts_st = pcr_st->priv_data; > + service->pcr_pid = ts_st->pid; > + } else > + ts_st = pcr_st->priv_data; pcr_st stream is set outside the for loop. No reason that it's part of this program. > + > + 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; These assignments do not change for each service/program. They should probably be outside of the for loop. > + 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); > + } else { > + service->pcr_packet_period = > + 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); pcr_packet_period may be incorrect here because ts_st may be coming from a stream that's not part of this program. > } > - } 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); > } > > ts->last_pat_ts = AV_NOPTS_VALUE; > @@ -1005,8 +1013,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; > 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".
