[FFmpeg-devel] [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple programs
Andriy Gelman
andriy.gelman at gmail.com
Sat Jul 27 10:05:17 EEST 2019
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 at 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 at 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
More information about the ffmpeg-devel
mailing list