On Sat, Nov 9, 2024 at 9:47 AM James Almer <jamr...@gmail.com> wrote:
> On 11/9/2024 12:55 PM, Pavel Koshevoy wrote: > > On Sat, Nov 9, 2024 at 6:22 AM James Almer <jamr...@gmail.com> wrote: > > > >> On 11/9/2024 1:40 AM, Pavel Koshevoy wrote: > >>> On Fri, Nov 8, 2024 at 7:29 PM James Almer <jamr...@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@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
