[FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v3]
Andriy Gelman
andriy.gelman at gmail.com
Mon Jul 29 09:46:37 EEST 2019
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 at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list