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 ?
> 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
> 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
> 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 (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 ?
>
> + /* 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 ?
> + {
> + 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
[...]
--
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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
