Hongcheng Zhong (12020-07-16): > From: spartazhc <sparta...@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 <sparta...@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,
--
Nicolas George
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
