| Message ID | 20190827073447.26947-2-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | f4eb7d84a7c219eac130750417e4bc2cbbff7b3f |
| Headers | show |
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, 27 de August de 2019 9:34, Marton Balint <cus@passwd.hu> wrote: > Fixes DTS < PCR errors with this command line: > > ./ffmpeg -loglevel verbose -y -f lavfi -i \ > "testsrc=s=64x64:d=20,split=2[out0][tmp1];[tmp1]vflip[out1];sine=d=20,asetnsamples=1000[out2]" \ > -flags +bitexact -fflags +bitexact -sws_flags +accurate_rnd+bitexact \ > -codec:v libx264 -codec:a mp2 -b:a 32k -pix_fmt yuv420p \ > -map '0:v:0' \ > -map '0:v:1' \ > -map '0:a:0' \ > -muxrate 800000 \ > -program st=0:st=2 -program st=1:st=2 -program st=2 -program st=0 -f mpegts out1.ts > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index 047961cdea..0678657d09 100644 > --- a/libavformat/mpegtsenc.c > +++ b/libavformat/mpegtsenc.c > [...] This change breaks my interleaved patch (waiting for your review). Please, note that the main problem at time with the mpegts muxer is that all PES packets are written sequentially. And this generates a lot of problems when the video PES packets are large, or when the audio packets aren't flushed at regular intervals. If you prefer to improve the current sequential mode before you do anything with the interleaved mode, then I give this suggestion: Use a PES SIZE INTERVAL for audio packets instead of calculating a TIME DELAY. With CBR audio steams, every audio PES packet has the same payload size. Regards. A.H. ---
On Tue, 27 Aug 2019, Andreas Håkon wrote: > Hi Marton, > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Tuesday, 27 de August de 2019 9:34, Marton Balint <cus@passwd.hu> wrote: > >> Fixes DTS < PCR errors with this command line: >> >> ./ffmpeg -loglevel verbose -y -f lavfi -i \ >> "testsrc=s=64x64:d=20,split=2[out0][tmp1];[tmp1]vflip[out1];sine=d=20,asetnsamples=1000[out2]" \ >> -flags +bitexact -fflags +bitexact -sws_flags +accurate_rnd+bitexact \ >> -codec:v libx264 -codec:a mp2 -b:a 32k -pix_fmt yuv420p \ >> -map '0:v:0' \ >> -map '0:v:1' \ >> -map '0:a:0' \ >> -muxrate 800000 \ >> -program st=0:st=2 -program st=1:st=2 -program st=2 -program st=0 -f mpegts out1.ts > >> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c >> index 047961cdea..0678657d09 100644 >> --- a/libavformat/mpegtsenc.c >> +++ b/libavformat/mpegtsenc.c >> [...] > > This change breaks my interleaved patch (waiting for your review). It will require some changes. I will respond in the proper thread. > > Please, note that the main problem at time with the mpegts muxer is that all PES packets are > written sequentially. And this generates a lot of problems when the video PES packets are large, > or when the audio packets aren't flushed at regular intervals. If you prefer to improve the > current sequential mode before you do anything with the interleaved mode, then I give this > suggestion: Use a PES SIZE INTERVAL for audio packets instead of calculating a TIME DELAY. With > CBR audio steams, every audio PES packet has the same payload size. I am not sure what you mean when you say PES size interval, but if you are referring to the size of the PES packet - that is exactly what we had in the very beginning, and it was not sufficent because for low bitrate streams when combining small audio packets to a PES packet it took too long time, and in order to generate a proper TS we have to make sure that we don't delay the audio packets too much, becuase if we do, then it will arrive at the destination later then the PCR which makes presentation impossible. So that is why timestamp based checks were added, but those checks were not sufficent. This is a completely different issue from interleaving. Interleaving is basically ticket #912. Regards, Marton
Hi Marton, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, 27 de August de 2019 23:33, Marton Balint <cus@passwd.hu> wrote: > > Please, note that the main problem at time with the mpegts muxer is that all PES packets are > > written sequentially. And this generates a lot of problems when the video PES packets are large, > > or when the audio packets aren't flushed at regular intervals. If you prefer to improve the > > current sequential mode before you do anything with the interleaved mode, then I give this > > suggestion: Use a PES SIZE INTERVAL for audio packets instead of calculating a TIME DELAY. With > > CBR audio steams, every audio PES packet has the same payload size. > > I am not sure what you mean when you say PES size interval, but if you are > referring to the size of the PES packet - that is exactly what we had in > the very beginning, and it was not sufficent because for low bitrate > streams when combining small audio packets to a PES packet it took too > long time, and in order to generate a proper TS we have to make sure that > we don't delay the audio packets too much, becuase if we do, then it will > arrive at the destination later then the PCR which makes presentation > impossible. The problem is that you're thinking of using the same pes_size for all audio packets! For each audio stream you need to calculate the correct pes_size. And the value is based on the bitrate. So for CBR audio streams the value is fixed, and you only need to recalculate it for VBR audio streams. Please, try to add some "pes_top_size" member at stream level, and use it for audio streams. You can calculate the value when you know the bitrate, and after that a simple "if ts_st->payload >= ts_st->pes_top_size" will be sufficient to trigger the dispatch of the PES packet. > So that is why timestamp based checks were added, but those checks were > not sufficent. Timestamp checks are required for VBR streams. But you can replace the checks with a function to recalculate the "pes_top_size" of such streams. Regards. A.H. ---
On Wed, 28 Aug 2019, Andreas Håkon wrote: > Hi Marton, > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Tuesday, 27 de August de 2019 23:33, Marton Balint <cus@passwd.hu> wrote: > >> > Please, note that the main problem at time with the mpegts muxer is that all PES packets are >> > written sequentially. And this generates a lot of problems when the video PES packets are large, >> > or when the audio packets aren't flushed at regular intervals. If you prefer to improve the >> > current sequential mode before you do anything with the interleaved mode, then I give this >> > suggestion: Use a PES SIZE INTERVAL for audio packets instead of calculating a TIME DELAY. With >> > CBR audio steams, every audio PES packet has the same payload size. >> >> I am not sure what you mean when you say PES size interval, but if you are >> referring to the size of the PES packet - that is exactly what we had in >> the very beginning, and it was not sufficent because for low bitrate >> streams when combining small audio packets to a PES packet it took too >> long time, and in order to generate a proper TS we have to make sure that >> we don't delay the audio packets too much, becuase if we do, then it will >> arrive at the destination later then the PCR which makes presentation >> impossible. > > The problem is that you're thinking of using the same pes_size for all audio packets! > For each audio stream you need to calculate the correct pes_size. max_pes_size = max_audio_delay * audio_bitrate it is the same thing for CBR, you calculate one from the other. > And the value is based on the bitrate. So for CBR audio streams the value is > fixed, and you only need to recalculate it for VBR audio streams. > > Please, try to add some "pes_top_size" member at stream level, and use it for > audio streams. You can calculate the value when you know the bitrate, and > after that a simple "if ts_st->payload >= ts_st->pes_top_size" will be sufficient > to trigger the dispatch of the PES packet. I don't see how calclating a max_pes_size is superior. It _only_ works for CBR, plus you don't really know the audio bitrate, you'd have to guess it from some frame sample count and audio packet size. What is the disadvantage of always using a timestamp based contraint instead of a sized based? A timestamp based one works for both CBR and VBR, and you don't have to recalculate anything based on some bitrate guessing. Regards, Marton
On Wed, 28 Aug 2019, Marton Balint wrote: > > > On Wed, 28 Aug 2019, Andreas Håkon wrote: > >> Hi Marton, >> >> >> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ >> On Tuesday, 27 de August de 2019 23:33, Marton Balint <cus@passwd.hu> > wrote: >> >>> > Please, note that the main problem at time with the mpegts muxer is that > all PES packets are >>> > written sequentially. And this generates a lot of problems when the > video PES packets are large, >>> > or when the audio packets aren't flushed at regular intervals. If you > prefer to improve the >>> > current sequential mode before you do anything with the interleaved > mode, then I give this >>> > suggestion: Use a PES SIZE INTERVAL for audio packets instead of > calculating a TIME DELAY. With >>> > CBR audio steams, every audio PES packet has the same payload size. >>> >>> I am not sure what you mean when you say PES size interval, but if you are >>> referring to the size of the PES packet - that is exactly what we had in >>> the very beginning, and it was not sufficent because for low bitrate >>> streams when combining small audio packets to a PES packet it took too >>> long time, and in order to generate a proper TS we have to make sure that >>> we don't delay the audio packets too much, becuase if we do, then it will >>> arrive at the destination later then the PCR which makes presentation >>> impossible. >> >> The problem is that you're thinking of using the same pes_size for all > audio packets! >> For each audio stream you need to calculate the correct pes_size. > > max_pes_size = max_audio_delay * audio_bitrate > > it is the same thing for CBR, you calculate one from the other. > >> And the value is based on the bitrate. So for CBR audio streams the value > is >> fixed, and you only need to recalculate it for VBR audio streams. >> >> Please, try to add some "pes_top_size" member at stream level, and use it > for >> audio streams. You can calculate the value when you know the bitrate, and >> after that a simple "if ts_st->payload >= ts_st->pes_top_size" will be > sufficient >> to trigger the dispatch of the PES packet. > > I don't see how calclating a max_pes_size is superior. It _only_ works for > CBR, plus you don't really know the audio bitrate, you'd have to guess it > from some frame sample count and audio packet size. > > What is the disadvantage of always using a timestamp based contraint > instead of a sized based? A timestamp based one works for both CBR and > VBR, and you don't have to recalculate anything based on some bitrate > guessing. Applied this as well. Regards, Marton
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 047961cdea..0678657d09 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -1531,6 +1531,7 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) MpegTSWrite *ts = s->priv_data; MpegTSWriteStream *ts_st = st->priv_data; const int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE) * 2; + const int64_t max_audio_delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE) / 2; int64_t dts = pkt->dts, pts = pkt->pts; int opus_samples = 0; int side_data_size; @@ -1729,25 +1730,9 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) } } - if (pkt->dts != AV_NOPTS_VALUE) { - int i; - for(i=0; i<s->nb_streams; i++) { - AVStream *st2 = s->streams[i]; - MpegTSWriteStream *ts_st2 = st2->priv_data; - if ( ts_st2->payload_size - && (ts_st2->payload_dts == AV_NOPTS_VALUE || dts - ts_st2->payload_dts > delay/2)) { - mpegts_write_pes(s, st2, ts_st2->payload, ts_st2->payload_size, - ts_st2->payload_pts, ts_st2->payload_dts, - ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); - ts_st2->payload_size = 0; - } - } - } - if (ts_st->payload_size && (ts_st->payload_size + size > ts->pes_payload_size || (dts != AV_NOPTS_VALUE && ts_st->payload_dts != AV_NOPTS_VALUE && - av_compare_ts(dts - ts_st->payload_dts, st->time_base, - s->max_delay, AV_TIME_BASE_Q) >= 0) || + dts - ts_st->payload_dts >= max_audio_delay) || ts_st->opus_queued_samples + opus_samples >= 5760 /* 120ms */)) { mpegts_write_pes(s, st, ts_st->payload, ts_st->payload_size, ts_st->payload_pts, ts_st->payload_dts,
7d097a0fc57f0fa8385962a539c657c2f40b5ed0 had the same purpose as 3700f655c55e2001b57215210b957b169d66b50f but the former is much simpler, so let's remove the latter. Unfortunately both checks were wrong, because in order to make sure DTS > PCR we have to give us some headroom, so instead of using a dts_difference < max_delay check let's use a dts_difference < max_delay/2 check. Fixes DTS < PCR errors with this command line: ./ffmpeg -loglevel verbose -y -f lavfi -i \ "testsrc=s=64x64:d=20,split=2[out0][tmp1];[tmp1]vflip[out1];sine=d=20,asetnsamples=1000[out2]" \ -flags +bitexact -fflags +bitexact -sws_flags +accurate_rnd+bitexact \ -codec:v libx264 -codec:a mp2 -b:a 32k -pix_fmt yuv420p \ -map '0:v:0' \ -map '0:v:1' \ -map '0:a:0' \ -muxrate 800000 \ -program st=0:st=2 -program st=1:st=2 -program st=2 -program st=0 -f mpegts out1.ts Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/mpegtsenc.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)