[FFmpeg-devel] [PATCH] mpegts: pcr period option for variable bitrate multiplexing

Michael Niedermayer michael at niedermayer.cc
Tue Mar 29 03:23:11 CEST 2016


On Mon, Mar 28, 2016 at 07:58:29PM -0400, Predrag Filipovic wrote:
> 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.

please correct me if iam wrong but
pcr is different but when the value stored is correct for the
point where it is stored then its basically the same.


[...]
> 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
> ...).

that would be great


> 
> I'll split patches per your suggestions and also include other recommended
> changes
> (see inline).

please do
more comments below 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 ?
> ---

if you can make it more similar to the pat/sdt case then please do


> 
> >
> > >          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 ?
> > ---

something like a named constant like PERIOD_UNLIMITED could be used


Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160329/33d0ffa8/attachment.sig>


More information about the ffmpeg-devel mailing list