[FFmpeg-devel] [PATCH] lavfi/zscale: Fix unaligned data ptr issues in realign_frame

[Pavel Koshevoy]

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.

... 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.



>
> > - s->tmp[job_nr] pointer was also unaligned.
>
> Please split this into one patch per issue fixed, to simplify future
> bisecting.
>
> >
> > https://github.com/sekrit-twc/zimg/issues/212
> > ---
> >   libavfilter/vf_zscale.c | 17 ++++++++++++-----
> >   libavutil/frame.c       |  8 +++++++-
> >   2 files changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c
> > index 4ba059064b..a74a3da8d9 100644
> > --- a/libavfilter/vf_zscale.c
> > +++ b/libavfilter/vf_zscale.c
> > @@ -112,6 +112,7 @@ typedef struct ZScaleContext {
> >       int force_original_aspect_ratio;
> >
> >       void *tmp[MAX_THREADS]; //separate for each thread;
> > +    void *tmp_aligned[MAX_THREADS];
> >       int nb_threads;
> >       int jobs_ret[MAX_THREADS];
> >       double in_slice_start[MAX_THREADS];
> > @@ -594,6 +595,7 @@ static int graphs_build(AVFrame *in, AVFrame *out,
> const AVPixFmtDescriptor *des
> >   {
> >       ZScaleContext *s = ctx->priv;
> >       int ret;
> > +    int misaligned_tmp;
> >       size_t size;
> >       zimg_image_format src_format;
> >       zimg_image_format dst_format;
> > @@ -628,11 +630,15 @@ static int graphs_build(AVFrame *in, AVFrame *out,
> const AVPixFmtDescriptor *des
> >       if (ret)
> >           return print_zimg_error(ctx);
> >
> > -    if (s->tmp[job_nr])
> > +    if (s->tmp[job_nr]) {
> >           av_freep(&s->tmp[job_nr]);
> > -    s->tmp[job_nr] = av_calloc(size, 1);
> > +        s->tmp_aligned[job_nr] = NULL;
> > +    }
> > +    s->tmp[job_nr] = av_calloc(size + ZIMG_ALIGNMENT - 1, 1);
> >       if (!s->tmp[job_nr])
> >           return AVERROR(ENOMEM);
> > +    misaligned_tmp = ((uintptr_t)(s->tmp[job_nr])) % ZIMG_ALIGNMENT;
>
> Use FFALIGN() instead.
>
> _______________________________________________
> 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".
>