diff mbox series

[FFmpeg-devel,2/3] avformat/hlsenc: Use AVBPrint to avoid allocations of strings

Message ID 20200408134807.19154-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/3] avformat/hlsenc: Fix memleak when deleting old segments | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork warning Failed to apply patch

Commit Message

Andreas Rheinhardt April 8, 2020, 1:48 p.m. UTC
when deleting old segments.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Further todos for this function:
1. Allocating vtt_dirname_r in order to get vtt_dirname is actually a
waste. One would need a dirname to AVBPrint function to avoid it.
2. replace_*_data_in_filename already work with AVBPrints internally.
Refactor them to allow to directly output to a caller-supplied AVBPrint 
and use it to avoid allocating dirname_repl.

 libavformat/hlsenc.c | 57 +++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

Comments

Steven Liu April 8, 2020, 2:15 p.m. UTC | #1
> 2020年4月8日 下午9:48,Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道:
> 
> when deleting old segments.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Further todos for this function:
> 1. Allocating vtt_dirname_r in order to get vtt_dirname is actually a
> waste. One would need a dirname to AVBPrint function to avoid it.
> 2. replace_*_data_in_filename already work with AVBPrints internally.
> Refactor them to allow to directly output to a caller-supplied AVBPrint 
> and use it to avoid allocating dirname_repl.
> 
> libavformat/hlsenc.c | 57 +++++++++++++++++++++-----------------------
> 1 file changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 0f1f558590..d7562ad20c 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -489,18 +489,20 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
> 
>     HLSSegment *segment, *previous_segment = NULL;
>     float playlist_duration = 0.0f;
> -    int ret = 0, path_size, sub_path_size;
> +    int ret = 0;
>     int segment_cnt = 0;
> -    char *dirname = NULL, *sub_path;
> +    AVBPrint path;
> +    char *dirname = NULL;
>     char *dirname_r = NULL;
>     char *dirname_repl = NULL;
> -    char *path = NULL;
>     char *vtt_dirname = NULL;
>     char *vtt_dirname_r = NULL;
>     AVDictionary *options = NULL;
>     AVIOContext *out = NULL;
>     const char *proto = NULL;
> 
> +    av_bprint_init(&path, 0, AV_BPRINT_SIZE_UNLIMITED);
> +
>     segment = vs->segments;
>     while (segment) {
>         playlist_duration += segment->duration;
> @@ -550,73 +552,68 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
>     while (segment) {
>         av_log(hls, AV_LOG_DEBUG, "deleting old segment %s\n",
>                segment->filename);
> -        path_size =  (hls->use_localtime_mkdir ? 0 : strlen(dirname)+1) + strlen(segment->filename) + 1;
> -        path = av_malloc(path_size);
> -        if (!path) {
> +        if (!hls->use_localtime_mkdir) // segment->filename contains basename only
> +            av_bprintf(&path, "%s%c", dirname, SEPARATOR);
> +        av_bprintf(&path, "%s", segment->filename);
> +
> +        if (!av_bprint_is_complete(&path)) {
>             ret = AVERROR(ENOMEM);
>             goto fail;
>         }
> 
> -        if (hls->use_localtime_mkdir)
> -            av_strlcpy(path, segment->filename, path_size);
> -        else { // segment->filename contains basename only
> -            snprintf(path, path_size, "%s%c%s", dirname, SEPARATOR, segment->filename);
> -        }
> -
>         proto = avio_find_protocol_name(s->url);
>         if (hls->method || (proto && !av_strcasecmp(proto, "http"))) {
>             av_dict_set(&options, "method", "DELETE", 0);
> -            if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0) {
> +            if ((ret = vs->avf->io_open(vs->avf, &out, path.str,
> +                                        AVIO_FLAG_WRITE, &options)) < 0) {
>                 if (hls->ignore_io_errors)
>                     ret = 0;
>                 goto fail;
>             }
>             ff_format_io_close(vs->avf, &out);
> -        } else if (unlink(path) < 0) {
> +        } else if (unlink(path.str) < 0) {
>             av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
> -                   path, strerror(errno));
> +                   path.str, strerror(errno));
>         }
> 
>         if ((segment->sub_filename[0] != '\0')) {
>             vtt_dirname_r = av_strdup(vs->vtt_avf->url);
>             vtt_dirname = (char*)av_dirname(vtt_dirname_r);
> -            sub_path_size = strlen(segment->sub_filename) + 1 + strlen(vtt_dirname) + 1;
> -            sub_path = av_malloc(sub_path_size);
> -            if (!sub_path) {
> +
> +            av_bprint_clear(&path);
> +            av_bprintf(&path, "%s%c%s", vtt_dirname, SEPARATOR,
> +                                         segment->sub_filename);
> +            av_freep(&vtt_dirname_r);
> +
> +            if (!av_bprint_is_complete(&path)) {
>                 ret = AVERROR(ENOMEM);
>                 goto fail;
>             }
> 
> -            snprintf(sub_path, sub_path_size, "%s%c%s", vtt_dirname, SEPARATOR, segment->sub_filename);
> -
> -            av_freep(&vtt_dirname_r);
> -
>             if (hls->method || (proto && !av_strcasecmp(proto, "http"))) {
>                 av_dict_set(&options, "method", "DELETE", 0);
> -                if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, sub_path, AVIO_FLAG_WRITE, &options)) < 0) {
> +                if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, path.str,
> +                                                AVIO_FLAG_WRITE, &options)) < 0) {
>                     if (hls->ignore_io_errors)
>                         ret = 0;
> -                    av_freep(&sub_path);
>                     goto fail;
>                 }
>                 ff_format_io_close(vs->vtt_avf, &out);
> -            } else if (unlink(sub_path) < 0) {
> +            } else if (unlink(path.str) < 0) {
>                 av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
> -                       sub_path, strerror(errno));
> +                       path.str, strerror(errno));
>             }
> -            av_freep(&sub_path);
>         }
> -        av_freep(&path);
> +        av_bprint_clear(&path);
>         previous_segment = segment;
>         segment = previous_segment->next;
>         av_freep(&previous_segment);
>     }
> 
> fail:
> -    av_freep(&path);
> +    av_bprint_finalize(&path, NULL);
>     av_freep(&dirname_r);
>     av_freep(&dirname_repl);
> -    av_freep(&vtt_dirname_r);
> 
>     return ret;
> }
> -- 
> 2.20.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

