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

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Thu Jul 16 18:45:49 EEST 2020


Hongcheng Zhong:
> From: spartazhc <spartazhc at 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
> 

The commit message should describe what this commit does; the version
history should not be part of the commit message and should be added

> Signed-off-by: spartazhc <spartazhc at gmail.com>
> ---

below the --- here.

>  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.
>  
> + at section abr
> +
> +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>
> +
> +enum ABRFormatType {
> +    ABR_TYPE_HLS,
> +    ABR_TYPE_DASH
> +};
> +
> +typedef struct variant_bitrate {
> +    int value;
> +    int index;
> +} variant_bitrate;
> +
> +typedef struct ABRContext {
> +    AVClass *class;

const AVClass *

> +    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)

const float *arr

> +{
> +    float tmp = 0;
> +
> +    if (num <= 0) return 0;
> +
> +    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);

The behaviour of atoi is undefined if the parsed value is too big. Use
something else. (Furthermore, you might also error out on something like
"42bar" where atoi would just return 42.)

> +    } 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);

Missing overflow check.

> +        if (!c->variants_bitrate)
> +            return AVERROR(ENOMEM);
> +    } else if (av_strstart(key, "variant_bitrate", NULL)) {
> +        c->variants_bitrate[c->index].value = atoi(value);
> +        c->variants_bitrate[c->index].index = c->index;
> +        c->index++;

This presumes that the "n_variants" entry is returned earlier than the
first variant_bitrate entry; and it presumes that the number of
variant_bitrate entries is <= n_variants. You must not rely on this.

I suggest to modify this to the following:

en = av_dict_get(dict, "n_variants", NULL, 0);
/* parse value and allocate c->variants_bitrate */
en = NULL;
while (en = av_dict_get(dict, "variant_bitrate", en,
AV_DICT_IGNORE_SUFFIX)) {
/* parse variant_bitrate value */
}

Notice that this enables you to remove c->index (it can then be a local
variable). Of course hls_param_parse() will have to receive an AVDict
for this.

Also notice that neither the current code nor my suggested alternative
ensure that the index of a variant_bitrate in the variants_bitrate array
coincides with the number after "variant_bitrate". If you want this (and
I don't know how your caller is supposed to interpret the "switch"
parameter without this, so you should want this), you have to parse the
value after "variant_bitrate" and make sure that it is < n_variants. If
a variant bitrate of 0 is invalid (I guess it is), then one also needs
to check for this and make sure that all entries of the variant_bitrate
array are actually set (by counting the number of set entries).

> +    } else if (!av_strcasecmp(key, "n_throughputs")) {
> +        c->n_throughputs = atoi(value);
> +        c->index = 0;
> +        if (c->n_throughputs > 0) {
> +            c->throughputs = av_malloc(sizeof(float) * c->n_throughputs);
> +            if (!c->throughputs)
> +                return AVERROR(ENOMEM);
> +        }
> +    } else if (av_strstart(key, "throughputs", NULL))
> +        c->throughputs[c->index++] = atof(value);

Basically the same as above also goes for throughputs. You should also
zero-initialize throughputs.

> +    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);

You are not forwarding errors from this function.

> +    } else if (type == ABR_TYPE_DASH) {
> +        dash_param_parse(c, key, value);
> +    }
> +    return 0;
> +}
> +
> +static int compare_vb(const void *a, const void *b)
> +{
> +    variant_bitrate *a1 = (variant_bitrate *)a;
> +    variant_bitrate *a2 = (variant_bitrate *)b;

You are unnecessarily casting const away here.

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

You can just use the -> operator here.

> +}
> +
> +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);
> +    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;

FFmpeg uses the old C90 standard (plus a few extensions) where all
variables have to be declared at the beginning of a block. You should
get a compiler warning for this.

> +    en = av_dict_get(c->abr_params, "", en, AV_DICT_IGNORE_SUFFIX);

The function argument en is known to be NULL at this point, so this
known value should be used directly.

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

Return a proper error code, not -1.

> +    if (!av_strcasecmp(en->key, "format")) {

You seem to presume that if there is a format dict-entry, it must be the
first one. This is not so. If you want to know whether an entry for
"format" exists, you have to explicitly search for it:

en = av_dict_get(c->abr_params, "format", NULL, 0);

> +        if (!av_strcasecmp(en->value, "hls")) {
> +            c->format = ABR_TYPE_HLS;
> +        } else if (!av_strcasecmp(en->value, "dash")) {
> +            c->format = ABR_TYPE_DASH;
> +        }

Is it really intended to not error out in case it is neither "hls" nor
"dash"? Currently ABR_TYPE_HLS is the default.

> +        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 the first entry of the dictionary is something else than "format",
then the current code ignores it here.

> +        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;
> +    }
> +    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:

If I am not mistaken, a protocol has to clean up after itself if its
url_open/url_open2 function fails. You don't do this here. The allocated
arrays may leak.

> +    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);

You could just return ffurl_seek() directly here.

> +    }

And what about the other possible values for whence?

> +
> +    return ret;
> +}
> +
> +static int abr_close(URLContext *h)
> +{
> +    ABRContext *c = h->priv_data;
> +    int ret = 0;

This variable is unnecessary.

> +
> +    ffurl_closep(&c->hd);
> +    av_free(c->variants_bitrate);
> +    av_free(c->throughputs);

Use av_freep() in order to avoid leaving stale pointers in memory.

> +    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;
> 



More information about the ffmpeg-devel mailing list