diff mbox series

[FFmpeg-devel,v2] avfilter/formats: optimize ff_all_formats

Message ID 20200103142521.37264-1-quinkblack@foxmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2] avfilter/formats: optimize ff_all_formats
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork fail Times out

Commit Message

zhilizhao Jan. 3, 2020, 2:25 p.m. UTC
From: Zhao Zhili <zhilizhao@tencent.com>

This is a micro-optimization. Saving almost 200 reallocations makes it
worth a try.
---
 libavfilter/formats.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

Comments

Andreas Rheinhardt Jan. 3, 2020, 5:09 p.m. UTC | #1
quinkblack@foxmail.com:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> This is a micro-optimization. Saving almost 200 reallocations makes it
> worth a try.
> ---
>  libavfilter/formats.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 33c64668a0..c04253d898 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -348,21 +348,48 @@ int ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t channel_layout)
>  
>  AVFilterFormats *ff_all_formats(enum AVMediaType type)
>  {
> -    AVFilterFormats *ret = NULL;
> +    int count, i;
> +    AVFilterFormats *ret = av_mallocz(sizeof(*ret));
> +
> +    if (!ret)
> +        return NULL;
>  
>      if (type == AVMEDIA_TYPE_VIDEO) {
>          const AVPixFmtDescriptor *desc = NULL;
> -        while ((desc = av_pix_fmt_desc_next(desc))) {
> -            if (ff_add_format(&ret, av_pix_fmt_desc_get_id(desc)) < 0)
> -                return NULL;
> +
> +        count = 0;
> +        while ((desc = av_pix_fmt_desc_next(desc)))
> +            count++;
> +
> +        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
> +        if (!ret->formats) {
> +            av_free(ret);
> +            return NULL;
> +        }
> +        ret->nb_formats = count;
> +
> +        for (i = 0, desc = NULL; i < count; i++) {
> +            desc = av_pix_fmt_desc_next(desc);
> +            ret->formats[i] = av_pix_fmt_desc_get_id(desc);
>          }
>      } else if (type == AVMEDIA_TYPE_AUDIO) {
>          enum AVSampleFormat fmt = 0;
> -        while (av_get_sample_fmt_name(fmt)) {
> -            if (ff_add_format(&ret, fmt) < 0)
> -                return NULL;
> -            fmt++;
> +
> +        count = 0;
> +        while (av_get_sample_fmt_name(fmt))
> +            count++;

This looks like an infinite loop. (The code you removed incremented
fmt in the loop.)

- Andreas

> +
> +        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
> +        if (!ret->formats) {
> +            av_free(ret);
> +            return NULL;
>          }
> +        ret->nb_formats = count;
> +
> +        for (fmt = 0; fmt < count; fmt++)
> +            ret->formats[fmt] = fmt;
> +    } else {
> +        av_freep(&ret);
>      }
>  
>      return ret;
>
Andriy Gelman Jan. 3, 2020, 5:27 p.m. UTC | #2
On Fri, 03. Jan 22:25, quinkblack@foxmail.com wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> This is a micro-optimization. Saving almost 200 reallocations makes it
> worth a try.
> ---
>  libavfilter/formats.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 33c64668a0..c04253d898 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -348,21 +348,48 @@ int ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t channel_layout)
>  
>  AVFilterFormats *ff_all_formats(enum AVMediaType type)
>  {
> -    AVFilterFormats *ret = NULL;
> +    int count, i;
> +    AVFilterFormats *ret = av_mallocz(sizeof(*ret));
> +
> +    if (!ret)
> +        return NULL;
>  
>      if (type == AVMEDIA_TYPE_VIDEO) {
>          const AVPixFmtDescriptor *desc = NULL;
> -        while ((desc = av_pix_fmt_desc_next(desc))) {
> -            if (ff_add_format(&ret, av_pix_fmt_desc_get_id(desc)) < 0)
> -                return NULL;
> +
> +        count = 0;
> +        while ((desc = av_pix_fmt_desc_next(desc)))
> +            count++;
> +
> +        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
> +        if (!ret->formats) {
> +            av_free(ret);
> +            return NULL;
> +        }
> +        ret->nb_formats = count;
> +
> +        for (i = 0, desc = NULL; i < count; i++) {
> +            desc = av_pix_fmt_desc_next(desc);
> +            ret->formats[i] = av_pix_fmt_desc_get_id(desc);
>          }
>      } else if (type == AVMEDIA_TYPE_AUDIO) {
>          enum AVSampleFormat fmt = 0;
> -        while (av_get_sample_fmt_name(fmt)) {
> -            if (ff_add_format(&ret, fmt) < 0)
> -                return NULL;
> -            fmt++;
> +
> +        count = 0;
> +        while (av_get_sample_fmt_name(fmt))
> +            count++;
> +
> +        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
> +        if (!ret->formats) {
> +            av_free(ret);
> +            return NULL;
>          }
> +        ret->nb_formats = count;
> +
> +        for (fmt = 0; fmt < count; fmt++)
> +            ret->formats[fmt] = fmt;
> +    } else {
> +        av_freep(&ret);
>      }
>  
>      return ret;
> -- 
> 2.22.0
> 

There is somekind of infinite loop here. 
Gets stuck in fate during:

TEST    lavf-mxf
diff mbox series

Patch

diff --git a/libavfilter/formats.c b/libavfilter/formats.c
index 33c64668a0..c04253d898 100644
--- a/libavfilter/formats.c
+++ b/libavfilter/formats.c
@@ -348,21 +348,48 @@  int ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t channel_layout)
 
 AVFilterFormats *ff_all_formats(enum AVMediaType type)
 {
-    AVFilterFormats *ret = NULL;
+    int count, i;
+    AVFilterFormats *ret = av_mallocz(sizeof(*ret));
+
+    if (!ret)
+        return NULL;
 
     if (type == AVMEDIA_TYPE_VIDEO) {
         const AVPixFmtDescriptor *desc = NULL;
-        while ((desc = av_pix_fmt_desc_next(desc))) {
-            if (ff_add_format(&ret, av_pix_fmt_desc_get_id(desc)) < 0)
-                return NULL;
+
+        count = 0;
+        while ((desc = av_pix_fmt_desc_next(desc)))
+            count++;
+
+        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
+        if (!ret->formats) {
+            av_free(ret);
+            return NULL;
+        }
+        ret->nb_formats = count;
+
+        for (i = 0, desc = NULL; i < count; i++) {
+            desc = av_pix_fmt_desc_next(desc);
+            ret->formats[i] = av_pix_fmt_desc_get_id(desc);
         }
     } else if (type == AVMEDIA_TYPE_AUDIO) {
         enum AVSampleFormat fmt = 0;
-        while (av_get_sample_fmt_name(fmt)) {
-            if (ff_add_format(&ret, fmt) < 0)
-                return NULL;
-            fmt++;
+
+        count = 0;
+        while (av_get_sample_fmt_name(fmt))
+            count++;
+
+        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
+        if (!ret->formats) {
+            av_free(ret);
+            return NULL;
         }
+        ret->nb_formats = count;
+
+        for (fmt = 0; fmt < count; fmt++)
+            ret->formats[fmt] = fmt;
+    } else {
+        av_freep(&ret);
     }
 
     return ret;