[FFmpeg-devel] [PATCH] avformat/hls: Reduce memory footprint by using dynamic allocated url/key

[Anssi Hannula]

09.04.2014 08:44, Schenk, Michael kirjoitti:
> Hi!

Hi,

> many thanks for your help. I hope this time the patch (format) is correctly.
> 
> Feedback welcome

Feedback below:

> 0001-reduce-memory-by-using-dynamic-allocated-url-key.patch
> 
> 
> From 31b3fc2923c4c31ccf2f385e38eba1b33336ddf1 Mon Sep 17 00:00:00 2001
> From: Michael Schenk <michael.schenk at albistechnologies.com>
> Date: Wed, 9 Apr 2014 07:39:37 +0200
> Subject: [PATCH] reduce memory by using dynamic allocated url/key
> 
> ---
>  libavformat/hls.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 0b4b58d..5907260 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -68,8 +68,8 @@ struct segment {
>      int64_t duration;
>      int64_t url_offset;
>      int64_t size;
> -    char url[MAX_URL_SIZE];
> -    char key[MAX_URL_SIZE];
> +    char *url;
> +    char *key;
>      enum KeyType key_type;
>      uint8_t iv[16];
>  };
> @@ -192,8 +192,13 @@ static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
>  static void free_segment_list(struct playlist *pls)
>  {
>      int i;
> -    for (i = 0; i < pls->n_segments; i++)
> +
> +    for (i = 0; i < pls->n_segments; i++) {
> +        av_free(pls->segments[i]->key);
> +        av_free(pls->segments[i]->url);
>          av_free(pls->segments[i]);
> +    }
> +
>      av_freep(&pls->segments);
>      pls->n_segments = 0;
>  }
> @@ -504,6 +509,11 @@ static int parse_playlist(HLSContext *c, const char *url,
>      int64_t seg_size = -1;
>      uint8_t *new_url = NULL;
>      struct variant_info variant_info;
> +    char tmp_key_url[MAX_URL_SIZE];
> +    char tmp_url[MAX_URL_SIZE];
> +    int tmp_len;
> +    
> +    key[0] = '\0';

Isn't key already an empty string?

>      if (!in) {
>          AVDictionary *opts = NULL;
> @@ -624,8 +634,26 @@ static int parse_playlist(HLSContext *c, const char *url,
>                      memset(seg->iv, 0, sizeof(seg->iv));
>                      AV_WB32(seg->iv + 12, seq);
>                  }
> -                ff_make_absolute_url(seg->key, sizeof(seg->key), url, key);
> -                ff_make_absolute_url(seg->url, sizeof(seg->url), url, line);
> +
> +                seg->key = seg->url = NULL;

No need to set seg->url to NULL here, it is set always below.

> +                tmp_len = strlen(key);
> +
> +                if (tmp_len != 0) {

You can replace this check with "if (key_type != KEY_NONE)".

> +                    ff_make_absolute_url(tmp_key_url, sizeof(tmp_key_url), url, key);
> +                    tmp_len = strlen(tmp_key_url);
> +                    seg->key = av_malloc(tmp_len + 1);
> +                    strcpy(seg->key, tmp_key_url);

Use av_strdup :)

> +                } else {
> +                    seg->key = av_malloc(sizeof(char));
> +                    seg->key[0] = '\0';

You can drop this allocation altogether if you check for KEY_NONE above,
since seg->key is never accessed if key_type == KEY_NONE.
You can also move "seg->key = NULL" here from above.

> +                }
> +
> +                ff_make_absolute_url(tmp_url, sizeof(tmp_url), url, line);
> +                tmp_len = strlen(tmp_url);
> +                seg->url = av_malloc(tmp_len + 1);
> +                strcpy(seg->url, tmp_url);

Same for av_strdup here.

Also, you are missing return value checks for the allocation, you'll
need to fail with AVERROR(ENOMEM) in that case.

>                  dynarray_add(&pls->segments, &pls->n_segments, seg);
>                  is_segment = 0;
>  

Thanks for looking into this.

-- 
Anssi Hannula