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

Nicolas George george at nsup.org
Fri Jul 17 17:36:03 EEST 2020


Hongcheng Zhong (12020-07-16):
> 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.

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 at 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.
>  

> + at 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,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200717/a9711715/attachment.sig>


More information about the ffmpeg-devel mailing list