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

[Schenk, Michael]

> -----Original Message-----
> From: ffmpeg-devel-bounces at ffmpeg.org [mailto:ffmpeg-devel-
> bounces at ffmpeg.org] On Behalf Of Anssi Hannula
> Sent: Donnerstag, 10. April 2014 23:34
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avformat/hls: Reduce memory footprint
> by using dynamic allocated url/key
> 
> 09.04.2014 08:44, Schenk, Michael kirjoitti:
> > Hi!
> 
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Many thanks for the feedback. Please find attached a revised version of the patch based on your feedback.

Feedback welcome!

Michael Schenk



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-reduce-memory-by-using-dynamic-allocated-url-key-rev.patch
Type: application/octet-stream
Size: 2679 bytes
Desc: 0001-reduce-memory-by-using-dynamic-allocated-url-key-rev.patch
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140411/b4276370/attachment.obj>