[FFmpeg-devel] [bug/patch] MPEG-TS muxer: PCR not in sync withPTS/DTS

[Baptiste Coudurier]

Hi,

On 10/22/2009 12:51 PM, Mike Scheutzow wrote:
> Attached is a patch for the MPEG-TS muxer which makes it generate an
> accurate Constant Bitrate transport stream. This requires null packet
> insertion so that the PCR values are correct.
>
> This patch incorporates the work done by Niobos in
> ffmpeg-mpegts-vbr-4.diff, and also includes the follow-on fix I
> suggested to him.
>
> This patch does not solve all the problems I'm aware of with the ffmpeg
> mpegts muxer.
>
> In particular, there are still situations where the estimated bitrate is
> too small. For example, it is wrong for H.264 video streams, and for
> audio-only streams. Those problems are separate bugs. For now, the
> -muxrate option can be used with these type of input streams.
>
> Comments?
>
> --
> Mike Scheutzow
> scheutzow at alcatel-lucent.com
>
>
> mpeg-mux-cbr-v1.patch
>
>
> Index: libavformat/mpegtsenc.c
> ===================================================================
> --- libavformat/mpegtsenc.c	(revision 20231)
> +++ libavformat/mpegtsenc.c	(working copy)
> @@ -384,7 +384,7 @@
>       MpegTSService *service;
>       AVStream *st;
>       AVMetadataTag *title;
> -    int i, total_bit_rate;
> +    int i, total_bit_rate, nb_pkt;
>       const char *service_name;
>       uint64_t sdt_size, pat_pmt_size, pos;
>
> @@ -448,15 +448,6 @@
>           service->pcr_pid = ts_st->pid;
>       }
>
> -    if (total_bit_rate<= 8 * 1024)
> -        total_bit_rate = 8 * 1024;
> -    service->pcr_packet_period = (total_bit_rate * PCR_RETRANS_TIME) /
> -        (TS_PACKET_SIZE * 8 * 1000);
> -    ts->sdt_packet_period = (total_bit_rate * SDT_RETRANS_TIME) /
> -        (TS_PACKET_SIZE * 8 * 1000);
> -    ts->pat_packet_period = (total_bit_rate * PAT_RETRANS_TIME) /
> -        (TS_PACKET_SIZE * 8 * 1000);
> -
>       ts->mux_rate = 1; // avoid div by 0
>
>       /* write info at the start of the file, so that it will be fast to
> @@ -471,23 +462,36 @@
>       }
>       pat_pmt_size = url_ftell(s->pb) - pos;
>
> -    total_bit_rate +=
> -        total_bit_rate *  4 / TS_PACKET_SIZE           + /* TS  header size */
> -        SDT_RETRANS_TIME * 8 * sdt_size     / 1000     + /* SDT size */
> -        PAT_RETRANS_TIME * 8 * pat_pmt_size / 1000     + /* PAT+PMT size */
> -        PCR_RETRANS_TIME * 8 * 8            / 1000;      /* PCR size */
> +    if (total_bit_rate<= 8 * 1024)
> +        total_bit_rate = 8 * 1024;
>
> -    av_log(s, AV_LOG_DEBUG, "muxrate %d freq sdt %d pat %d\n",
> -           total_bit_rate, ts->sdt_packet_period, ts->pat_packet_period);
> +    total_bit_rate += 1000 * 8 * 8 / PCR_RETRANS_TIME;            /* PCR size */
> +    total_bit_rate += 4 * total_bit_rate / (TS_PACKET_SIZE-4)   + /* TS  header size */
> +         1000 * 8 * sdt_size             / SDT_RETRANS_TIME     + /* SDT size */
> +         1000 * 8 * pat_pmt_size         / PAT_RETRANS_TIME;      /* PAT+PMT size */

I think TS header size must be added first, then PCR, then SDT/PAT. 
Let's compute the bitrate correctly :)

 > [...]
>
> +    // want to output a PCR as soon as possible
> +    service->pcr_packet_count = service->pcr_packet_period;

Ok.

> +    av_log(s, AV_LOG_INFO, "calculated bitrate %dbps, muxrate %dbps, sdt every %d, pat/pmt every %d pkts\n",
> +           total_bit_rate, ts->mux_rate, ts->sdt_packet_period, ts->pat_packet_period);
> +
> +    // calc current pcr value using new ts->mux_rate (first ts pkt is pcr=0)
> +    nb_pkt = (sdt_size + pat_pmt_size) / TS_PACKET_SIZE;
> +    ts->cur_pcr = (nb_pkt * TS_PACKET_SIZE * 8 * 90000LL) / ts->mux_rate;

This should not be needed. cur_pcr is already adjusted in write_pat and 
write_pmt. That's why cur_pcr is adjusted below in the code.

 > [...]
>
> +/* Write a single null transport stream packet */
> +static void mpegts_insert_null_packet( AVFormatContext *s )

No space after '(' and before ')', this applies to the whole patch.

> [...]
 >
> +
> +/* Add a pes header to the front of payload, and segment into an integer number of
> + * ts packets. The final ts packet is padded using an over-sized adaptation header
> + * to exactly fill the last ts packet.
> + * NOTE: 'payload' contains a complete PES payload.
> + */
>   static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
>                                const uint8_t *payload, int payload_size,
>                                int64_t pts, int64_t dts)
> @@ -545,6 +602,7 @@
>       int val, is_start, len, header_len, write_pcr, private_code, flags;
>       int afc_len, stuffing_len;
>       int64_t pcr = -1; /* avoid warning */
> +    int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE);
>
>       is_start = 1;
>       while (payload_size>  0) {
> @@ -560,6 +618,18 @@
>               }
>           }
>
> +        if ( dts != AV_NOPTS_VALUE&&  (dts - (int64_t)ts->cur_pcr)>  delay ) {
> +
> +            /* pcr insert gets priority over null packet insert */
> +            if ( write_pcr ) {
> +                mpegts_insert_pcr_only(s, st);
> +            } else {
> +                mpegts_insert_null_packet(s);
> +            }
> +
> +            continue; /* recalculate write_pcr and possibly retransmit si_info */
> +        }
> +

Can't pcr be written along with data ?

Maybe something like:

if (!write_pcr && dts != AV_NOPTS_VALUE && dts - .. > delay) {
     mpegts_insert_null_packet(s);
     ts_st->service->pcr_packet_count++;
     continue;
}

>           /* prepare packet header */
>           q = buf;
>           *q++ = 0x47;
> @@ -709,7 +779,7 @@
>       uint8_t *buf= pkt->data;
>       uint8_t *data= NULL;
>       MpegTSWriteStream *ts_st = st->priv_data;
> -    const uint64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE);
> +    const int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE);

Left over ?

[...]

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org