[FFmpeg-devel] [PATCH] mpegts: pcr period option for variable bitrate multiplexing
Predrag Filipovic
agoracsinc at gmail.com
Tue Mar 29 01:58:29 CEST 2016
Inline answers and some questions/advice_sought are marked by "---" (start
and end)
Couple of NOTES and a bit more:
NOTE 1:
PCR is a different "animal" from PCR/PAT/PMT/SDT (PSI's): PSI have upper
bound
deadline with no consequences if inserted early while PCR value needs to
reflect
time at the "time" of insertion, and precisely so if system is to avoid
violating T-STD model.
- My guess is that the above was reasoning behind "original" logical
separation of
PCR and PAT/SDT handling
NOTE 2:
Given relatively minor "intended improvements" we tried to preserve current
structure
(architecture) despite its many flaws (PCR for VBR was plainly not
working).
NOTE 3:
CBR cases (for PCR/PAT/PMT/SDT) are very simple since muxrate provides time
measurement based on packet count. Its true that CBR is a special case of
VBR but
due significant difference in complexity between the two, special cases for
VBR are
"more efficient" way of handling requirements.
VBR mux-ing was not functional, and even with these additions, its not
really functioning
properly, To do that, code will need to compute "piece-wise" CBR segments
and base
insertion and PCR value on [n-1] + Rate[n]*bytes (piece-wise CBR). Add
separate PMT too.
This was paid effort for specific issue. I do plan to proceed with proper
design (I hope
paid effort but if not, once I get some time ... May onwards, after NAB
...).
I'll split patches per your suggestions and also include other recommended
changes
(see inline).
Regards
Predrag Filipovic
On Sun, Mar 27, 2016 at 8:09 AM, Michael Niedermayer <michael at niedermayer.cc
> wrote:
> On Fri, Mar 25, 2016 at 12:50:29PM -0400, Predrag Filipovic wrote:
> > Enable proper PCR insertion for VBR multiplexing (muxrate not specified).
> > Insertion timing is based on video frame keys and frame period,
> consequently
> > pcr period precision is limited to +/- one video frame period.
> >
> > Signed-off-by: Predrag Filipovic <agoracsinc at gmail.com>
> > ---
> > libavformat/mpegtsenc.c | 80
> +++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 61 insertions(+), 19 deletions(-)
> >
> > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> > index 7656720..7ed9076 100644
> > --- a/libavformat/mpegtsenc.c
> > +++ b/libavformat/mpegtsenc.c
> > @@ -105,6 +105,7 @@ typedef struct MpegTSWrite {
> > int tables_version;
> > double pat_period;
> > double sdt_period;
> > + int64_t last_pcr_ts;
> > int64_t last_pat_ts;
> > int64_t last_sdt_ts;
> >
> > @@ -903,6 +904,9 @@ static int mpegts_init(AVFormatContext *s)
> > ts_st = pcr_st->priv_data;
> >
> > if (ts->mux_rate > 1) {
> > + if (ts->pcr_period >= INT_MAX/2) {
> > + ts->pcr_period = PCR_RETRANS_TIME;
> > + }
>
> Currently pcr_period defaults are handled differently from
> pat_period and sdt_period
> after this patch its still different, why ?
>
> ts->sdt_packet_period and ts->pat_packet_period are initiaized to
> defaults, and disabled later in case of user provided parameters
>
> for pcr_period the user provided value is overridden (this is a bit
> ugly IMHO) and pcr_packet_period set to the user provided value if any
> and later only conditionally overriden in the VBR case
>
> why do you not change pcr_packet_period / pcr_period to work like
> pat_period & sdt_period ?
>
---
See NOTE 1, 3 at the start of Email
PCR override follows the same concept as overrides for pat and sdt, it was
just placed "hire up" where CBR and VBR cases are already split.
Yes, its ugly, should I move it down (same cluster as pat, sdt ?
---
>
> > 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 /
> > @@ -931,10 +935,19 @@ static int mpegts_init(AVFormatContext *s)
> > service->pcr_packet_period =
> > ts_st->user_tb.den / (10 * ts_st->user_tb.num);
> > }
> > - if (!service->pcr_packet_period)
> > + /* if pcr_period specified, mark pcr_packet_period as NA
> (=INT_MAX) */
> > + if (ts->pcr_period < INT_MAX/2) {
> > + service->pcr_packet_period = INT_MAX;
> > + } else {
> > + if (!service->pcr_packet_period) {
> > service->pcr_packet_period = 1;
> > + } else if (service->pcr_packet_period == INT_MAX) {
> > + service->pcr_packet_period--;
> > + }
> > + }
> > }
> >
>
> > + ts->last_pcr_ts = AV_NOPTS_VALUE;
>
> this looks wrong, i suspect this should be a loop over all services
> and service->last_pcr_ts = AV_NOPTS_VALUE;
>
> its ok to change this in a seperate patch of corse if thats cleaner
>
---
In current code, none of pcr_* varaibles should be a part of struct
MpegTSService
since these are used for muxing only (mpegtsenc.c only). These pcr_*
variables
should be moved (for current code) to struct MpegTSWrite like pat_* and
sdt_* vars.
"ts->last_pcr_ts = AV_NOPTS_VALUE;" here follows last_pat/sdt structure
---
>
>
> > ts->last_pat_ts = AV_NOPTS_VALUE;
> > ts->last_sdt_ts = AV_NOPTS_VALUE;
> > // The user specified a period, use only it
> > @@ -1032,10 +1045,9 @@ static void
> mpegts_insert_null_packet(AVFormatContext *s)
> > avio_write(s->pb, buf, TS_PACKET_SIZE);
> > }
> >
> > -/* Write a single transport stream packet with a PCR and no payload */
> > -static void mpegts_insert_pcr_only(AVFormatContext *s, AVStream *st)
> > +/* Write a single transport stream packet with a PCR (value in arg) and
> no payload */
> > +static void mpegts_insert_pcr_only(AVFormatContext *s, AVStream *st,
> int64_t pcr)
> > {
> > - MpegTSWrite *ts = s->priv_data;
> > MpegTSWriteStream *ts_st = st->priv_data;
> > uint8_t *q;
> > uint8_t buf[TS_PACKET_SIZE];
> > @@ -1050,7 +1062,7 @@ static void mpegts_insert_pcr_only(AVFormatContext
> *s, AVStream *st)
> > *q++ = 0x10; /* Adaptation flags: PCR present */
> >
> > /* PCR coded into 6 bytes */
> > - q += write_pcr_bits(q, get_pcr(ts, s->pb));
> > + q += write_pcr_bits(q, pcr);
> >
> > /* stuffing bytes */
> > memset(q, 0xFF, TS_PACKET_SIZE - (q - buf));
> > @@ -1109,6 +1121,9 @@ static uint8_t *get_ts_payload_start(uint8_t *pkt)
> > * number of TS packets. The final TS packet is padded using an
> oversized
> > * adaptation header to exactly fill the last TS packet.
> > * NOTE: 'payload' contains a complete PES payload. */
> > +/* PCR insertion for VBR TS is based on video frames time and key frames
> > + * which leaves non-video TS with PCR insertion at key frames only.
> > + * NOTE: PCR period "precision" for VBR TS is +/- one video frame
> period. */
> > static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> > const uint8_t *payload, int payload_size,
> > int64_t pts, int64_t dts, int key)
>
> > @@ -1135,26 +1150,53 @@ static void mpegts_write_pes(AVFormatContext *s,
> AVStream *st,
> >
> > write_pcr = 0;
> > if (ts_st->pid == ts_st->service->pcr_pid) {
> > - if (ts->mux_rate > 1 || is_start) // VBR pcr period is
> based on frames
> > + if (ts->mux_rate > 1 || is_start)
>
> > - ts_st->service->pcr_packet_count++;
> > + if (ts_st->service->pcr_packet_period != INT_MAX)
> ts_st->service->pcr_packet_count++;
>
> looks unneeded
>
---
See next comment (same issue)
---
>
>
> > if (ts_st->service->pcr_packet_count >=
> > - ts_st->service->pcr_packet_period) {
> > + ts_st->service->pcr_packet_period) { /* case is NA for
> VBR TS with specified pcr period*/
> > ts_st->service->pcr_packet_count = 0;
> > - write_pcr = 1;
> > + if (ts_st->service->pcr_packet_period != INT_MAX)
> write_pcr = 1;
> > }
>
> looks unneeded as well
>
> these cases wont trigger
> nor would it matter if they do trigger, like one PCR more
> every 2000 gb
>
> they complicate the logic thugh as INT_MAX becomes a special case
> ---
> If we take worst case scenario, 80Mbps stream (5.32e+04 packets per sec)
> on 32 bit machine
> (INT_MAX = 2^31), and calling function did not specify pcr_period
> (assigned INT_MAX), we could
> get unintended PCR insertion every 11 hours. This might be too "cautious"
> and, yes, I agree,
> we should use dedicated flag "pcr_period not specified" (same for pat and
> sdt) instead of
> check against INT_MAX
> Should I change per above for patch re-submission ?
> ---
>
>
> > }
> >
> > + if (ts->mux_rate > 1) {
> > + pcr = get_pcr(ts, s->pb);
> > + } else {
> > + pcr = (dts - delay) * 300;
> > + }
> > + if (pcr < 0) {
> > + av_log(s, AV_LOG_WARNING, "calculated pcr < 0, TS is
> invalid\n");
> > + pcr = 0;
> > + }
> > +
> > if (ts->mux_rate > 1 && dts != AV_NOPTS_VALUE &&
> > - (dts - get_pcr(ts, s->pb) / 300) > delay) {
> > + (dts - pcr / 300) > delay) {
> > /* pcr insert gets priority over null packet insert */
> > if (write_pcr)
> > - mpegts_insert_pcr_only(s, st);
> > + mpegts_insert_pcr_only(s, st, pcr);
> > else
> > mpegts_insert_null_packet(s);
> > /* recalculate write_pcr and possibly retransmit si_info */
> > continue;
> > }
>
> can changes to the pcr value be split into a seperate patch from
> changes to the periods when pcrs are inserted ?
>
---
Certainly. Will do
---
>
>
> >
> > + /* Insert PCR for VBR TS with specified pcr_period based on
> video frame time */
> > + if ( (ts->mux_rate <= 1) && (st->codec->codec_type ==
> AVMEDIA_TYPE_VIDEO)
> > + && (ts_st->service->pcr_packet_period == INT_MAX) )
>
> why is this VBR specific ?
>
> if pcr_period is setup like sdt_period/pat_period and the VBR check
> is removed, wouldnt that "just work" and be consistent with sdt/pat ?
>
---
See NOTE 1, 3 at the start of Email
---
>
>
> > + {
> > + if ( (dts != AV_NOPTS_VALUE && ts->last_pcr_ts ==
> AV_NOPTS_VALUE) ||
> > + (dts != AV_NOPTS_VALUE && (dts - delay -
> ts->last_pcr_ts) >= ts->pcr_period*90) )
> > + {
> > + ts->last_pcr_ts = pcr / 300;
> > + ts_st->service->pcr_packet_count = 0;
> > + if (ts_st->pid != ts_st->service->pcr_pid) {
> > + mpegts_insert_pcr_only(s, st, pcr);
> > + continue;
> > + }
> > + write_pcr = 1;
> > + }
> > + }
> > +
> > /* prepare packet header */
> > q = buf;
> > *q++ = 0x47;
> > @@ -1166,20 +1208,20 @@ static void mpegts_write_pes(AVFormatContext *s,
> AVStream *st,
> > ts_st->cc = ts_st->cc + 1 & 0xf;
> > *q++ = 0x10 | ts_st->cc; // payload indicator + CC
> > if (key && is_start && pts != AV_NOPTS_VALUE) {
> > - // set Random Access for key frames
> > - if (ts_st->pid == ts_st->service->pcr_pid)
> > + if (ts_st->pid == ts_st->service->pcr_pid) {
> > write_pcr = 1;
>
> > + if ( (ts->mux_rate <= 1) &&
> (ts_st->service->pcr_packet_period == INT_MAX) ) {
>
> why would this just be done for VBR ?
>
> of course if you like to do a patch that does all changes just for VBR
> and then seperately enable it all for CBR in a 2nd patch thats perfectly
> ok but i think the code shouldnt be full of VBR special cases
>
---
See NOTE 1, 3 at the start of Email
---
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The bravest are surely those who have the clearest vision
> of what is before them, glory and danger alike, and yet
> notwithstanding go out to meet it. -- Thucydides
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
More information about the ffmpeg-devel
mailing list