[FFmpeg-devel] libavformat/mpegtsenc: fix incorrect PCR with multiple programs [v4]

Andriy Gelman andriy.gelman at gmail.com
Thu Aug 1 01:23:23 EEST 2019


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.hakon at 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.hakon at 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


More information about the ffmpeg-devel mailing list