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

[James Almer]

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.

> 
> 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)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241109/a5730961/attachment.sig>