[FFmpeg-devel] [PATCH] lavfi/zscale: Fix unaligned data ptr issues in realign_frame
Pavel Koshevoy
pkoshevoy at gmail.com
Sat Nov 9 17:55:29 EET 2024
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.
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list