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

[Paul B Mahol <onemda@gmail.com>]

On Sat, May 11, 2024 at 3:24 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Paul B Mahol:
> > On Sat, May 11, 2024 at 5:28 AM James Almer <jamrial@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@foxmail.com>
> >> wrote:
> >>>
> >>>> From: Zhao Zhili <zhilizhao@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 after (a)movie filter that calls ff_null_get_video_buffer()
will not? work and using
ff_default_get_video_buffer() will allocate new frame.

Original idea was/is to improve performance by using DR support provided by
decoders.


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