[FFmpeg-devel] [PATCH] avfilter/src_movie: Remove align dimension to fix crash

[Andreas Rheinhardt]

Paul B Mahol:
> On Sat, May 11, 2024 at 5:28 AM James Almer <jamrial at gmail.com> wrote:
> 
>> On 5/10/2024 6:26 PM, Paul B Mahol wrote:
>>> On Fri, May 10, 2024 at 10:56 AM Zhao Zhili <quinkblack at foxmail.com>
>> wrote:
>>>
>>>> From: Zhao Zhili <zhilizhao at tencent.com>
>>>>
>>>> The alignment is handled by ff_default_get_video_buffer2. We
>>>> shouldn't use the aligned width/height as FFFramePool width/height.
>>>> It cause recreate FFFramePool inside ff_default_get_video_buffer2.
>>>> The recreate of FFFramePool together with multi-threads decoding
>>>> leading to data race and crash, for example,
>>>> ./ffplay -f lavfi -i movie=foo.mp4,drawtext=text=bar
>>>> ---
>>>>   libavfilter/src_movie.c | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/libavfilter/src_movie.c b/libavfilter/src_movie.c
>>>> index e2cdcf17db..5099012100 100644
>>>> --- a/libavfilter/src_movie.c
>>>> +++ b/libavfilter/src_movie.c
>>>> @@ -187,7 +187,6 @@ static int get_buffer(AVCodecContext *avctx, AVFrame
>>>> *frame, int flags)
>>>>
>>>>       switch (avctx->codec_type) {
>>>>       case AVMEDIA_TYPE_VIDEO:
>>>> -        avcodec_align_dimensions2(avctx, &w, &h, linesize_align);
>>>>
>>>
>>> Left unused linesize_align[].
>>>
>>> With locking introduced, cant you remove copy check hack above and force
>>> new_pool every time?
>>
>> Can't this just always use avcodec_default_get_buffer2() (So in short,
>> not set a custom get_buffer2() callback at all), since it also has its
>> own buffer pool? Much like it's being used for AV_CODEC_CAP_DR1
>> decoders, it can be used for all of them.
>>
>> ff_default_get_{video,audio}_buffer() don't seem like they bring any
>> benefit here. In fact, not using them may be an improvement considering
>> they allocate a whole new AVFrame forcing the callback to copy props,
>> move the reference and then free the superfluous AVFrame afterwards.
>>
> 
> In that case frame is always not part of frame pool and thus not writable,

What makes you think that frames that are not part of a frame pool are
not writable?

> any next
> filter that needs writable frames will just allocate and sometimes even
> copy whole frame data.
> The original idea is to avoid that copy.
>