[FFmpeg-devel,RFC,GSoC,v2,1/6] avformat/abr: Adaptive Bitrate support

From: spartazhc <spartazhc@gmail.com>

Add abr module for hls/dash.

v1 fixed:
1. add an "ff" prefix to the protocol name to mark it internal.
2. use 1.2f for float constant 1.2.
3. simplify abr_seek for we just need AVSEEK_SIZE only.

v2 fixed:
1. fix error return
2. simplify abr_seek

Signed-off-by: spartazhc <spartazhc@gmail.com>
---
 doc/protocols.texi      |   7 ++
 libavformat/Makefile    |   1 +
 libavformat/abr.c       | 249 ++++++++++++++++++++++++++++++++++++++++
 libavformat/protocols.c |   1 +
 4 files changed, 258 insertions(+)
 create mode 100644 libavformat/abr.c

On 7/16/2020 9:51 AM, Hongcheng Zhong wrote:
> From: spartazhc <spartazhc@gmail.com>
> 
> Add av_packet_buffer_filter to remove AVPackets whose stream_index is not
> in st_index list. st_index has length of AVMEDIA_TYPE_NB, contains
> the stream_index of all these media types.
> 
> Generally s->internal->packet_buffer may have pkts from different
> stream, and stream_index will be used to discard pkt that is not
> needed. But in case of abr, several streams may pass the stream_index
> check. So we need a function to remove AVPackets not needed in pktl
> added by hls_read_header.
> 
> v1 fixed:
> 1. rename function name *_clean to *_filter
> 2. fix memory leak in ff_packet_buffer_filter
> 3. update the doc
> 
> Signed-off-by: spartazhc <spartazhc@gmail.com>
> ---
>  libavformat/avformat.h | 12 ++++++++++
>  libavformat/internal.h | 16 +++++++++++++
>  libavformat/utils.c    | 54 ++++++++++++++++++++++++++++++++++++++++++
>  libavformat/version.h  |  2 +-
>  4 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index e91e7f1d33..c796fd0391 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -2474,6 +2474,18 @@ int avformat_seek_file(AVFormatContext *s, int stream_index, int64_t min_ts, int
>   */
>  int avformat_flush(AVFormatContext *s);
>  
> +/**
> + * Filter the packet buffer list of the AVFormatContext, remove the AVPackets
> + * do not need according to st_index.

"Not needed", here and below.

> + * Only filter the packet_buffer list.
> + *
> + * @param s media file handle
> + * @param st_index the stream_index list which is needed
> + *        st_index has length of AVMEDIA_TYPE_NB
> + *        in index AVMEDIA_TYPE_XXX contains the stream_index needed of type XXX

From an API user PoV, what does this function do? What is a "packet
buffer list"? How do i know what this does and how do i know when i need it?

> + */
> +int av_packet_buffer_filter(AVFormatContext *s, int *st_index);

No, av_packet_* is a libavcodec namespace for AVPacket helpers. You
can't use it here.

> +
>  /**
>   * Start playing a network-based stream (e.g. RTSP stream) at the
>   * current position.
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 17a6ab07d3..58ebcb2e35 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -772,6 +772,22 @@ int ff_packet_list_get(AVPacketList **head, AVPacketList **tail,
>   */
>  void ff_packet_list_free(AVPacketList **head, AVPacketList **tail);
>  
> +/**
> + * Remove the AVPackets do not need in the packet buffer list.
> + * For each type in AVMediaType, at most keep one stream and
> + * the others will be removed.
> + *
> + * @param head List head element
> + * @param tail List tail element
> + * @param st_index the stream_index list which is needed
> + *        st_index has length of AVMEDIA_TYPE_NB
> + *        in index AVMEDIA_TYPE_XXX contains the stream_index needed of type XXX
> + * @return 0 on success. Success is guaranteed
> + *         if the packet list is not empty.
> + */
> +int ff_packet_buffer_filter(AVPacketList **head, AVPacketList **tail,
> +                            int *st_index);
> +
>  void avpriv_register_devices(const AVOutputFormat * const o[], const AVInputFormat * const i[]);
>  
>  #endif /* AVFORMAT_INTERNAL_H */
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 807d9f10cb..7674e4ea3d 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1565,6 +1565,60 @@ int ff_packet_list_get(AVPacketList **pkt_buffer,
>      return 0;
>  }
>  
> +/**
> + * return 1 if needed
> + */
> +static int ff_check_st_index(int st, int *st_index)

Static functions don't use the ff_ prefix.

> +{
> +    for (int i = 0; i < AVMEDIA_TYPE_NB; ++i) {
> +        if (st_index[i] == st)
> +            return 1;
> +    }
> +    return 0;
> +}
> +
> +int ff_packet_buffer_filter(AVPacketList **pkt_buffer,
> +                         AVPacketList **pkt_buffer_end,
> +                         int           *st_index)
> +{
> +    AVPacketList *pktl, *pktn;
> +    av_assert0(*pkt_buffer);
> +    pktl = *pkt_buffer;
> +    pktn = pktl->next;
> +
> +    /* num >= 2 */
> +    while (pktn) {
> +        if (!ff_check_st_index(pktn->pkt.stream_index, st_index)) {
> +            av_packet_unref(&pktn->pkt);
> +            pktl->next = pktn->next;
> +            av_freep(&pktn);
> +            pktn = pktl->next;
> +        } else {
> +            pktl = pktn;
> +            pktn = pktn->next;
> +        }
> +    }
> +    *pkt_buffer_end = pktl;
> +    /* first one*/
> +    pktl = *pkt_buffer;
> +    if (!ff_check_st_index(pktl->pkt.stream_index, st_index)) {
> +        av_packet_unref(&pktl->pkt);
> +        *pkt_buffer = pktl->next;
> +        if (!pktl->next)
> +            *pkt_buffer_end = NULL;
> +        av_freep(&pktl);
> +    }

Why are you not using the existing packet list helpers for this?

> +
> +    return 0;
> +}
> +
> +int av_packet_buffer_filter(AVFormatContext *s, int *st_index)
> +{
> +    int ret = ff_packet_buffer_filter(&s->internal->packet_buffer,
> +                                   &s->internal->packet_buffer_end, st_index);
> +    return ret;
> +}
> +
>  static int64_t ts_to_samples(AVStream *st, int64_t ts)
>  {
>      return av_rescale(ts, st->time_base.num * st->codecpar->sample_rate, st->time_base.den);
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 75c03fde0a..c17727cf73 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -33,7 +33,7 @@
>  // Also please add any ticket numbers that you believe might be affected here
>  #define LIBAVFORMAT_VERSION_MAJOR  58
>  #define LIBAVFORMAT_VERSION_MINOR  48
> -#define LIBAVFORMAT_VERSION_MICRO 100
> +#define LIBAVFORMAT_VERSION_MICRO 101
>  
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>                                                 LIBAVFORMAT_VERSION_MINOR, \
>

Hongcheng Zhong (12020-07-16):
> From: spartazhc <spartazhc@gmail.com>
> 
> Add abr module for hls/dash.
> 
> v1 fixed:

> 1. add an "ff" prefix to the protocol name to mark it internal.

Then the file name should be changed the same way.

> 2. use 1.2f for float constant 1.2.
> 3. simplify abr_seek for we just need AVSEEK_SIZE only.
> 
> v2 fixed:
> 1. fix error return
> 2. simplify abr_seek
> 
> Signed-off-by: spartazhc <spartazhc@gmail.com>
> ---
>  doc/protocols.texi      |   7 ++
>  libavformat/Makefile    |   1 +
>  libavformat/abr.c       | 249 ++++++++++++++++++++++++++++++++++++++++
>  libavformat/protocols.c |   1 +
>  4 files changed, 258 insertions(+)
>  create mode 100644 libavformat/abr.c
> 
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index 64ad3f05d6..ffbb36147e 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -51,6 +51,13 @@ in microseconds.
>  
>  A description of the currently available protocols follows.
>  

> +@section abr

If it is internal, should it be documented in the user documentation?

Anyway, forgot to change the name here.

> +
> +Adaptive bitrate sub-protocol work for hls/dash.
> +
> +The abr protocol takes stream information from hls/dash as input,
> +use bandwidth estimation to decide whether to switch or not.
> +
>  @section amqp
>  
>  Advanced Message Queueing Protocol (AMQP) version 0-9-1 is a broker based
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 26af859a28..7d74e45d2a 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -595,6 +595,7 @@ OBJS-$(CONFIG_CACHE_PROTOCOL)            += cache.o
>  OBJS-$(CONFIG_CONCAT_PROTOCOL)           += concat.o
>  OBJS-$(CONFIG_CRYPTO_PROTOCOL)           += crypto.o
>  OBJS-$(CONFIG_DATA_PROTOCOL)             += data_uri.o
> +OBJS-$(CONFIG_FFABR_PROTOCOL)            += abr.o
>  OBJS-$(CONFIG_FFRTMPCRYPT_PROTOCOL)      += rtmpcrypt.o rtmpdigest.o rtmpdh.o
>  OBJS-$(CONFIG_FFRTMPHTTP_PROTOCOL)       += rtmphttp.o
>  OBJS-$(CONFIG_FILE_PROTOCOL)             += file.o
> diff --git a/libavformat/abr.c b/libavformat/abr.c
> new file mode 100644
> index 0000000000..7699b9baef
> --- /dev/null
> +++ b/libavformat/abr.c
> @@ -0,0 +1,249 @@
> +/*
> + * Adaptive Bitrate Module for HLS / DASH
> + * Copyright (c) 2020 Hongcheng Zhong
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "avformat.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/time.h"
> +#include "libavutil/avstring.h"
> +#include "url.h"

> +#include <math.h>

System headers usually come before project headers.

> +
> +enum ABRFormatType {
> +    ABR_TYPE_HLS,
> +    ABR_TYPE_DASH
> +};
> +

> +typedef struct variant_bitrate {

FFmpeg's convention is to use capitals in type names.

> +    int value;
> +    int index;

Do these need to be negative? If not, then let us avoid one round of
fuzzing finding an integer overflow and make them unsigned. Same for
other similar cases.

> +} variant_bitrate;
> +
> +typedef struct ABRContext {
> +    AVClass *class;
> +    URLContext *hd;
> +    AVDictionary *abr_params;
> +    AVDictionary *abr_metadata;
> +    enum ABRFormatType format;
> +    int cur_pls;
> +    int can_switch;
> +    int n_variants;
> +    variant_bitrate *variants_bitrate;
> +    int index;
> +    int n_throughputs;
> +    float *throughputs;
> +} ABRContext;
> +

> +static float harmonic_mean(int num, float* arr)

Pointer marks belong with the variable, not with the type.

The of arrays is usually passed after the array, not before.

The size should be size_t, especially since you use size_t later.

> +{
> +    float tmp = 0;
> +

> +    if (num <= 0) return 0;

If num <= 0 is not an acceptable value, then an assert is the correct
way of dealing with it. This would just make debugging harder.

> +

> +    for (size_t i = 0; i < num; i++) {
> +        tmp += 1 / arr[i];
> +    }
> +
> +    return num / tmp;
> +}
> +
> +static int hls_param_parse(ABRContext *c, const char *key, const char *value)
> +{
> +    if (!av_strcasecmp(key, "cur_pls")) {

> +        c->cur_pls = atoi(value);

Please do not use atoi() in new code.

> +    } else if (!av_strcasecmp(key, "can_switch")) {
> +        c->can_switch = atoi(value);
> +    } else if (!av_strcasecmp(key, "n_variants")) {
> +        c->n_variants = atoi(value);

> +        c->variants_bitrate = av_mallocz(sizeof(variant_bitrate) * c->n_variants);

Possible overflow.

> +        if (!c->variants_bitrate)
> +            return AVERROR(ENOMEM);

> +    } else if (av_strstart(key, "variant_bitrate", NULL)) {

Is the suffix meaningless?

> +        c->variants_bitrate[c->index].value = atoi(value);
> +        c->variants_bitrate[c->index].index = c->index;
> +        c->index++;

Is there a check somewhere to guarantee that the array is large enough?

> +    } else if (!av_strcasecmp(key, "n_throughputs")) {
> +        c->n_throughputs = atoi(value);
> +        c->index = 0;

> +        if (c->n_throughputs > 0) {

Is a negative value acceptable? If not, it should be rejected.

> +            c->throughputs = av_malloc(sizeof(float) * c->n_throughputs);

sizeof(var) rather than sizeof(type).

Possible overflow.

> +            if (!c->throughputs)
> +                return AVERROR(ENOMEM);
> +        }

> +    } else if (av_strstart(key, "throughputs", NULL))
> +        c->throughputs[c->index++] = atof(value);

Nit: include braces for the last case too.

> +    return 0;
> +}
> +
> +static int dash_param_parse(ABRContext *c, const char *key, const char *value)
> +{
> +    return 0;
> +}
> +
> +static int abr_param_parse(ABRContext *c, enum ABRFormatType type, const char *key, const char *value)
> +{
> +    if (type == ABR_TYPE_HLS) {
> +        hls_param_parse(c, key, value);
> +    } else if (type == ABR_TYPE_DASH) {

> +        dash_param_parse(c, key, value);

Is is really necessary?

> +    }
> +    return 0;
> +}
> +
> +static int compare_vb(const void *a, const void *b)
> +{
> +    variant_bitrate *a1 = (variant_bitrate *)a;
> +    variant_bitrate *a2 = (variant_bitrate *)b;

> +    return (*a2).value - (*a1).value;

FFDIFFSIGN() to avoid overflow.

> +}
> +
> +static int abr_rule(ABRContext *c, float bw_estimate)
> +{
> +    int ret = -1;
> +
> +    if (c->n_throughputs > 6) {
> +        if (bw_estimate < c->variants_bitrate[c->cur_pls].value / 1000 * 1.2f &&
> +            bw_estimate > c->variants_bitrate[c->cur_pls].value / 1000 * 0.8f)
> +            return -1;
> +        qsort(c->variants_bitrate, c->n_variants, sizeof(variant_bitrate), compare_vb);
> +        for (int i = 0; i < c->n_variants; i++) {
> +            if (bw_estimate > c->variants_bitrate[i].value / 1000) {
> +                ret = i;
> +                break;
> +            }
> +        }
> +    }
> +    if (ret == c->cur_pls)
> +        ret = -1;

> +    av_log(c, AV_LOG_VERBOSE, "[switch] bwe=%.2fkbps, cur=%d, switch=%d\n", bw_estimate, c->cur_pls, ret);

Log to public context, not private context.

> +    return ret;
> +}
> +
> +static int abr_open(URLContext *h, const char *uri, int flags, AVDictionary **options)
> +{
> +    const char *nested_url;
> +    uint64_t start, end;
> +    float bw_estimation;
> +    int switch_request = -1;
> +    int ret = 0;
> +    ABRContext *c = h->priv_data;
> +
> +    if (!av_strstart(uri, "ffabr+", &nested_url) &&
> +        !av_strstart(uri, "ffabr:", &nested_url)) {
> +        av_log(h, AV_LOG_ERROR, "Unsupported url %s\n", uri);
> +        ret = AVERROR(EINVAL);
> +        goto err;
> +    }
> +
> +    AVDictionaryEntry *en = NULL;
> +    en = av_dict_get(c->abr_params, "", en, AV_DICT_IGNORE_SUFFIX);

> +    if (!en)
> +        return -1;

Proper error code please.

> +    if (!av_strcasecmp(en->key, "format")) {
> +        if (!av_strcasecmp(en->value, "hls")) {
> +            c->format = ABR_TYPE_HLS;
> +        } else if (!av_strcasecmp(en->value, "dash")) {
> +            c->format = ABR_TYPE_DASH;
> +        }
> +        av_log(h, AV_LOG_VERBOSE, "%s is using ABR\n", en->value);
> +    }
> +
> +    while (en = av_dict_get(c->abr_params, "", en, AV_DICT_IGNORE_SUFFIX)) {
> +        if (abr_param_parse(c, c->format, en->key, en->value) < 0)
> +            av_log(h, AV_LOG_WARNING,"Error parsing option '%s = %s'.\n",
> +                    en->key, en->value);
> +    }
> +
> +    start = av_gettime();
> +    if ((ret = ffurl_open_whitelist(&c->hd, nested_url, flags,
> +                                    &h->interrupt_callback, options,
> +                                    h->protocol_whitelist, h->protocol_blacklist, h)) < 0) {
> +        av_log(h, AV_LOG_ERROR, "Unable to open resource: %s\n", nested_url);

> +        goto err;

Why? There is no common cleanup.

> +    }
> +    end = av_gettime();
> +
> +    bw_estimation = harmonic_mean(c->n_throughputs, c->throughputs);
> +
> +    if (c->can_switch == 1)
> +        switch_request = abr_rule(c, bw_estimation);
> +
> +    av_dict_set_int(&c->abr_metadata, "download_time", (end - start), 0);
> +    av_dict_set_int(&c->abr_metadata, "switch_request", switch_request, 0);
> +
> +err:
> +    return ret;
> +}
> +
> +
> +static int abr_read(URLContext *h, uint8_t *buf, int size)
> +{
> +    ABRContext *c = h->priv_data;
> +
> +    return ffurl_read(c->hd, buf, size);
> +}
> +
> +static int64_t abr_seek(URLContext *h, int64_t pos, int whence)
> +{
> +    ABRContext *c = h->priv_data;

> +    int64_t ret = 0;
> +
> +    if (whence == AVSEEK_SIZE) {
> +        ret = ffurl_seek(c->hd, pos, AVSEEK_SIZE);
> +    }
> +
> +    return ret;

It seems it always succeeds. Strange.

> +}
> +
> +static int abr_close(URLContext *h)
> +{
> +    ABRContext *c = h->priv_data;
> +    int ret = 0;
> +
> +    ffurl_closep(&c->hd);
> +    av_free(c->variants_bitrate);
> +    av_free(c->throughputs);
> +    return ret;
> +}
> +
> +#define OFFSET(x) offsetof(ABRContext, x)
> +#define D AV_OPT_FLAG_DECODING_PARAM
> +static const AVOption ffabr_options[] = {
> +    { "abr-params",  "Informations ABR needed, using a :-separated list of key=value parameters", OFFSET(abr_params), AV_OPT_TYPE_DICT, { 0 }, 0, 0, D },
> +    { "abr-metadata",  "Metadata return from abr, including switch signal and network bandwidth", OFFSET(abr_metadata), AV_OPT_TYPE_DICT, { 0 }, 0, 0, D },
> +    { NULL }
> +};
> +
> +static const AVClass ffabr_class = {
> +    .class_name = "ffabr",
> +    .item_name  = av_default_item_name,
> +    .option     = ffabr_options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
> +const URLProtocol ff_ffabr_protocol = {
> +    .name            = "ffabr",
> +    .url_open2       = abr_open,
> +    .url_read        = abr_read,
> +    .url_seek        = abr_seek,
> +    .url_close       = abr_close,
> +    .priv_data_size  = sizeof(ABRContext),
> +    .priv_data_class = &ffabr_class,
> +};
> diff --git a/libavformat/protocols.c b/libavformat/protocols.c
> index 7df18fbb3b..1d6af8e380 100644
> --- a/libavformat/protocols.c
> +++ b/libavformat/protocols.c
> @@ -29,6 +29,7 @@ extern const URLProtocol ff_cache_protocol;
>  extern const URLProtocol ff_concat_protocol;
>  extern const URLProtocol ff_crypto_protocol;
>  extern const URLProtocol ff_data_protocol;
> +extern const URLProtocol ff_ffabr_protocol;
>  extern const URLProtocol ff_ffrtmpcrypt_protocol;
>  extern const URLProtocol ff_ffrtmphttp_protocol;
>  extern const URLProtocol ff_file_protocol;

Regards,