Regarding "pcr is different but once the value is correct, mechanism is the
same ... pat/sdt"
Correct. These are (pcr vs other) or can be identical for CBR. Identical
(mechanisms) is OK
for VBR (as a matter of principle) but one could also have simpler
implementation
for pat/sdt since their lower bound is flexible (and no need to constantly
check pcr value).
In general, I will do per you suggestions (as soon as I get some "breathing
space" ...)
Regards,
Predrag Filipovic
On Mon, Mar 28, 2016 at 9:23 PM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:
> 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
> <mich...@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 <agoracs...@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
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
