[FFmpeg-devel] [PATCH] avformat/mpegts.c: reduce buffering during initialization

[Marton Balint]

On Mon, 4 Mar 2019, andriy.gelman at gmail.com wrote:

> From: Andriy Gelman <andriy.gelman at 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