| Message ID | 20190804203010.24977-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Sunday, 4 de August de 2019 22:30, Marton Balint <cus@passwd.hu> wrote: > PCR generation was based on counting packets for both CBR and VBR streams. > Couting packets might have worked for CBR streams (when muxrate was specified) > but it only took into account the packets of a service (or the packets of the > PCR stream lately), so even that was problematic for multi program streams. > > The new code works on actual PCR for CBR and packet DTS values for VBR streams, > so the default 20ms PCR retransmission time is now respected for both CBR and > VBR. > I do some tests with this patch and the previous https://patchwork.ffmpeg.org/patch/14210/ And the result is that the repetition rate of PCR is inconsistent: it's never constant and varies between different PCR streams. But fortunately I have an idea... > Signed-off-by: Marton Balint cus@passwd.hu > > libavformat/mpegtsenc.c | 60 +++++++++++++++---------------------------------- > tests/ref/lavf/ts | 2 +- > 2 files changed, 19 insertions(+), 43 deletions(-) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c [...] > @@ -1194,18 +1168,24 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > > is_start = 1; > while (payload_size > 0) { > + int64_t pcr = -1; /* avoid warning */ > + > retransmit_si_info(s, force_pat, dts); > force_pat = 0; > > write_pcr = 0; > - if (ts_st->pcr_packet_period) { > - if (ts->mux_rate > 1 || is_start) // VBR pcr period is based on frames > - ts_st->pcr_packet_count++; > - if (ts_st->pcr_packet_count >= > - ts_st->pcr_packet_period) { > - ts_st->pcr_packet_count = 0; > - write_pcr = 1; > + if (ts_st->pcr_period) { > + if (ts->mux_rate > 1) { > + pcr = get_pcr(ts, s->pb); > + if (pcr - ts_st->last_pcr >= ts_st->pcr_period) > + write_pcr = 1; > + } else if (dts != AV_NOPTS_VALUE) { > + pcr = (dts - delay) * 300; > + if (pcr - ts_st->last_pcr >= ts_st->pcr_period && is_start) > + write_pcr = 1; > } > + if (write_pcr) > + ts_st->last_pcr = FFMAX(pcr - ts_st->pcr_period, ts_st->last_pcr + ts_st->pcr_period); > } IMHO, here you return to make the same mistake of the previous code: only consider one PCR stream. Instead of the "if (ts_st->pcr_period)" it's required to iterate over all PCR streams and do the corresponding checks. And if some PCR stream has exceeded the pcr_period limit then enforce to write an empty TS packet with a PCR mark. In fact, the root cause is the sequential PES writing on the MPEG-TS muxer. And this is the reason for my interleaving mux patch: https://patchwork.ffmpeg.org/patch/13745/ However, until I rebase your code for the interleave mux mode, the PCR generation of this patch needs to be fixed. [...] Regards. A.H. ---
On Mon, 5 Aug 2019, Andreas Håkon wrote: > Hi Marton, > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Sunday, 4 de August de 2019 22:30, Marton Balint <cus@passwd.hu> wrote: > >> PCR generation was based on counting packets for both CBR and VBR streams. >> Couting packets might have worked for CBR streams (when muxrate was specified) >> but it only took into account the packets of a service (or the packets of the >> PCR stream lately), so even that was problematic for multi program streams. >> >> The new code works on actual PCR for CBR and packet DTS values for VBR streams, >> so the default 20ms PCR retransmission time is now respected for both CBR and >> VBR. >> > > I do some tests with this patch and the previous https://patchwork.ffmpeg.org/patch/14210/ > > And the result is that the repetition rate of PCR is inconsistent: it's never > constant and varies between different PCR streams. Are you testing CRB or VBR? For VBR you get PCR at the start of every packet with the command line you provided, so PCR interval should be constant. DVB Inspector always shows the streams as if they were CBR, so the PCR/PTS/DTS view is not showing the intervals properly for VBR. >> + int64_t pcr = -1; /* avoid warning */ >> + >> retransmit_si_info(s, force_pat, dts); >> force_pat = 0; >> >> write_pcr = 0; >> - if (ts_st->pcr_packet_period) { >> - if (ts->mux_rate > 1 || is_start) // VBR pcr period is based on frames >> - ts_st->pcr_packet_count++; >> - if (ts_st->pcr_packet_count >= >> - ts_st->pcr_packet_period) { >> - ts_st->pcr_packet_count = 0; >> - write_pcr = 1; >> + if (ts_st->pcr_period) { >> + if (ts->mux_rate > 1) { >> + pcr = get_pcr(ts, s->pb); >> + if (pcr - ts_st->last_pcr >= ts_st->pcr_period) >> + write_pcr = 1; >> + } else if (dts != AV_NOPTS_VALUE) { >> + pcr = (dts - delay) * 300; >> + if (pcr - ts_st->last_pcr >= ts_st->pcr_period && is_start) >> + write_pcr = 1; >> } >> + if (write_pcr) >> + ts_st->last_pcr = FFMAX(pcr - ts_st->pcr_period, ts_st->last_pcr + ts_st->pcr_period); >> } > > IMHO, here you return to make the same mistake of the previous code: > only consider one PCR stream. Instead of the "if (ts_st->pcr_period)" it's > required to iterate over all PCR streams and do the corresponding checks. > And if some PCR stream has exceeded the pcr_period limit then enforce to > write an empty TS packet with a PCR mark. For CBR, I agree, even if the extra PCR packets increase the bitrate slightly. I can try implementing this. I am not sure what are we trying to achieve for VBR though. Since you don't know the bitrate you can't sensibly calculate PCR for non-start packets. So I believe for VBR the code works as it is supposed to. Regards, Marton
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, 6 de August de 2019 0:20, Marton Balint <cus@passwd.hu> wrote: > On Mon, 5 Aug 2019, Andreas Håkon wrote: > > > Hi Marton, > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > > On Sunday, 4 de August de 2019 22:30, Marton Balint cus@passwd.hu wrote: > > > > > PCR generation was based on counting packets for both CBR and VBR streams. > > > Couting packets might have worked for CBR streams (when muxrate was specified) > > > but it only took into account the packets of a service (or the packets of the > > > PCR stream lately), so even that was problematic for multi program streams. > > > The new code works on actual PCR for CBR and packet DTS values for VBR streams, > > > so the default 20ms PCR retransmission time is now respected for both CBR and > > > VBR. > > > > I do some tests with this patch and the previous https://patchwork.ffmpeg.org/patch/14210/ > > And the result is that the repetition rate of PCR is inconsistent: it's never > > constant and varies between different PCR streams. > > Are you testing CRB or VBR? For VBR you get PCR at the start of every > packet with the command line you provided, so PCR interval should be > constant. DVB Inspector always shows the streams as if they were CBR, so > the PCR/PTS/DTS view is not showing the intervals properly for VBR. Ummmm... I'm testing with and without "-muxrate". However, I feel you're right and every time I check it with VBR the results are wrong. I'll continue testing it. > > > + int64_t pcr = -1; /* avoid warning */ > > > + retransmit_si_info(s, force_pat, dts); > > > force_pat = 0; > > > write_pcr = 0; > > > + if (ts_st->pcr_packet_period) { > > > + if (ts->mux_rate > 1 || is_start) // VBR pcr period is based on frames > > > + ts_st->pcr_packet_count++; > > > + if (ts_st->pcr_packet_count >= > > > + ts_st->pcr_packet_period) { > > > + ts_st->pcr_packet_count = 0; > > > + write_pcr = 1; > > > + if (ts_st->pcr_period) { > > > + if (ts->mux_rate > 1) { > > > + pcr = get_pcr(ts, s->pb); > > > + if (pcr - ts_st->last_pcr >= ts_st->pcr_period) > > > + write_pcr = 1; > > > + } else if (dts != AV_NOPTS_VALUE) { > > > + pcr = (dts - delay) * 300; > > > + if (pcr - ts_st->last_pcr >= ts_st->pcr_period && is_start) > > > + write_pcr = 1; > > > } > > > + if (write_pcr) > > > + ts_st->last_pcr = FFMAX(pcr - ts_st->pcr_period, ts_st->last_pcr + ts_st->pcr_period); > > > } > > > > > > > IMHO, here you return to make the same mistake of the previous code: > > only consider one PCR stream. Instead of the "if (ts_st->pcr_period)" it's > > required to iterate over all PCR streams and do the corresponding checks. > > And if some PCR stream has exceeded the pcr_period limit then enforce to > > write an empty TS packet with a PCR mark. > > For CBR, I agree, even if the extra PCR packets increase the bitrate > slightly. I can try implementing this. The increase in additional packages is negligible. PCR in empty TS packets is a common practice. So don't worry about it. > I am not sure what are we trying to achieve for VBR though. Since you > don't know the bitrate you can't sensibly calculate PCR for non-start > packets. So I believe for VBR the code works as it is supposed to. My concern is about CBR streams (used for broadcasting), and not VBR. So I prefer to fix the issue with such streams. However, I feel you're right with VBR: the code works. And futhermore, when my interleaving patch will be applied then the problem will be less problematic. The reason will be that then each stream will be processed more or less at the same time in parallel. Therefore, PCR calculations in a VBR streams can only have minimum deviations. Regards. A.H. ---
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 02651308f8..a501807711 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -237,10 +237,9 @@ typedef struct MpegTSWriteStream { int payload_flags; uint8_t *payload; AVFormatContext *amux; - AVRational user_tb; - int pcr_packet_count; - int pcr_packet_period; + int pcr_period; + int64_t last_pcr; /* For Opus */ int opus_queued_samples; @@ -792,32 +791,9 @@ static void enable_pcr_generation_for_stream(AVFormatContext *s, AVStream *pcr_s MpegTSWrite *ts = s->priv_data; MpegTSWriteStream *ts_st = pcr_st->priv_data; - if (ts->mux_rate > 1) { - ts_st->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / - (TS_PACKET_SIZE * 8 * 1000); - } else { - if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { - int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0); - if (!frame_size) { - av_log(s, AV_LOG_WARNING, "frame size not set\n"); - ts_st->pcr_packet_period = - pcr_st->codecpar->sample_rate / (10 * 512); - } else { - ts_st->pcr_packet_period = - pcr_st->codecpar->sample_rate / (10 * frame_size); - } - } else { - // max delta PCR 0.1s - // TODO: should be avg_frame_rate - ts_st->pcr_packet_period = - ts_st->user_tb.den / (10 * ts_st->user_tb.num); - } - if (!ts_st->pcr_packet_period) - ts_st->pcr_packet_period = 1; - } - + ts_st->pcr_period = av_rescale(ts->pcr_period, PCR_TIME_BASE, 1000); // output a PCR as soon as possible - ts_st->pcr_packet_count = ts_st->pcr_packet_period; + ts_st->last_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE) - ts_st->pcr_period; } static void select_pcr_streams(AVFormatContext *s) @@ -907,7 +883,6 @@ static int mpegts_init(AVFormatContext *s) } st->priv_data = ts_st; - ts_st->user_tb = st->time_base; avpriv_set_pts_info(st, 33, 1, 90000); ts_st->payload = av_mallocz(ts->pes_payload_size); @@ -1183,7 +1158,6 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, uint8_t *q; int val, is_start, len, header_len, write_pcr, is_dvb_subtitle, is_dvb_teletext, flags; int afc_len, stuffing_len; - int64_t pcr = -1; /* avoid warning */ int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE); int force_pat = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && key && !ts_st->prev_payload_key; @@ -1194,18 +1168,24 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, is_start = 1; while (payload_size > 0) { + int64_t pcr = -1; /* avoid warning */ + retransmit_si_info(s, force_pat, dts); force_pat = 0; write_pcr = 0; - if (ts_st->pcr_packet_period) { - if (ts->mux_rate > 1 || is_start) // VBR pcr period is based on frames - ts_st->pcr_packet_count++; - if (ts_st->pcr_packet_count >= - ts_st->pcr_packet_period) { - ts_st->pcr_packet_count = 0; - write_pcr = 1; + if (ts_st->pcr_period) { + if (ts->mux_rate > 1) { + pcr = get_pcr(ts, s->pb); + if (pcr - ts_st->last_pcr >= ts_st->pcr_period) + write_pcr = 1; + } else if (dts != AV_NOPTS_VALUE) { + pcr = (dts - delay) * 300; + if (pcr - ts_st->last_pcr >= ts_st->pcr_period && is_start) + write_pcr = 1; } + if (write_pcr) + ts_st->last_pcr = FFMAX(pcr - ts_st->pcr_period, ts_st->last_pcr + ts_st->pcr_period); } if (ts->mux_rate > 1 && dts != AV_NOPTS_VALUE && @@ -1236,7 +1216,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, } if (key && is_start && pts != AV_NOPTS_VALUE) { // set Random Access for key frames - if (ts_st->pcr_packet_period) + if (ts_st->pcr_period) write_pcr = 1; set_af_flag(buf, 0x40); q = get_ts_payload_start(buf); @@ -1245,10 +1225,6 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, set_af_flag(buf, 0x10); q = get_ts_payload_start(buf); // add 11, pcr references the last byte of program clock reference base - if (ts->mux_rate > 1) - pcr = get_pcr(ts, s->pb); - else - pcr = (dts - delay) * 300; if (dts != AV_NOPTS_VALUE && dts < pcr / 300) av_log(s, AV_LOG_WARNING, "dts < pcr, TS is invalid\n"); extend_af(buf, write_pcr_bits(q, pcr)); diff --git a/tests/ref/lavf/ts b/tests/ref/lavf/ts index 09960f84d3..8644c260e3 100644 --- a/tests/ref/lavf/ts +++ b/tests/ref/lavf/ts @@ -1,3 +1,3 @@ -38f4b14d43a0e416be8d598628997cbc *tests/data/lavf/lavf.ts +3eee8c2ef073ecdb2b85f40bb9b2e652 *tests/data/lavf/lavf.ts 407020 tests/data/lavf/lavf.ts tests/data/lavf/lavf.ts CRC=0x71287e25
PCR generation was based on counting packets for both CBR and VBR streams. Couting packets might have worked for CBR streams (when muxrate was specified) but it only took into account the packets of a service (or the packets of the PCR stream lately), so even that was problematic for multi program streams. The new code works on actual PCR for CBR and packet DTS values for VBR streams, so the default 20ms PCR retransmission time is now respected for both CBR and VBR. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/mpegtsenc.c | 60 +++++++++++++++---------------------------------- tests/ref/lavf/ts | 2 +- 2 files changed, 19 insertions(+), 43 deletions(-)