| Message ID | tencent_964E02704D2D199A9040F59A7B9F86ABC008@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/src_movie: Remove align dimension to fix crash | expand |
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". >
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.
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". >
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. >
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". >
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:
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(-)