| Message ID | tencent_376A38FF163CCD73206B4DA64FB4D5392707@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,RFC] avfilter/video: Protect frame_pool from multi-threads access | expand |
> 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 >
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; }
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(-)