LGTM

Thanks

Steven Liu
Nicolas George April 8, 2020, 3:02 p.m. UTC | #2
Andreas Rheinhardt (12020-04-08):
> when deleting old segments.

Your commit message got wrapped. ;-)

I do not maintain this part of the code. The use of BPrint seems ok, not
changing the semantic.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Further todos for this function:
> 1. Allocating vtt_dirname_r in order to get vtt_dirname is actually a
> waste. One would need a dirname to AVBPrint function to avoid it.
> 2. replace_*_data_in_filename already work with AVBPrints internally.
> Refactor them to allow to directly output to a caller-supplied AVBPrint 
> and use it to avoid allocating dirname_repl.
> 
>  libavformat/hlsenc.c | 57 +++++++++++++++++++++-----------------------
>  1 file changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 0f1f558590..d7562ad20c 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -489,18 +489,20 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
>  
>      HLSSegment *segment, *previous_segment = NULL;
>      float playlist_duration = 0.0f;
> -    int ret = 0, path_size, sub_path_size;
> +    int ret = 0;
>      int segment_cnt = 0;
> -    char *dirname = NULL, *sub_path;
> +    AVBPrint path;
> +    char *dirname = NULL;
>      char *dirname_r = NULL;
>      char *dirname_repl = NULL;
> -    char *path = NULL;
>      char *vtt_dirname = NULL;
>      char *vtt_dirname_r = NULL;
>      AVDictionary *options = NULL;
>      AVIOContext *out = NULL;
>      const char *proto = NULL;
>  
> +    av_bprint_init(&path, 0, AV_BPRINT_SIZE_UNLIMITED);
> +
>      segment = vs->segments;
>      while (segment) {
>          playlist_duration += segment->duration;
> @@ -550,73 +552,68 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
>      while (segment) {
>          av_log(hls, AV_LOG_DEBUG, "deleting old segment %s\n",
>                 segment->filename);
> -        path_size =  (hls->use_localtime_mkdir ? 0 : strlen(dirname)+1) + strlen(segment->filename) + 1;
> -        path = av_malloc(path_size);
> -        if (!path) {
> +        if (!hls->use_localtime_mkdir) // segment->filename contains basename only
> +            av_bprintf(&path, "%s%c", dirname, SEPARATOR);
> +        av_bprintf(&path, "%s", segment->filename);
> +
> +        if (!av_bprint_is_complete(&path)) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
>          }
>  
> -        if (hls->use_localtime_mkdir)
> -            av_strlcpy(path, segment->filename, path_size);
> -        else { // segment->filename contains basename only
> -            snprintf(path, path_size, "%s%c%s", dirname, SEPARATOR, segment->filename);
> -        }
> -
>          proto = avio_find_protocol_name(s->url);
>          if (hls->method || (proto && !av_strcasecmp(proto, "http"))) {
>              av_dict_set(&options, "method", "DELETE", 0);
> -            if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0) {
> +            if ((ret = vs->avf->io_open(vs->avf, &out, path.str,
> +                                        AVIO_FLAG_WRITE, &options)) < 0) {
>                  if (hls->ignore_io_errors)
>                      ret = 0;
>                  goto fail;
>              }
>              ff_format_io_close(vs->avf, &out);
> -        } else if (unlink(path) < 0) {
> +        } else if (unlink(path.str) < 0) {
>              av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
> -                   path, strerror(errno));
> +                   path.str, strerror(errno));
>          }
>  
>          if ((segment->sub_filename[0] != '\0')) {
>              vtt_dirname_r = av_strdup(vs->vtt_avf->url);
>              vtt_dirname = (char*)av_dirname(vtt_dirname_r);
> -            sub_path_size = strlen(segment->sub_filename) + 1 + strlen(vtt_dirname) + 1;
> -            sub_path = av_malloc(sub_path_size);
> -            if (!sub_path) {
> +
> +            av_bprint_clear(&path);
> +            av_bprintf(&path, "%s%c%s", vtt_dirname, SEPARATOR,
> +                                         segment->sub_filename);
> +            av_freep(&vtt_dirname_r);
> +
> +            if (!av_bprint_is_complete(&path)) {
>                  ret = AVERROR(ENOMEM);
>                  goto fail;
>              }
>  
> -            snprintf(sub_path, sub_path_size, "%s%c%s", vtt_dirname, SEPARATOR, segment->sub_filename);
> -
> -            av_freep(&vtt_dirname_r);
> -
>              if (hls->method || (proto && !av_strcasecmp(proto, "http"))) {
>                  av_dict_set(&options, "method", "DELETE", 0);
> -                if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, sub_path, AVIO_FLAG_WRITE, &options)) < 0) {
> +                if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, path.str,
> +                                                AVIO_FLAG_WRITE, &options)) < 0) {
>                      if (hls->ignore_io_errors)
>                          ret = 0;
> -                    av_freep(&sub_path);
>                      goto fail;
>                  }
>                  ff_format_io_close(vs->vtt_avf, &out);
> -            } else if (unlink(sub_path) < 0) {
> +            } else if (unlink(path.str) < 0) {
>                  av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
> -                       sub_path, strerror(errno));
> +                       path.str, strerror(errno));
>              }
> -            av_freep(&sub_path);
>          }
> -        av_freep(&path);

