[FFmpeg-devel] [RFC PATCH] avfilter/video: Protect frame_pool from multi-threads access

[FFmpeg-devel] [RFC PATCH] avfilter/video: Protect frame_pool from multi-threads access

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Fix crash with
./ffplay -f lavfi -i movie=foo.mp4,drawtext=text=bar
---
 libavfilter/avfilter.c          |  2 ++
 libavfilter/avfilter_internal.h |  2 ++
 libavfilter/video.c             | 14 ++++++++------
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index 049e4f62ca..44da809c15 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -176,6 +176,7 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
     li = av_mallocz(sizeof(*li));
     if (!li)
         return AVERROR(ENOMEM);
+    ff_mutex_init(&li->frame_pool_mutex, NULL);
     link = &li->l;
 
     src->outputs[srcpad] = dst->inputs[dstpad] = link;
@@ -203,6 +204,7 @@ static void link_free(AVFilterLink **link)
 
     ff_framequeue_free(&li->fifo);
     ff_frame_pool_uninit(&li->frame_pool);
+    ff_mutex_destroy(&li->frame_pool_mutex);
     av_channel_layout_uninit(&(*link)->ch_layout);
 
     av_freep(link);
diff --git a/libavfilter/avfilter_internal.h b/libavfilter/avfilter_internal.h
index 2c31c3e7de..eae3a699c3 100644
--- a/libavfilter/avfilter_internal.h
+++ b/libavfilter/avfilter_internal.h
@@ -27,6 +27,7 @@
 
 #include <stdint.h>
 
+#include "libavutil/thread.h"
 #include "avfilter.h"
 #include "framequeue.h"
 
