[FFmpeg-devel] [PATCH] avfilter/framepool: use av_image_fill_plane_sizes() to calculate pool sizes

[Xiang, Haihao]

On Tue, 2022-03-01 at 12:03 -0300, James Almer wrote:
> 
> On 3/1/2022 11:53 AM, Paul B Mahol wrote:
> > On Tue, Mar 1, 2022 at 3:36 PM James Almer <jamrial at gmail.com> wrote:
> > 
> > > On 3/1/2022 11:31 AM, Paul B Mahol wrote:
> > > > On Tue, Mar 1, 2022 at 3:23 PM James Almer <jamrial at gmail.com> wrote:
> > > > 
> > > > > Signed-off-by: James Almer <jamrial at gmail.com>
> > > > > ---
> > > > >    libavfilter/framepool.c | 20 ++++++++++++++------
> > > > >    1 file changed, 14 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/libavfilter/framepool.c b/libavfilter/framepool.c
> > > > > index 5b510c9af9..cf6a1d0ea0 100644
> > > > > --- a/libavfilter/framepool.c
> > > > > +++ b/libavfilter/framepool.c
> > > > > @@ -57,6 +57,8 @@ FFFramePool *ff_frame_pool_video_init(AVBufferRef*
> > > > > (*alloc)(size_t size),
> > > > >        int i, ret;
> > > > >        FFFramePool *pool;
> > > > >        const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(format);
> > > > > +    ptrdiff_t linesizes[4];
> > > > > +    size_t sizes[4];
> > > > > 
> > > > >        if (!desc)
> > > > >            return NULL;
> > > > > @@ -89,13 +91,19 @@ FFFramePool *ff_frame_pool_video_init(AVBufferRef*
> > > > > (*alloc)(size_t size),
> > > > >            }
> > > > >        }
> > > > > 
> > > > > -    for (i = 0; i < 4 && pool->linesize[i]; i++) {
> > > > > -        int h = pool->height;
> > > > > -        if (i == 1 || i == 2)
> > > > > -            h = AV_CEIL_RSHIFT(h, desc->log2_chroma_h);
> > > > > +    for (i = 0; i < 4; i++)
> > > > > +        linesizes[i] = pool->linesize[i];
> > > > > 
> > > > > -        pool->pools[i] = av_buffer_pool_init(pool->linesize[i] * h +
> > > > > align,
> > > > > -                                             alloc);
> > > > > +    if (av_image_fill_plane_sizes(sizes, pool->format,
> > > > > +                                  FFALIGN(pool->height, align),
> > > > > 
> > > > 
> > > > This is not needed for height.
> > > 
> > > av_frame_get_buffer() does it, and the lavc pool uses
> > > avcodec_align_dimensions2() to align height and achieve the same effect
> > > (Look at what it does for yuv422p). It's the only change in
> > > 17a59a634c39b00a680c6ebbaea58db95594d13d that could have generated the
> > > issue Haihao reported.
> > > 
> > 
> > I see only victims of cargo cultism here. Filter frames are unrelated to
> > avcodec frames.
> > 
> > Why I can not reproduce the issue, I'm on similar intel cpu.
> 
> What CPU do you have? He has an Ice-Lake, so av_cpu_max_align() returns 
> 64. Unless you have one of those, or an Alder-Lake with the E-cores 
> disabled, you'll get 32 out of it.

Yes, I experienced this issue with Ice Lake, and I can't reproduce this issue
with another CPU in which av_cpu_max_align() returns 32. 

> 
> Hardcode align to 64 in ff_default_get_video_buffer() to reproduce the 
> failure. Also, i was wrong and aligning height makes no difference, it 
> will still fail, so the problem is elsewhere.
> 

This patch doesn't work for me too.

Thanks
Haihao


> > 
> > 
> > > 
> > > > 
> > > > Original reporter never posted valgrind report of bug.
> > > > 
> > > > 
> > > > > +                                  linesizes) < 0) {
> > > > > +        goto fail;
> > > > > +    }
> > > > > +
> > > > > +    for (i = 0; i < 4 && sizes[i]; i++) {
> > > > > +        if (sizes[i] > SIZE_MAX - align)
> > > > > +            goto fail;
> > > > > +        pool->pools[i] = av_buffer_pool_init(sizes[i] + align,
> > > > > alloc);
> > > > >            if (!pool->pools[i])
> > > > >                goto fail;
> > > > >        }
> > > > > --
> > > > > 2.35.1
> > > > > 
> > > > > _______________________________________________
> > > > > ffmpeg-devel mailing list
> > > > > ffmpeg-devel at ffmpeg.org
> > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > 
> > > > > To unsubscribe, visit link above, or email
> > > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > > > 
> > > > 
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel at ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > 
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > 
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> > > 
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".