> +        av_bprint_clear(&path);

I would advise to put this before the first use instead of here: it will
be more robust if somebody adds "continue" in the loop.

>          previous_segment = segment;
>          segment = previous_segment->next;
>          av_freep(&previous_segment);
>      }
>  
>  fail:
> -    av_freep(&path);
> +    av_bprint_finalize(&path, NULL);
>      av_freep(&dirname_r);
>      av_freep(&dirname_repl);
> -    av_freep(&vtt_dirname_r);
>  
>      return ret;
>  }

By the way, some time ago I proposed an API that wraps BPrint into
something a little more elegant and a lot more versatile, designed for
all cases where we have functions that build and return strings. Since
we are focussing on BPrint these days, I would appreciate if you had
input about it:

https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254042.html
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-January/255767.html

Regards,
diff mbox series

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 0f1f558590..d7562ad20c 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -489,18 +489,20 @@  static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
 
     HLSSegment *segment, *previous_segment = NULL;
     float playlist_duration = 0.0f;
-    int ret = 0, path_size, sub_path_size;
+    int ret = 0;
     int segment_cnt = 0;
-    char *dirname = NULL, *sub_path;
+    AVBPrint path;
+    char *dirname = NULL;
     char *dirname_r = NULL;
     char *dirname_repl = NULL;
