[FFmpeg-devel] [PATCH] lavfi/zscale: Fix unaligned data ptr issues in realign_frame
James Almer
jamrial at gmail.com
Sat Nov 9 18:47:17 EET 2024
On 11/9/2024 12:55 PM, Pavel Koshevoy wrote:
> 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.
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 *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".
>>
> _______________________________________________
> 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".
-------------- 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/ac761335/attachment.sig>
More information about the ffmpeg-devel
mailing list