[FFmpeg-devel] [bug/patch] MPEG-TS muxer: PCR not in sync with PTS/DTS
Baptiste Coudurier
baptiste.coudurier
Wed Jul 22 01:52:01 CEST 2009
Hi,
First thanks for your work.
On Tue, Jul 21, 2009 at 05:24:53PM +0200, Niobos wrote:
> I'm trying to transcode a video file to h264/x264 + aac into an
> MPEG-TS container. FFmpeg almost immediately (after a few seconds
> encoding) warns me that "dts < pcr, TS is invalid". When I try to
> play the resulting TS-file, it stops playing around the timestamp
> the warning showed up. I tried playing on iPhone and VLC, both show
> the problem.
>
> I only have a limited knowledge of MPEG-TS and ffmpeg, so be warned.
>
> ffmpeg (libavformat/mpegtsenc.c in particular) calculates a muxrate
> from the stream bitrates. The PCR is generated from this muxrate;
> the PTS (and DTS) are generated from the stream itself.
> The calculated muxrate is too low, this causes the stream (DTS) and
> container (PCR) to slowly drift apart. Specifying a higher -muxrate
> on the command line solves the "DTS < PCR" warning.
>
> I'm not an expert programmer, but I gave it a shot to write a patch:
> * mpegts-varname-freq-period.diff : Pure cosmetics : I was trying to
> understand the calculation-code. There are 3 variables
> {sdt,pat,pcr}_packet_freq. They contain the number of packets
> between an {sdt,pat,pcr} insertion. I suggest renaming these to
> {sdt,pat,pcr}_packet_period. A higher *frequency* means less time
> between items, this is not how the vars are used in the code. A
> higher *period* means more time between the items, which _is_ how
> they're used. (period = 1/frequency)
>
> * mpegts-overhead-calc.diff : Algorithm change : I rewrote the
> overhead calculations. The PES overhead is correct for my specific
> case (1 PES per video frame, 1 PES per 150ms audio), but might need
> some work to be more generic. The PAT, PMT, SDT and PCR overhead
> calculations should be correct; the original code was wrong: a
> _longer_ time between PCRs would _increase_ the overhead...
>
> The resulting code works much better, but the PCR still drifts apart
> from the DTS. A "normal" MPEG-TS contains a NULL-stream to keep the
> required bitrate constant. Another way to implement it would be to
> indicate a rate incontinuity and change the muxrate. I consider
> inserting NULL packets easier. The third patch is a try to implement
> this in ffmpeg:
> * The muxrate is put at 2% higher than calculated
> * If DTS - PCR becomes larger than 1.5 seconds (just a guess,
> hardcoded for now) one (or more) NULL-packet(s) is (are) inserted to
> get the PCR near the DTS again
>
> This whole thing is also posted on roundup, before I learned that
> this mailinglist was a better place to post to:
> https://roundup.ffmpeg.org/roundup/ffmpeg/issue1279
> Index: libavformat/mpegtsenc.c
> ===================================================================
> --- libavformat/mpegtsenc.c (revision 19469)
> +++ libavformat/mpegtsenc.c (working copy)
> @@ -163,7 +163,7 @@
> #define DEFAULT_PES_HEADER_FREQ 16
> #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170)
>
> -/* we retransmit the SI info at this rate */
> +/* we retransmit the SI info at this rate (in ms) */
> #define SDT_RETRANS_TIME 500
> #define PAT_RETRANS_TIME 100
> #define PCR_RETRANS_TIME 20
> @@ -429,9 +429,10 @@
> /* PES header size */
> if (st->codec->codec_type == CODEC_TYPE_VIDEO ||
> st->codec->codec_type == CODEC_TYPE_SUBTITLE)
> - total_bit_rate += 25 * 8 / av_q2d(st->codec->time_base);
> + total_bit_rate += (19*8 + 92*8) / av_q2d(st->codec->time_base); /* 19B PES header every frame */
> + /* on average 92B of padding at the end of every packet */
> else
> - total_bit_rate += total_bit_rate * 25 / DEFAULT_PES_PAYLOAD_SIZE;
> + total_bit_rate += (14*8 + 92*8) * 7; /* 14B PES header 6.7 times a second */
> }
What's the rationale behind the avrage padding value ?
Is it necessary if we increase the bitrate by 2% ?
>
> /* if no video stream, use the first stream as PCR */
> @@ -465,9 +466,9 @@
>
> 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 */
> + 1000 * 8 * sdt_size / SDT_RETRANS_TIME + /* SDT size */
> + 1000 * 8 * pat_pmt_size / PAT_RETRANS_TIME + /* PAT+PMT size */
> + 1000 * 8 * 8 / PCR_RETRANS_TIME; /* PCR size */
>
> av_log(s, AV_LOG_DEBUG, "muxrate %d freq sdt %d pat %d\n",
> total_bit_rate, ts->sdt_packet_freq, ts->pat_packet_freq);
This hunk LGTM.
> Index: libavformat/mpegtsenc.c
> ===================================================================
> --- libavformat/mpegtsenc.c (revision 19469)
> +++ libavformat/mpegtsenc.c (working copy)
> @@ -470,6 +470,8 @@
> 1000 * 8 * pat_pmt_size / PAT_RETRANS_TIME + /* PAT+PMT size */
> 1000 * 8 * 8 / PCR_RETRANS_TIME; /* PCR size */
>
> + total_bit_rate += total_bit_rate / 50; /* add 2% extra as spare */
> +
> av_log(s, AV_LOG_DEBUG, "muxrate %d freq sdt %d pat %d\n",
> total_bit_rate, ts->sdt_packet_freq, ts->pat_packet_freq);
>
> @@ -512,6 +514,25 @@
> }
> }
>
> +/* Insert NULL packets to keep PTS near PCR */
You mean DTS.
> +static void instert_null_packets(AVFormatContext *s, int64_t dts)
typo.
> + MpegTSWrite *ts = s->priv_data;
> + uint8_t buf[TS_PACKET_SIZE];
> + uint8_t *q;
> +
> + while( dts != AV_NOPTS_VALUE && dts > ts->cur_pcr && dts - ts->cur_pcr > 135000LL ) {
> + q = buf;
> + *q++ = 0x47;
> + *q++ = 0x00 | 0x1f;
> + *q++ = 0xff;
> + *q++ = 0x10;
> + memset(q, 0x00, TS_PACKET_SIZE-4);
> + put_buffer(s->pb, buf, TS_PACKET_SIZE);
> + ts->cur_pcr += TS_PACKET_SIZE*8*90000LL/ts->mux_rate;
> + }
I think the constraint delta PCR <= 0.1s might not be respected here.
Btw coding style is:
- no space after ( and before )
- 4 spaces indentation
> +
> static void write_pts(uint8_t *q, int fourbits, int64_t pts)
> {
> int val;
> @@ -542,6 +563,7 @@
> is_start = 1;
> while (payload_size > 0) {
> retransmit_si_info(s);
> + instert_null_packets(s, dts);
>
> write_pcr = 0;
> if (ts_st->pid == ts_st->service->pcr_pid) {
The other solution is to only bump PCR and not
writing these padding packets. However I remember reading,
about some specs requiring these packets to be present.
> Index: libavformat/mpegtsenc.c
> ===================================================================
> --- libavformat/mpegtsenc.c (revision 19469)
> +++ libavformat/mpegtsenc.c (working copy)
> @@ -44,7 +44,7 @@
> char *provider_name;
> int pcr_pid;
> int pcr_packet_count;
> - int pcr_packet_freq;
> + int pcr_packet_period;
> } MpegTSService;
>
> typedef struct MpegTSWrite {
> @@ -52,9 +52,9 @@
> MpegTSSection sdt; /* MPEG2 sdt table context */
> MpegTSService **services;
> int sdt_packet_count;
> - int sdt_packet_freq;
> + int sdt_packet_period;
> int pat_packet_count;
> - int pat_packet_freq;
> + int pat_packet_period;
> int nb_services;
> int onid;
> int tsid;
> @@ -442,11 +442,11 @@
>
> if (total_bit_rate <= 8 * 1024)
> total_bit_rate = 8 * 1024;
> - service->pcr_packet_freq = (total_bit_rate * PCR_RETRANS_TIME) /
> + service->pcr_packet_period = (total_bit_rate * PCR_RETRANS_TIME) /
> (TS_PACKET_SIZE * 8 * 1000);
> - ts->sdt_packet_freq = (total_bit_rate * SDT_RETRANS_TIME) /
> + ts->sdt_packet_period = (total_bit_rate * SDT_RETRANS_TIME) /
> (TS_PACKET_SIZE * 8 * 1000);
> - ts->pat_packet_freq = (total_bit_rate * PAT_RETRANS_TIME) /
> + ts->pat_packet_period = (total_bit_rate * PAT_RETRANS_TIME) /
> (TS_PACKET_SIZE * 8 * 1000);
>
> ts->mux_rate = 1; // avoid div by 0
> @@ -470,7 +470,7 @@
> PCR_RETRANS_TIME * 8 * 8 / 1000; /* PCR size */
>
> av_log(s, AV_LOG_DEBUG, "muxrate %d freq sdt %d pat %d\n",
> - total_bit_rate, ts->sdt_packet_freq, ts->pat_packet_freq);
> + total_bit_rate, ts->sdt_packet_period, ts->pat_packet_period);
>
> if (s->mux_rate)
> ts->mux_rate = s->mux_rate;
> @@ -498,11 +498,11 @@
> MpegTSWrite *ts = s->priv_data;
> int i;
>
> - if (++ts->sdt_packet_count == ts->sdt_packet_freq) {
> + if (++ts->sdt_packet_count == ts->sdt_packet_period) {
> ts->sdt_packet_count = 0;
> mpegts_write_sdt(s);
> }
> - if (++ts->pat_packet_count == ts->pat_packet_freq) {
> + if (++ts->pat_packet_count == ts->pat_packet_period) {
> ts->pat_packet_count = 0;
> mpegts_write_pat(s);
> for(i = 0; i < ts->nb_services; i++) {
> @@ -546,7 +546,7 @@
> if (ts_st->pid == ts_st->service->pcr_pid) {
> ts_st->service->pcr_packet_count++;
> if (ts_st->service->pcr_packet_count >=
> - ts_st->service->pcr_packet_freq) {
> + ts_st->service->pcr_packet_period) {
> ts_st->service->pcr_packet_count = 0;
> write_pcr = 1;
> }
This one LGTM.
[...]
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-devel
mailing list