Re: [FFmpeg-devel] [PATCH] avfilter/framepool: fix alignment requirements for audio and video filters

[James Almer <jamrial@gmail.com>]

On 2/21/2022 3:14 PM, Paul B Mahol wrote:
> On Mon, Feb 21, 2022 at 6:57 PM James Almer <jamrial@gmail.com> wrote:
> 
>>
>>
>> On 2/21/2022 9:27 AM, Paul B Mahol wrote:
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>    libavfilter/audio.c     | 11 +++++------
>>>    libavfilter/framepool.c | 18 ++++++++----------
>>>    libavfilter/video.c     | 11 +++++------
>>>    3 files changed, 18 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/libavfilter/audio.c b/libavfilter/audio.c
>>> index cebc9709dd..a0408226a3 100644
>>> --- a/libavfilter/audio.c
>>> +++ b/libavfilter/audio.c
>>> @@ -22,15 +22,13 @@
>>>    #include "libavutil/avassert.h"
>>>    #include "libavutil/channel_layout.h"
>>>    #include "libavutil/common.h"
>>> +#include "libavutil/cpu.h"
>>>
>>>    #include "audio.h"
>>>    #include "avfilter.h"
>>>    #include "framepool.h"
>>>    #include "internal.h"
>>>
>>> -#define BUFFER_ALIGN 0
>>> -
>>> -
>>>    AVFrame *ff_null_get_audio_buffer(AVFilterLink *link, int nb_samples)
>>>    {
>>>        return ff_get_audio_buffer(link->dst->outputs[0], nb_samples);
>>> @@ -41,12 +39,13 @@ AVFrame *ff_default_get_audio_buffer(AVFilterLink
>> *link, int nb_samples)
>>>        AVFrame *frame = NULL;
>>>        int channels = link->channels;
>>>        int channel_layout_nb_channels =
>> av_get_channel_layout_nb_channels(link->channel_layout);
>>> +    int align = av_cpu_max_align();
>>>
>>>        av_assert0(channels == channel_layout_nb_channels ||
>> !channel_layout_nb_channels);
>>>
>>>        if (!link->frame_pool) {
>>>            link->frame_pool = ff_frame_pool_audio_init(av_buffer_allocz,
>> channels,
>>> -                                                    nb_samples,
>> link->format, BUFFER_ALIGN);
>>> +                                                    nb_samples,
>> link->format, align);
>>>            if (!link->frame_pool)
>>>                return NULL;
>>>        } else {
>>> @@ -62,11 +61,11 @@ AVFrame *ff_default_get_audio_buffer(AVFilterLink
>> *link, int nb_samples)
>>>            }
>>>
>>>            if (pool_channels != channels || pool_nb_samples < nb_samples
>> ||
>>> -            pool_format != link->format || pool_align != BUFFER_ALIGN) {
>>> +            pool_format != link->format || pool_align != align) {
>>>
>>>                ff_frame_pool_uninit((FFFramePool **)&link->frame_pool);
>>>                link->frame_pool =
>> ff_frame_pool_audio_init(av_buffer_allocz, channels,
>>> -                                                        nb_samples,
>> link->format, BUFFER_ALIGN);
>>> +                                                        nb_samples,
>> link->format, align);
>>>                if (!link->frame_pool)
>>>                    return NULL;
>>>            }
>>> diff --git a/libavfilter/framepool.c b/libavfilter/framepool.c
>>> index 7c63807df3..5b510c9af9 100644
>>> --- a/libavfilter/framepool.c
>>> +++ b/libavfilter/framepool.c
>>> @@ -76,27 +76,25 @@ FFFramePool *ff_frame_pool_video_init(AVBufferRef*
>> (*alloc)(size_t size),
>>>        }
>>>
>>>        if (!pool->linesize[0]) {
>>> -        for(i = 1; i <= align; i += i) {
>>> -            ret = av_image_fill_linesizes(pool->linesize, pool->format,
>>> -                                          FFALIGN(pool->width, i));
>>> -            if (ret < 0) {
>>> -                goto fail;
>>> -            }
>>> -            if (!(pool->linesize[0] & (pool->align - 1)))
>>> -                break;
>>> +        ret = av_image_fill_linesizes(pool->linesize, pool->format,
>>> +                                      FFALIGN(pool->width, align));
>>> +        if (ret < 0) {
>>> +            goto fail;
>>>            }
>>>
>>>            for (i = 0; i < 4 && pool->linesize[i]; i++) {
>>>                pool->linesize[i] = FFALIGN(pool->linesize[i],
>> pool->align);
>>> +            if ((pool->linesize[i] & (pool->align - 1)))
>>
>> Wont this check always succeed?
>>
>>
> Works here fine, it just there for case of possible programming error in
> future.
> It never enters that condition here.

Ok, LGTM then.

> 
> 
>>> +                goto fail;
>>>            }
>>>        }
>>>
>>>        for (i = 0; i < 4 && pool->linesize[i]; i++) {
>>> -        int h = FFALIGN(pool->height, 32);
>>> +        int h = pool->height;
>>>            if (i == 1 || i == 2)
>>>                h = AV_CEIL_RSHIFT(h, desc->log2_chroma_h);
>>>
>>> -        pool->pools[i] = av_buffer_pool_init(pool->linesize[i] * h + 16
>> + 16 - 1,
>>> +        pool->pools[i] = av_buffer_pool_init(pool->linesize[i] * h +
>> align,
>>>                                                 alloc);
>>>            if (!pool->pools[i])
>>>                goto fail;
>>> diff --git a/libavfilter/video.c b/libavfilter/video.c
>>> index 7ef04144e4..fa3d588044 100644
>>> --- a/libavfilter/video.c
>>> +++ b/libavfilter/video.c
>>> @@ -24,6 +24,7 @@
>>>    #include <stdio.h>
>>>
>>>    #include "libavutil/buffer.h"
>>> +#include "libavutil/cpu.h"
>>>    #include "libavutil/hwcontext.h"
>>>    #include "libavutil/imgutils.h"
>>>
>>> @@ -32,9 +33,6 @@
>>>    #include "internal.h"
>>>    #include "video.h"
>>>
>>> -#define BUFFER_ALIGN 32
>>> -
>>> -
>>>    AVFrame *ff_null_get_video_buffer(AVFilterLink *link, int w, int h)
>>>    {
>>>        return ff_get_video_buffer(link->dst->outputs[0], w, h);
>>> @@ -46,6 +44,7 @@ AVFrame *ff_default_get_video_buffer(AVFilterLink
>> *link, int w, int h)
>>>        int pool_width = 0;
>>>        int pool_height = 0;
>>>        int pool_align = 0;
>>> +    int align = av_cpu_max_align();
>>>        enum AVPixelFormat pool_format = AV_PIX_FMT_NONE;
>>>
>>>        if (link->hw_frames_ctx &&
>>> @@ -65,7 +64,7 @@ AVFrame *ff_default_get_video_buffer(AVFilterLink
>> *link, int w, int h)
>>>
>>>        if (!link->frame_pool) {
>>>            link->frame_pool = ff_frame_pool_video_init(av_buffer_allocz,
>> w, h,
>>> -                                                    link->format,
>> BUFFER_ALIGN);
>>> +                                                    link->format,
>> align);
>>>            if (!link->frame_pool)
>>>                return NULL;
>>>        } else {
>>> @@ -76,11 +75,11 @@ AVFrame *ff_default_get_video_buffer(AVFilterLink
>> *link, int w, int h)
>>>            }
>>>
>>>            if (pool_width != w || pool_height != h ||
>>> -            pool_format != link->format || pool_align != BUFFER_ALIGN) {
>>> +            pool_format != link->format || pool_align != align) {
>>>
>>>                ff_frame_pool_uninit((FFFramePool **)&link->frame_pool);
>>>                link->frame_pool =
>> ff_frame_pool_video_init(av_buffer_allocz, w, h,
>>> -                                                        link->format,
>> BUFFER_ALIGN);
>>> +                                                        link->format,
>> align);
>>>                if (!link->frame_pool)
>>>                    return NULL;
>>>            }
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@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-request@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-request@ffmpeg.org with subject "unsubscribe".