[FFmpeg-devel] [PATCH] lavfi/zscale: Fix unaligned data ptr issues in realign_frame

[Pavel Koshevoy]

On Sat, Nov 9, 2024 at 9:47 AM James Almer <jamrial at gmail.com> wrote:

> On 11/9/2024 12:55 PM, Pavel Koshevoy wrote:
> > On Sat, Nov 9, 2024 at 6:22 AM James Almer <jamrial at gmail.com> wrote:
> >
> >> On 11/9/2024 1:40 AM, Pavel Koshevoy wrote:
> >>> On Fri, Nov 8, 2024 at 7:29 PM James Almer <jamrial at gmail.com> wrote:
> >>>
> >>>> On 11/8/2024 10:51 PM, Pavel Koshevoy wrote:
> >>>>> I ran into segfaults in zimg when I attempted to use zscale
> >>>>> to convert a 512x538 at yuv444p16le(tv) image from HLG to Bt.709
> >>>>> with this filter chain:
> >>>>>
> >>>>>
> >>>>
> >>
> buffer=width=512:height=538:pix_fmt=yuv444p16le:time_base=1/1:sar=1/1,zscale=min=2020_ncl:rin=limited:pin=2020:tin=arib-std-b67:cin=left:t=linear,format=gbrpf32le,tonemap=gamma:desat=0,zscale=tin=linear:npl=100:p=709:m=709:r=limited:t=709,format=pix_fmts=yuv444p16le,buffersink
> >>>>>
> >>>>> I found several issues:
> >>>>> - realign_frame called av_pix_fmt_count_planes with incorrect
> >> parameter.
> >>>>> - av_frame_get_buffer did not align data pointers to specified
> >> alignment.
> >>>>
> >>>> Because it's not supposed to. The align parameter doxy states "buffer
> >>>> size alignment", which is the result of aligning the stride/linesizes,
> >>>> not the data pointers.
> >>>>
> >>>> You may want to use ff_default_get_video_buffer2(), which will add
> align
> >>>> amount of bytes to the allocated buffers (On top of aligning the
> >>>> linesizes to it), and then align the pointers with FFALIGN().
> >>>>
> >>>
> >>> I am not the maintainer of vf_zscale, and I am not intimately familiar
> >> with
> >>> private ffmpeg APIs such as ff_default_get_video_buffer2, and at first
> >>> glance that function
> >>> doesn't appear to be a drop-in replacement for av_frame_get_buffer.
> >>>
> >>> ff_default_get_video_buffer2 requires AVFilterLink parameter?! --
> >>> realign_frame doesn't
> >>> have that, it has an AVFrame which it needs to re-align to make it
> >>> compatible with libzimg API.
> >>
> >> It's trivial to make realign_frame take the necessary AVFilterLink as
> >> input argument.
> >>
> >>>
> >>> ... and why isn't av_frame_get_buffer populating AVFrame.data pointers
> >>> aligned
> >>> as specified by the align parameter? It costs at most (align - 1) more
> >>> padding bytes
> >>> to make it align the pointers, so I don't understand why it doesn't do
> >> that.
> >>
> >> Buffer alignment is set at configure time. It will be set to the highest
> >> needed alignment for the enabled simd (64 if avx512, else 32 if avx,
> >> else 16 if neon/sse, else 8). This is handled by av_malloc(), which is
> >> ultimately used by all alloc functions.
> >>
> >
> > Yes, I have noticed this while stepping through ffmpeg and zimg code.
> > The surprising thing I've observed is that ff_get_cpu_max_align_x86()
> > (called from av_cpu_max_align()) returned 8 ... it's surprising for an
> > ffmpeg
> > built and running on a Ryzen R9 5950x -- I would have expected 32.
> >
> > As a side note ... this ffmpeg build (and zimg build) were produced by
> > conan,
> > so perhaps the conan recipe for ffmpeg needs some changes to make
> > av_cpu_max_align() work as expected on 5950x.
> > (https://conan.io/center/recipes/ffmpeg)
> >
> >
> >
> >> As an specific alignment is not guaranteed, workarounds are needed if a
> >> module requires one.
> >>
> >
> > That is true of av_malloc, but av_frame_get_buffer is given an explicit
> > align parameter,
> > and it could trivially align the pointers accordingly making any external
> > workarounds
> > unnecessary.
> >
> > I still think my change to av_frame_get_buffer is the better approach:
> > - it doesn't break the ABI
> > - it doesn't break the API
> > - it improves the API behavior at little cost in allocated buffer padding
> > - it likely fixes other (unknown) instances where av_frame_get_buffer
> >    was expected to provide aligned data pointers and didn't, and the
> caller
> > is unaware of this.
> >
> >
> >
> >>
> >> I took the time to do it for zscale, as follows:
> >>
> >>
> > Thank you for providing this patch.  However, I would argue that this
> > functionality
> > (allocating a sufficient buffer and filling an AVFrame with properly
> > aligned data pointers
> > according to an explicitly specified alignment parameter) should be
> > available
> > via a public AVFrame API like av_frame_get_buffer,
> > and not require calls to a private libavfilter API.
> >
> > It feels a little bit like a Banana - Gorilla - Jungle problem when an
> > AVFilterLink
> > is required to produce an AVFrame with properly aligned data pointers.
>
> I sent a separate patch to have ff_default_get_video_buffer2() align the
> buffers using the provided align value. See "avfilter/framepool: align
> the frame buffers".
> ff_default_get_video_buffer2() also uses a buffer pool, so it's better
> than av_frame_get_buffer() in this case.
>
>
>

I still think my change to get_video_buffer buys us more than it costs
(including fixing the issue in vf_zscale), so I've submitted a separate
patch for it.

If that change is accepted than no further patches to vf_zscale will be
necessary.
However, if it is rejected -- please submit your own patch for
vf_zscale realign_buffer re-implemented with ff_default_get_video_buffer2.