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. > > diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c > > index 4ba059064b..c6518b0f9f 100644 > > --- a/libavfilter/vf_zscale.c > > +++ b/libavfilter/vf_zscale.c > > @@ -34,6 +34,7 @@ > > #include "filters.h" > > #include "formats.h" > > #include "video.h" > > +#include "libavutil/cpu.h" > > #include "libavutil/eval.h" > > #include "libavutil/internal.h" > > #include "libavutil/intreadwrite.h" > > @@ -657,27 +658,23 @@ static int graphs_build(AVFrame *in, AVFrame *out, > const AVPixFmtDescriptor *des > > return 0; > > } > > > > -static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame > **frame, int needs_copy) > > +static int realign_frame(AVFilterLink *link, const AVPixFmtDescriptor > *desc, AVFrame **frame, int needs_copy) > > { > > AVFrame *aligned = NULL; > > int ret = 0, plane, planes; > > > > /* Realign any unaligned input frame. */ > > - planes = av_pix_fmt_count_planes(desc->nb_components); > > + planes = av_pix_fmt_count_planes((*frame)->format); > > for (plane = 0; plane < planes; plane++) { > > int p = desc->comp[plane].plane; > > if ((uintptr_t)(*frame)->data[p] % ZIMG_ALIGNMENT || > (*frame)->linesize[p] % ZIMG_ALIGNMENT) { > > - if (!(aligned = av_frame_alloc())) { > > - ret = AVERROR(ENOMEM); > > - goto fail; > > - } > > + aligned = ff_default_get_video_buffer2(link, > (*frame)->width, (*frame)->height, > > + > FFMAX(av_cpu_max_align(), ZIMG_ALIGNMENT)); > > + if (!aligned) > > + return AVERROR(ENOMEM); > > > > - aligned->format = (*frame)->format; > > - aligned->width = (*frame)->width; > > - aligned->height = (*frame)->height; > > - > > - if ((ret = av_frame_get_buffer(aligned, ZIMG_ALIGNMENT)) < > 0) > > - goto fail; > > + for (int i = 0; i < 4 && aligned->data[i]; i++) > > + aligned->data[i] = (uint8_t > *)FFALIGN((uintptr_t)aligned->data[i], FFMAX(av_cpu_max_align(), > ZIMG_ALIGNMENT)); > > > > if (needs_copy && (ret = av_frame_copy(aligned, *frame)) < > 0) > > goto fail; > > @@ -802,20 +799,22 @@ static int filter_frame(AVFilterLink *link, > AVFrame *in) > > (s->src_format.pixel_type !=s->dst_format.pixel_type) || > > (s->src_format.transfer_characteristics > !=s->dst_format.transfer_characteristics) > > ){ > > - out = ff_get_video_buffer(outlink, outlink->w, outlink->h); > > + out = ff_default_get_video_buffer2(outlink, outlink->w, > outlink->h, > > + FFMAX(av_cpu_max_align(), > ZIMG_ALIGNMENT)); > > if (!out) { > > ret = AVERROR(ENOMEM); > > goto fail; > > } > > > > - if ((ret = realign_frame(odesc, &out, 0)) < 0) > > - goto fail; > > + for (int i = 0; i < 4 && out->data[i]; i++) > > + out->data[i] = (uint8_t *)FFALIGN((uintptr_t)out->data[i], > > + FFMAX(av_cpu_max_align(), > ZIMG_ALIGNMENT)); > > > > av_frame_copy_props(out, in); > > out->colorspace = outlink->colorspace; > > out->color_range = outlink->color_range; > > > > - if ((ret = realign_frame(desc, &in, 1)) < 0) > > + if ((ret = realign_frame(link, desc, &in, 1)) < 0) > > goto fail; > > > > snprintf(buf, sizeof(buf)-1, "%d", outlink->w); > > > _______________________________________________ > 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". > _______________________________________________ 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".