@@ -34,6 +35,7 @@ typedef struct FilterLinkInternal {
     AVFilterLink l;
 
     struct FFFramePool *frame_pool;
+    AVMutex frame_pool_mutex;
 
     /**
      * Queue of frames waiting to be filtered.
diff --git a/libavfilter/video.c b/libavfilter/video.c
index 89d0797ab5..30eba76cbd 100644
--- a/libavfilter/video.c
+++ b/libavfilter/video.c
@@ -70,17 +70,17 @@ AVFrame *ff_default_get_video_buffer2(AVFilterLink *link, int w, int h, int alig
         return frame;
     }
 
+    ff_mutex_lock(&li->frame_pool_mutex);
     if (!li->frame_pool) {
         li->frame_pool = ff_frame_pool_video_init(av_buffer_allocz, w, h,
                                                   link->format, align);
         if (!li->frame_pool)
-            return NULL;
+            goto out;
     } else {
         if (ff_frame_pool_get_video_config(li->frame_pool,
                                            &pool_width, &pool_height,
-                                           &pool_format, &pool_align) < 0) {
-            return NULL;
-        }
+                                           &pool_format, &pool_align) < 0)
+            goto out;
 
         if (pool_width != w || pool_height != h ||
             pool_format != link->format || pool_align != align) {
@@ -89,18 +89,20 @@ AVFrame *ff_default_get_video_buffer2(AVFilterLink *link, int w, int h, int alig
             li->frame_pool = ff_frame_pool_video_init(av_buffer_allocz, w, h,
                                                       link->format, align);
             if (!li->frame_pool)
-                return NULL;
+                goto out;
         }
     }
 
     frame = ff_frame_pool_get(li->frame_pool);
     if (!frame)
-        return NULL;
+        goto out;
 
     frame->sample_aspect_ratio = link->sample_aspect_ratio;
     frame->colorspace  = link->colorspace;
     frame->color_range = link->color_range;
 
+out:
+    ff_mutex_unlock(&li->frame_pool_mutex);
     return frame;
 }
 
-- 
2.42.0

_______________________________________________
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".

Re: [FFmpeg-devel] [RFC PATCH] avfilter/video: Protect frame_pool from multi-threads access

[Zhao Zhili]

> On May 10, 2024, at 16:57, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Fix crash with
> ./ffplay -f lavfi -i movie=foo.mp4,drawtext=text=bar

This is an RFC because I’m not sure whether it’s a valid use case or not to call
 ff_default_get_video_buffer2 from multiple threads. On one hand, it’s rare used
with multithreads. On the other hand, it can be hidden deep and dangerous.

> ---
> libavfilter/avfilter.c          |  2 ++
> libavfilter/avfilter_internal.h |  2 ++
> libavfilter/video.c             | 14 ++++++++------
> 3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 049e4f62ca..44da809c15 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -176,6 +176,7 @@ int avfilter_link(AVFilterContext *src, unsigned srcpad,
>     li = av_mallocz(sizeof(*li));
>     if (!li)
>         return AVERROR(ENOMEM);
> +    ff_mutex_init(&li->frame_pool_mutex, NULL);
>     link = &li->l;
> 
>     src->outputs[srcpad] = dst->inputs[dstpad] = link;
> @@ -203,6 +204,7 @@ static void link_free(AVFilterLink **link)
> 
>     ff_framequeue_free(&li->fifo);
>     ff_frame_pool_uninit(&li->frame_pool);
> +    ff_mutex_destroy(&li->frame_pool_mutex);
>     av_channel_layout_uninit(&(*link)->ch_layout);
> 
>     av_freep(link);
> diff --git a/libavfilter/avfilter_internal.h b/libavfilter/avfilter_internal.h
> index 2c31c3e7de..eae3a699c3 100644
> --- a/libavfilter/avfilter_internal.h
> +++ b/libavfilter/avfilter_internal.h
> @@ -27,6 +27,7 @@
> 
> #include <stdint.h>
> 
> +#include "libavutil/thread.h"
> #include "avfilter.h"
> #include "framequeue.h"
> 
> @@ -34,6 +35,7 @@ typedef struct FilterLinkInternal {
>     AVFilterLink l;
> 
>     struct FFFramePool *frame_pool;
> +    AVMutex frame_pool_mutex;
> 
>     /**
>      * Queue of frames waiting to be filtered.
> diff --git a/libavfilter/video.c b/libavfilter/video.c
> index 89d0797ab5..30eba76cbd 100644
> --- a/libavfilter/video.c
> +++ b/libavfilter/video.c
> @@ -70,17 +70,17 @@ AVFrame *ff_default_get_video_buffer2(AVFilterLink *link, int w, int h, int alig
>         return frame;
>     }
> 
> +    ff_mutex_lock(&li->frame_pool_mutex);
>     if (!li->frame_pool) {
>         li->frame_pool = ff_frame_pool_video_init(av_buffer_allocz, w, h,
>                                                   link->format, align);
>         if (!li->frame_pool)
> -            return NULL;
> +            goto out;
>     } else {
>         if (ff_frame_pool_get_video_config(li->frame_pool,
>                                            &pool_width, &pool_height,
> -                                           &pool_format, &pool_align) < 0) {
> -            return NULL;
> -        }
> +                                           &pool_format, &pool_align) < 0)
> +            goto out;
> 
>         if (pool_width != w || pool_height != h ||
>             pool_format != link->format || pool_align != align) {
> @@ -89,18 +89,20 @@ AVFrame *ff_default_get_video_buffer2(AVFilterLink *link, int w, int h, int alig
>             li->frame_pool = ff_frame_pool_video_init(av_buffer_allocz, w, h,
>                                                       link->format, align);
>             if (!li->frame_pool)
> -                return NULL;
> +                goto out;
>         }
>     }
> 
>     frame = ff_frame_pool_get(li->frame_pool);
>     if (!frame)
> -        return NULL;
> +        goto out;
> 
>     frame->sample_aspect_ratio = link->sample_aspect_ratio;
>     frame->colorspace  = link->colorspace;
>     frame->color_range = link->color_range;
> 
> +out:
> +    ff_mutex_unlock(&li->frame_pool_mutex);
>     return frame;
> }
> 
> -- 
> 2.42.0
> 

_______________________________________________
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".

Re: [FFmpeg-devel] [RFC PATCH] avfilter/video: Protect frame_pool from multi-threads access

[Anton Khirnov]

Quoting Zhao Zhili (2024-05-11 06:45:12)
> 
> > On May 10, 2024, at 16:57, Zhao Zhili <quinkblack@foxmail.com> wrote:
> > 
> > From: Zhao Zhili <zhilizhao@tencent.com>
> > 
> > Fix crash with
> > ./ffplay -f lavfi -i movie=foo.mp4,drawtext=text=bar
> 
> This is an RFC because I’m not sure whether it’s a valid use case or not to call
>  ff_default_get_video_buffer2 from multiple threads. On one hand, it’s rare used
> with multithreads. On the other hand, it can be hidden deep and dangerous.

Naively it sounds like something that should not happen, since lavfi
only has slice threading and slice threads should not allocate frames.

In your example it seems to happen because of frame threading in the
movie source, but that should ideally be handled inside that filter.

-- 
Anton Khirnov
_______________________________________________
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".