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,buffersinkI found several issues: - realign_frame called av_pix_fmt_count_planes with incorrectparameter.- av_frame_get_buffer did not align data pointers to specifiedalignment.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 familiarwithprivate 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 dothat. 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.
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 *desreturn 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".
OpenPGP_signature.asc
Description: OpenPGP digital signature
_______________________________________________ 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".
