[FFmpeg-devel] [PATCH] lavfi/zscale: Fix unaligned data ptr issues in realign_frame
James Almer
jamrial at gmail.com
Sat Nov 9 15:22:01 EET 2024
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.
As an specific alignment is not guaranteed, workarounds are needed if a
module requires one.
I took the time to do it for zscale, as follows:
> 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);
-------------- 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/58a4dad4/attachment.sig>
More information about the ffmpeg-devel
mailing list