-    char *path = NULL;
     char *vtt_dirname = NULL;
     char *vtt_dirname_r = NULL;
     AVDictionary *options = NULL;
     AVIOContext *out = NULL;
     const char *proto = NULL;
 
+    av_bprint_init(&path, 0, AV_BPRINT_SIZE_UNLIMITED);
+
     segment = vs->segments;
     while (segment) {
         playlist_duration += segment->duration;
@@ -550,73 +552,68 @@  static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
     while (segment) {
         av_log(hls, AV_LOG_DEBUG, "deleting old segment %s\n",
                segment->filename);
-        path_size =  (hls->use_localtime_mkdir ? 0 : strlen(dirname)+1) + strlen(segment->filename) + 1;
-        path = av_malloc(path_size);
-        if (!path) {
+        if (!hls->use_localtime_mkdir) // segment->filename contains basename only
+            av_bprintf(&path, "%s%c", dirname, SEPARATOR);
+        av_bprintf(&path, "%s", segment->filename);
+
+        if (!av_bprint_is_complete(&path)) {
             ret = AVERROR(ENOMEM);
             goto fail;
         }
 
-        if (hls->use_localtime_mkdir)
-            av_strlcpy(path, segment->filename, path_size);
-        else { // segment->filename contains basename only
-            snprintf(path, path_size, "%s%c%s", dirname, SEPARATOR, segment->filename);
-        }
-
         proto = avio_find_protocol_name(s->url);
         if (hls->method || (proto && !av_strcasecmp(proto, "http"))) {
             av_dict_set(&options, "method", "DELETE", 0);
-            if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0) {
+            if ((ret = vs->avf->io_open(vs->avf, &out, path.str,
+                                        AVIO_FLAG_WRITE, &options)) < 0) {
                 if (hls->ignore_io_errors)
                     ret = 0;
                 goto fail;
             }
             ff_format_io_close(vs->avf, &out);
-        } else if (unlink(path) < 0) {
+        } else if (unlink(path.str) < 0) {
             av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
-                   path, strerror(errno));
+                   path.str, strerror(errno));
         }
 
         if ((segment->sub_filename[0] != '\0')) {
             vtt_dirname_r = av_strdup(vs->vtt_avf->url);
             vtt_dirname = (char*)av_dirname(vtt_dirname_r);
-            sub_path_size = strlen(segment->sub_filename) + 1 + strlen(vtt_dirname) + 1;
-            sub_path = av_malloc(sub_path_size);
-            if (!sub_path) {
+
+            av_bprint_clear(&path);
+            av_bprintf(&path, "%s%c%s", vtt_dirname, SEPARATOR,
+                                         segment->sub_filename);
+            av_freep(&vtt_dirname_r);
+
+            if (!av_bprint_is_complete(&path)) {
                 ret = AVERROR(ENOMEM);
                 goto fail;
             }
 
-            snprintf(sub_path, sub_path_size, "%s%c%s", vtt_dirname, SEPARATOR, segment->sub_filename);
-
-            av_freep(&vtt_dirname_r);
-
             if (hls->method || (proto && !av_strcasecmp(proto, "http"))) {
                 av_dict_set(&options, "method", "DELETE", 0);
-                if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, sub_path, AVIO_FLAG_WRITE, &options)) < 0) {
+                if ((ret = vs->vtt_avf->io_open(vs->vtt_avf, &out, path.str,
+                                                AVIO_FLAG_WRITE, &options)) < 0) {
                     if (hls->ignore_io_errors)
                         ret = 0;
-                    av_freep(&sub_path);
                     goto fail;
                 }
                 ff_format_io_close(vs->vtt_avf, &out);
-            } else if (unlink(sub_path) < 0) {
+            } else if (unlink(path.str) < 0) {
                 av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
-                       sub_path, strerror(errno));
+                       path.str, strerror(errno));
             }
-            av_freep(&sub_path);
         }
-        av_freep(&path);
+        av_bprint_clear(&path);
         previous_segment = segment;
         segment = previous_segment->next;
         av_freep(&previous_segment);
     }
 
 fail:
-    av_freep(&path);
+    av_bprint_finalize(&path, NULL);
     av_freep(&dirname_r);
     av_freep(&dirname_repl);
-    av_freep(&vtt_dirname_r);
 
     return ret;
 }