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