[FFmpeg-devel] [bug/patch] MPEG-TS muxer: PCR not in sync with PTS/DTS
Baptiste Coudurier
baptiste.coudurier
Sun Jul 26 08:37:07 CEST 2009
Hi,
On Wed, Jul 22, 2009 at 09:52:40AM +0200, Niobos wrote:
> On 22 Jul 2009, at 01:52, Baptiste Coudurier wrote:
>
> >>@@ -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% ?
>
> The average padding is added to compensate for the last packet: Even
> if only 1 byte is needed, a full 184+4 bytes will be written/sent.
> Statistics tell us that, on average, the last packet will be half
> filled, hence the 184/2 bytes of padding.
> It's still needed when we add the 2%. Especially if the bitrate is
> relatively low compared to the framerate, the 92B/frame might become
> more than 2%. I'm asuming 1 frame = 1 PES. Example for 50fps at
> 100kbps (which is probably as low as anyone ever wants to go):
> 100kbps = 12.5kB/s; 50fps => 4.6kB/s padding (on average). 4.6kB/s
> is more than 2% of 12kB/s
>
> I agree that these calculations are not as clean as they should be.
> Another thing to consider is using -maxrate instead of -b to do
> these calculations.
Well, in CBR, maxrate == bitrate so it wouldn't change much.
We indeed to increase total bitrate reasonably to compensate VBR.
> >>
> >> /* 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.
>
> Well, I needed to change the first line as well (TS header): This is
> calculated in Bytes instead of bits. The new version is in the
> attached patch.
>
>
> >>+/* Insert NULL packets to keep PTS near PCR */
> >
> >You mean DTS.
>
> >>+static void instert_null_packets(AVFormatContext *s, int64_t dts)
> >
> >typo.
>
> Indeed. Corrected.
>
> >>+ 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
>
> Code style adopted.
> About the PCR interval < 100ms: True, I fixed this in the updated patch.
> I also added a "cur_pcr += written_length" to the
> retransmit_si_info, since this function writes bits without counting
> them.
>
> What about the hard-coded 1.5seconds (135000LL)?
You can use AVFormatContext->max_delay I think, so user can configure
it.
> >>+
> >>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.
>
> Quote from the specs (ISO 13818-1, section 2.4.2.2):
> >Data from the Transport Stream enters the T-STD at a piecewise
> >constant rate
>
> So just leaving out the packets violates this. We could dynamically
> step the muxrate to a lower value, but should ensure that the new
> muxrate is held "long enough" for the decoder to synchronize it's
> PLL (although software decoders synchronize almost instantly,
> hardware might need more time).
> But as Alexandre Ferrieux pointed out:
> >I confirm that the solution works in practice for several set-top-
> >boxes. Even disgusting hacks like setting PCR to (DTS-offset) give
> >a perfect playback, provided the timing of sending of the data
> >(over UDP) doesn't drift from this synthetic PCR (I wrote an
> >external spacer for that).
>
> I'm ok to make the insert_null_packets() an option which defaults to off
>
> Since you are considering all of my patches, I combined mpegts-
> overhead-calc.diff and mpegts-overhead-null-packets.diff into one
> patch. I also included a graph of {PCR,DTS,PTS} vs packet number for
> the original and patched version, which illustrates the problem
> quite well.
Please keep patches splited, because patches will be applied in
separate commits.
> Index: libavformat/mpegtsenc.c
> ===================================================================
> --- libavformat/mpegtsenc.c (revision 19469)
> +++ libavformat/mpegtsenc.c (working copy)
> @@ -53,8 +53,10 @@
> MpegTSService **services;
> int sdt_packet_count;
> int sdt_packet_freq;
> + uint64_t sdt_packet_size;
unsigned is sufficient.
> int pat_packet_count;
> int pat_packet_freq;
> + uint64_t pat_packet_size;
same.
But I don't think you need these fields.
> int nb_services;
> int onid;
> int tsid;
> @@ -163,7 +165,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
> @@ -386,7 +388,7 @@
> AVMetadataTag *title;
> int i, total_bit_rate;
> const char *service_name;
> - uint64_t sdt_size, pat_pmt_size, pos;
> + uint64_t pos;
>
> ts->tsid = DEFAULT_TSID;
> ts->onid = DEFAULT_ONID;
> @@ -429,9 +431,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 */
> }
>
> /* if no video stream, use the first stream as PCR */
> @@ -455,20 +458,22 @@
> find them */
> pos = url_ftell(s->pb);
> mpegts_write_sdt(s);
> - sdt_size = url_ftell(s->pb) - pos;
> + ts->sdt_packet_size = url_ftell(s->pb) - pos;
> pos = url_ftell(s->pb);
> mpegts_write_pat(s);
> for(i = 0; i < ts->nb_services; i++) {
> mpegts_write_pmt(s, ts->services[i]);
> }
> - pat_pmt_size = url_ftell(s->pb) - pos;
> + ts->pat_packet_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 */
> + 8 * 4 * total_bit_rate / (TS_PACKET_SIZE-4) + /* TS header size */
> + 1000 * 8 * ts->sdt_packet_size / SDT_RETRANS_TIME + /* SDT size */
> + 1000 * 8 * ts->pat_packet_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);
>
> @@ -501,6 +506,7 @@
> if (++ts->sdt_packet_count == ts->sdt_packet_freq) {
> ts->sdt_packet_count = 0;
> mpegts_write_sdt(s);
> + ts->cur_pcr += ts->sdt_packet_size *8*90000LL/ts->mux_rate;
> }
This is unneeded, mpegts_write_sdt will update pcr accordingly through
mpegts_write_section.
> if (++ts->pat_packet_count == ts->pat_packet_freq) {
> ts->pat_packet_count = 0;
> @@ -508,9 +514,31 @@
> for(i = 0; i < ts->nb_services; i++) {
> mpegts_write_pmt(s, ts->services[i]);
> }
> + ts->cur_pcr += ts->pat_packet_size *8*90000LL/ts->mux_rate;
> }
Same here.
> +
> static void write_pts(uint8_t *q, int fourbits, int64_t pts)
> {
> int val;
> @@ -552,6 +580,10 @@
> }
> }
>
> + /* Allow 1 payload packet if we need to send the PCR */
> + if( ! write_pcr && insert_null_packets(s, dts) )
> + continue;
Spaces before and after ( )
[...]
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-devel
mailing list