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

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

[Zhao Zhili]

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);
         new = ff_default_get_video_buffer(outlink, w, h);
         break;
     case AVMEDIA_TYPE_AUDIO:
-- 
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] [PATCH] avfilter/src_movie: Remove align dimension to fix crash

[Paul B Mahol]

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?


>          new = ff_default_get_video_buffer(outlink, w, h);
>          break;
>      case AVMEDIA_TYPE_AUDIO:
> --
> 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".
>
_______________________________________________
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] [PATCH] avfilter/src_movie: Remove align dimension to fix crash

[James Almer]

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.
_______________________________________________
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] [PATCH] avfilter/src_movie: Remove align dimension to fix crash

[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,
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".

Re: [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@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 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".

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

[Paul B Mahol]

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

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

[Anton Khirnov]

Quoting Paul B Mahol (2024-05-11 15:48:08)
> 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.

As I remember, the original intent behind lavfi get_buffer functions
was:

1) Allow a downstream filter like pad to allocate a larger frame in
order to avoid copying.

2) Allow direct rendering into caller-provided buffers (currently not
supported).

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