| Message ID | r0Lgih9CodfArUuzyWOmC-NtjPSeEXGklvVcozuLKDOW9ooUTBTli_N5XF2Q-OGA8lM1H3xWuqPCpDA7P2pKKQSK68QE6qel8Jr2Rlx-DtE=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
Ping ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Monday, 22 de July de 2019 14:22, Andreas Håkon <andreas.hakon@protonmail.com> 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. > > This supersedes too: https://patchwork.ffmpeg.org/patch/13745/ Waiting for comments or acceptance. https://trac.ffmpeg.org/ticket/8039 Regards. A.H. ---
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.hakon@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.hakon@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
Hi Andriy, I'm glad to read you! ------- Original Message ------- On Saturday, 27 de July de 2019 9:05, 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. > > 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. Yes, that's the root cause. > > + 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? First of all, note that most of the inserted code are just copy&paste of previous code. I have needed to move some blocks within a loop to process the data for "every" program. That's the objective of this patch. In any case, it's good to discuss about your comments. In this particular case "pcr_st = s->streams[0];" it seems that you're right. I have only checked with services using VIDEO streams. And analyzing the code Now I see that this case fails when there's no video stream. Any ideas? > > + 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. I'm going to take a closer look, I may have missed something. Thank you! > > + 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. Well, the code uses a "if" block and I prefer not to touch the original code much. Also, the code uses the var "ts->pcr_period", which must be initialized. And the initialization is only guaranteed within this loop, so even if the code seems redundant I prefer to leave it here. Any other reason to move it? > > + 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. I don't agree with you on this. The repetition rate of PCR marks is based on the TS structure, and not based on the service. That's an MPTS and not a SPTS. Therefore, the values are computed using the bitrate of the TS, and are EQUAL for all services within the TS. So, the value is correct regardless the service used. In fact, if you calculate it for one service, you can copy it for the rest. So don't care about this. > Andriy Thank you for your comments! I'll write more later. Stay tuned. Regards. A.H. ---
Hi Andriy, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Saturday, 27 de July de 2019 20:06, Andreas Håkon <andreas.hakon@protonmail.com> wrote: > > Thank you for your comments! I'll write more later. Stay tuned. > > Regards. > A.H. > Check the new version https://patchwork.ffmpeg.org/patch/14099/ Regards. A.H. ---
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]; + 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); + } 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); } - } 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;