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

[Pavel Koshevoy]

On Sat, Nov 9, 2024 at 10:31 AM Pavel Koshevoy <pkoshevoy at gmail.com> wrote:

>
>
> On Sat, Nov 9, 2024 at 9:53 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.
>>
>> Yeah, av_cpu_max_align() returns a value depending on runtime
>> parameters, so unless you force cpuflags using av_force_cpu_flags() to
>> disable everything SSE and above (or it being disabled during
>> configure), it should not return 8.
>>
>
> I certainly did not call av_force_cpu_flags myself, and as far as I can
> see
> conan ffmpeg recipe does not pass any --disable-sse or --disable-avx
> options
> to ffmpegs configure script ... I will have to investigate this separately
> when I am back at work.
>
>
>
>
>> >
>> > 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)
>>
>>
The conan recipe for ffmpeg adds --disable-mmx to ffmpeg configure options
when building a Debug package.
Unfortunately, this disabled not only MMX, but also SSE and AVX, and
affected av_cpu_max_align().
I've modified my local ffmpeg conan recipe to omit the --disable-mmx option
from Debug builds.

Pavel.