| Message ID | 20190305032101.18661-1-andriy.gelman@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 4 Mar 2019, andriy.gelman@gmail.com wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > Reduces buffering during estimation of mpegts raw_packet_size > parameter. Instead of buffering a fixed 8192 bytes, calculate > probe scores on a smaller buffer. Increase buffer size until > probe score is greater than minimum value. What is the issue that you are seeing and that this patch fix? Is it only matters for low bitrate mpegts? (like 64 kbps streams?) Or does this improve high bitrate streams as well somehow? Thanks, Marton
> What is the issue that you are seeing and that this patch fix? Is it only > matters for low bitrate mpegts? (like 64 kbps streams?) Or does this > improve high bitrate streams as well somehow? I'm muxing hevc video + very low bitrate binary messages into mpegts. If the hevc video is not present, I would have to buffer 8192 bytes of binary messages to get past mpegts_read_header, which was creating a large latency. This patch reduces the startup time. As you mentioned, it does not apply to high bitrate streams. Also in git history, I saw that the size of the buffer was changed in ad-hoc way from 1024 -> 5*1024-> 8 * 1024 because I assume probing failed in some cases. I feel that the proposed patch would be a better solution as it follows the same ideas when probing for unknown parameters in other parts of the code. Regards, Andriy On Tue, 5 Mar 2019 at 11:59, Marton Balint <cus@passwd.hu> wrote: > > > On Mon, 4 Mar 2019, andriy.gelman@gmail.com wrote: > > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > Reduces buffering during estimation of mpegts raw_packet_size > > parameter. Instead of buffering a fixed 8192 bytes, calculate > > probe scores on a smaller buffer. Increase buffer size until > > probe score is greater than minimum value. > > What is the issue that you are seeing and that this patch fix? Is it only > matters for low bitrate mpegts? (like 64 kbps streams?) Or does this > improve high bitrate streams as well somehow? > > Thanks, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, 4 Mar 2019, andriy.gelman@gmail.com wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > Reduces buffering during estimation of mpegts raw_packet_size > parameter. Instead of buffering a fixed 8192 bytes, calculate > probe scores on a smaller buffer. Increase buffer size until > probe score is greater than minimum value. Please extend the commit message with the justification for this change, e.g. "reduces buffering latency with low bitrate streams, where 8192 bytes can mean several seconds" > --- > libavformat/mpegts.c | 82 +++++++++++++++++++++++++++++++------------- > 1 file changed, 59 insertions(+), 23 deletions(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index b04fd7b4f4..a7b33eae69 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -53,6 +53,10 @@ > (prev_dividend) = (dividend); \ > } while (0) > > +#define MAX_RAW_PACKET_PROBE 8192 > +#define PROBE_PACKET_STEP 512 > +#define RAW_PACKET_MIN_SCORE 10 > + > enum MpegTSFilterType { > MPEGTS_PES, > MPEGTS_SECTION, > @@ -591,28 +595,64 @@ static int analyze(const uint8_t *buf, int size, int packet_size, > return best_score - FFMAX(stat_all - 10*best_score, 0)/10; > } > > -/* autodetect fec presence. Must have at least 1024 bytes */ > -static int get_packet_size(const uint8_t *buf, int size) > +/* autodetect fec presence */ > +static int get_packet_size(AVIOContext* pb) > { > int score, fec_score, dvhs_score; > + int pd_packet_size = TS_PACKET_SIZE; The initial value can be 0 or AVERROR_INVALIDDATA, because calling code falls back to this anyway and reports the issue. > + int best_score = 0; > + int ret; > > - if (size < (TS_FEC_PACKET_SIZE * 5 + 1)) > - return AVERROR_INVALIDDATA; > + AVProbeData pd = { 0 }; > + while (best_score < RAW_PACKET_MIN_SCORE && > + pd.buf_size + PROBE_PACKET_STEP <= MAX_RAW_PACKET_PROBE) { > + > + /*create extra space for next packet*/ > + uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size + PROBE_PACKET_STEP); > + if (new_buf) { > + pd.buf = new_buf; > + ret = avio_read(pb, pd.buf + pd.buf_size, PROBE_PACKET_STEP); I can see that you are increasing linearly, maybe doubling the bufsize with each step is better, although in this case it probably does not matter too much. On the other hand, the maximum 8K seems small enough to allocate it at the start of the function instead of reallocing it with each step. > + if (ret < 0) { > + av_log(pb, AV_LOG_ERROR, "Error reading from input: %s.\n", > + av_err2str(ret)); This probably should not report an error for AVERROR_EOF, mpegts files less than 8192 bytes can be valid. > + break; > + } > + pd.buf_size += ret; > + } else > + goto fail; > + > + /*check score for each fec packet size*/ > + score = analyze(pd.buf, pd.buf_size, TS_PACKET_SIZE, 0); > + if (score > best_score) { > + best_score = score; > + pd_packet_size = TS_PACKET_SIZE; > + } > > - score = analyze(buf, size, TS_PACKET_SIZE, 0); > - dvhs_score = analyze(buf, size, TS_DVHS_PACKET_SIZE, 0); > - fec_score = analyze(buf, size, TS_FEC_PACKET_SIZE, 0); > - av_log(NULL, AV_LOG_TRACE, "score: %d, dvhs_score: %d, fec_score: %d \n", > - score, dvhs_score, fec_score); > - > - if (score > fec_score && score > dvhs_score) > - return TS_PACKET_SIZE; > - else if (dvhs_score > score && dvhs_score > fec_score) > - return TS_DVHS_PACKET_SIZE; > - else if (score < fec_score && dvhs_score < fec_score) > - return TS_FEC_PACKET_SIZE; > - else > - return AVERROR_INVALIDDATA; > + dvhs_score = analyze(pd.buf, pd.buf_size, TS_DVHS_PACKET_SIZE, 0); > + if (dvhs_score > best_score) { > + best_score = dvhs_score; > + pd_packet_size = TS_DVHS_PACKET_SIZE; > + } > + > + fec_score = analyze(pd.buf, pd.buf_size, TS_FEC_PACKET_SIZE, 0); > + if (fec_score > best_score) { > + best_score = fec_score; > + pd_packet_size = TS_FEC_PACKET_SIZE; > + } You seem to be changing the logic: Old code: report packet size if candidate score is better than other scores. New code: report packet size with first best score no matter if the other scores are the same. Old logic seems less error-prone, so you should determine the winner based on that, and stay in the loop if no winner is found. > + > + av_log(NULL, AV_LOG_TRACE, "Probe size: %d, score: %d, dvhs_score: %d, fec_score: %d \n", > + pd.buf_size, score, dvhs_score, fec_score); > + } > + > + if (pd.buf) > + av_freep(&pd.buf); > + > + return pd_packet_size; > + > +fail: > + if (pd.buf) > + av_freep(&pd.buf); > + return AVERROR(ENOMEM); > } > > typedef struct SectionHeader { > @@ -2841,8 +2881,6 @@ static int mpegts_read_header(AVFormatContext *s) > { > MpegTSContext *ts = s->priv_data; > AVIOContext *pb = s->pb; > - uint8_t buf[8 * 1024] = {0}; > - int len; > int64_t pos, probesize = s->probesize; > > s->internal->prefer_codec_framerate = 1; > @@ -2850,10 +2888,8 @@ static int mpegts_read_header(AVFormatContext *s) > if (ffio_ensure_seekback(pb, probesize) < 0) > av_log(s, AV_LOG_WARNING, "Failed to allocate buffers for seekback\n"); > > - /* read the first 8192 bytes to get packet size */ > pos = avio_tell(pb); > - len = avio_read(pb, buf, sizeof(buf)); > - ts->raw_packet_size = get_packet_size(buf, len); > + ts->raw_packet_size = get_packet_size(pb); > if (ts->raw_packet_size <= 0) { > av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, defaulting to non-FEC/DVHS\n"); > ts->raw_packet_size = TS_PACKET_SIZE; > -- Regards, Marton
On Mon, Mar 04, 2019 at 10:21:01PM -0500, andriy.gelman@gmail.com wrote: > From: Andriy Gelman <andriy.gelman@gmail.com> > > Reduces buffering during estimation of mpegts raw_packet_size > parameter. Instead of buffering a fixed 8192 bytes, calculate > probe scores on a smaller buffer. Increase buffer size until > probe score is greater than minimum value. > --- > libavformat/mpegts.c | 82 +++++++++++++++++++++++++++++++------------- > 1 file changed, 59 insertions(+), 23 deletions(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index b04fd7b4f4..a7b33eae69 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -53,6 +53,10 @@ > (prev_dividend) = (dividend); \ > } while (0) > > +#define MAX_RAW_PACKET_PROBE 8192 > +#define PROBE_PACKET_STEP 512 > +#define RAW_PACKET_MIN_SCORE 10 > + > enum MpegTSFilterType { > MPEGTS_PES, > MPEGTS_SECTION, > @@ -591,28 +595,64 @@ static int analyze(const uint8_t *buf, int size, int packet_size, > return best_score - FFMAX(stat_all - 10*best_score, 0)/10; > } > > -/* autodetect fec presence. Must have at least 1024 bytes */ > -static int get_packet_size(const uint8_t *buf, int size) > +/* autodetect fec presence */ > +static int get_packet_size(AVIOContext* pb) > { > int score, fec_score, dvhs_score; > + int pd_packet_size = TS_PACKET_SIZE; > + int best_score = 0; > + int ret; > > - if (size < (TS_FEC_PACKET_SIZE * 5 + 1)) > - return AVERROR_INVALIDDATA; > + AVProbeData pd = { 0 }; > + while (best_score < RAW_PACKET_MIN_SCORE && > + pd.buf_size + PROBE_PACKET_STEP <= MAX_RAW_PACKET_PROBE) { the end condition is not robust if you have scores 100 and 101, 101 is better but the difference is too small to stop the loop without risk that the other moght be better later the use of AVProbeData does not seem to help this code in any way > + > + /*create extra space for next packet*/ > + uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size + PROBE_PACKET_STEP); the previous code needed no dynamic allocation. Changing that is not needed and would e a seperate issue and require an explanation why. overall this patch looks like it changes alot more than whats needed to run the code with less data in an initial iteration thx [...]
Marton, Michael, thanks for your feedback. I've updated the patch based on your comments. > I can see that you are increasing linearly, maybe doubling the bufsize > with each step is better, although in this case it probably does not > matter too much. > On the other hand, the maximum 8K seems small enough to allocate it at the > start of the function instead of reallocing it with each step. I've allocated the buffer at the start of the function. > You seem to be changing the logic: > Old code: report packet size if candidate score is better than other > scores. > New code: report packet size with first best score no matter if the other > scores are the same. > Old logic seems less error-prone, so you should determine the winner based > on that, and stay in the loop if no winner is found. I've changed the logic to follow the original code. The only difference is that I've added a margin such that the winner is larger by PROBE_PACKET_MARGIN. The margin is set to zero when the buffer size is 8192 bytes, so that the results of new and old code will be the same when buffer is filled, or there is an eof. > the end condition is not robust > if you have scores 100 and 101, 101 is better but the difference is too small > to stop the loop without risk that the other might be better later I've added a margin when comparing the scores. I.e.: if (score > FFMAX(fec_score, dvhs_score) + margin) return TS_PACKET_SIZE; Does the use of margin address your comment? > the use of AVProbeData does not seem to help this code in any way I've removed AVProbeData and added buffer to stack. Regards, Andriy On Tue, 5 Mar 2019 at 19:25, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Mar 04, 2019 at 10:21:01PM -0500, andriy.gelman@gmail.com wrote: > > From: Andriy Gelman <andriy.gelman@gmail.com> > > > > Reduces buffering during estimation of mpegts raw_packet_size > > parameter. Instead of buffering a fixed 8192 bytes, calculate > > probe scores on a smaller buffer. Increase buffer size until > > probe score is greater than minimum value. > > --- > > libavformat/mpegts.c | 82 +++++++++++++++++++++++++++++++------------- > > 1 file changed, 59 insertions(+), 23 deletions(-) > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > index b04fd7b4f4..a7b33eae69 100644 > > --- a/libavformat/mpegts.c > > +++ b/libavformat/mpegts.c > > @@ -53,6 +53,10 @@ > > (prev_dividend) = (dividend); > \ > > } while (0) > > > > +#define MAX_RAW_PACKET_PROBE 8192 > > +#define PROBE_PACKET_STEP 512 > > +#define RAW_PACKET_MIN_SCORE 10 > > + > > enum MpegTSFilterType { > > MPEGTS_PES, > > MPEGTS_SECTION, > > @@ -591,28 +595,64 @@ static int analyze(const uint8_t *buf, int size, > int packet_size, > > return best_score - FFMAX(stat_all - 10*best_score, 0)/10; > > } > > > > -/* autodetect fec presence. Must have at least 1024 bytes */ > > -static int get_packet_size(const uint8_t *buf, int size) > > +/* autodetect fec presence */ > > +static int get_packet_size(AVIOContext* pb) > > { > > int score, fec_score, dvhs_score; > > + int pd_packet_size = TS_PACKET_SIZE; > > + int best_score = 0; > > + int ret; > > > > - if (size < (TS_FEC_PACKET_SIZE * 5 + 1)) > > - return AVERROR_INVALIDDATA; > > + AVProbeData pd = { 0 }; > > + while (best_score < RAW_PACKET_MIN_SCORE && > > + pd.buf_size + PROBE_PACKET_STEP <= MAX_RAW_PACKET_PROBE) { > > the end condition is not robust > if you have scores 100 and 101, 101 is better but the difference is too > small > to stop the loop without risk that the other moght be better later > > the use of AVProbeData does not seem to help this code in any way > > > > + > > + /*create extra space for next packet*/ > > + uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size + > PROBE_PACKET_STEP); > > the previous code needed no dynamic allocation. Changing that is not needed > and would e a seperate issue and require an explanation why. > > overall this patch looks like it changes alot more than whats needed > to run the code with less data in an initial iteration > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > No snowflake in an avalanche ever feels responsible. -- Voltaire > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index b04fd7b4f4..a7b33eae69 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -53,6 +53,10 @@ (prev_dividend) = (dividend); \ } while (0) +#define MAX_RAW_PACKET_PROBE 8192 +#define PROBE_PACKET_STEP 512 +#define RAW_PACKET_MIN_SCORE 10 + enum MpegTSFilterType { MPEGTS_PES, MPEGTS_SECTION, @@ -591,28 +595,64 @@ static int analyze(const uint8_t *buf, int size, int packet_size, return best_score - FFMAX(stat_all - 10*best_score, 0)/10; } -/* autodetect fec presence. Must have at least 1024 bytes */ -static int get_packet_size(const uint8_t *buf, int size) +/* autodetect fec presence */ +static int get_packet_size(AVIOContext* pb) { int score, fec_score, dvhs_score; + int pd_packet_size = TS_PACKET_SIZE; + int best_score = 0; + int ret; - if (size < (TS_FEC_PACKET_SIZE * 5 + 1)) - return AVERROR_INVALIDDATA; + AVProbeData pd = { 0 }; + while (best_score < RAW_PACKET_MIN_SCORE && + pd.buf_size + PROBE_PACKET_STEP <= MAX_RAW_PACKET_PROBE) { + + /*create extra space for next packet*/ + uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size + PROBE_PACKET_STEP); + if (new_buf) { + pd.buf = new_buf; + ret = avio_read(pb, pd.buf + pd.buf_size, PROBE_PACKET_STEP); + if (ret < 0) { + av_log(pb, AV_LOG_ERROR, "Error reading from input: %s.\n", + av_err2str(ret)); + break; + } + pd.buf_size += ret; + } else + goto fail; + + /*check score for each fec packet size*/ + score = analyze(pd.buf, pd.buf_size, TS_PACKET_SIZE, 0); + if (score > best_score) { + best_score = score; + pd_packet_size = TS_PACKET_SIZE; + } - score = analyze(buf, size, TS_PACKET_SIZE, 0); - dvhs_score = analyze(buf, size, TS_DVHS_PACKET_SIZE, 0); - fec_score = analyze(buf, size, TS_FEC_PACKET_SIZE, 0); - av_log(NULL, AV_LOG_TRACE, "score: %d, dvhs_score: %d, fec_score: %d \n", - score, dvhs_score, fec_score); - - if (score > fec_score && score > dvhs_score) - return TS_PACKET_SIZE; - else if (dvhs_score > score && dvhs_score > fec_score) - return TS_DVHS_PACKET_SIZE; - else if (score < fec_score && dvhs_score < fec_score) - return TS_FEC_PACKET_SIZE; - else - return AVERROR_INVALIDDATA; + dvhs_score = analyze(pd.buf, pd.buf_size, TS_DVHS_PACKET_SIZE, 0); + if (dvhs_score > best_score) { + best_score = dvhs_score; + pd_packet_size = TS_DVHS_PACKET_SIZE; + } + + fec_score = analyze(pd.buf, pd.buf_size, TS_FEC_PACKET_SIZE, 0); + if (fec_score > best_score) { + best_score = fec_score; + pd_packet_size = TS_FEC_PACKET_SIZE; + } + + av_log(NULL, AV_LOG_TRACE, "Probe size: %d, score: %d, dvhs_score: %d, fec_score: %d \n", + pd.buf_size, score, dvhs_score, fec_score); + } + + if (pd.buf) + av_freep(&pd.buf); + + return pd_packet_size; + +fail: + if (pd.buf) + av_freep(&pd.buf); + return AVERROR(ENOMEM); } typedef struct SectionHeader { @@ -2841,8 +2881,6 @@ static int mpegts_read_header(AVFormatContext *s) { MpegTSContext *ts = s->priv_data; AVIOContext *pb = s->pb; - uint8_t buf[8 * 1024] = {0}; - int len; int64_t pos, probesize = s->probesize; s->internal->prefer_codec_framerate = 1; @@ -2850,10 +2888,8 @@ static int mpegts_read_header(AVFormatContext *s) if (ffio_ensure_seekback(pb, probesize) < 0) av_log(s, AV_LOG_WARNING, "Failed to allocate buffers for seekback\n"); - /* read the first 8192 bytes to get packet size */ pos = avio_tell(pb); - len = avio_read(pb, buf, sizeof(buf)); - ts->raw_packet_size = get_packet_size(buf, len); + ts->raw_packet_size = get_packet_size(pb); if (ts->raw_packet_size <= 0) { av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, defaulting to non-FEC/DVHS\n"); ts->raw_packet_size = TS_PACKET_SIZE;
From: Andriy Gelman <andriy.gelman@gmail.com> Reduces buffering during estimation of mpegts raw_packet_size parameter. Instead of buffering a fixed 8192 bytes, calculate probe scores on a smaller buffer. Increase buffer size until probe score is greater than minimum value. --- libavformat/mpegts.c | 82 +++++++++++++++++++++++++++++++------------- 1 file changed, 59 insertions(+), 23 deletions(-)