[FFmpeg-devel] [PATCH 3/3] avutil/frame: also align data pointers in av_frame_get_buffer()

[Pavel Koshevoy]

On Sat, Nov 16, 2024 at 10:02 AM James Almer <jamrial at gmail.com> wrote:

> From: Pavel Koshevoy <pkoshevoy at gmail.com>
>
> This avoids unpleasant surprises to av_frame_get_buffer callers
> that explicitly specified 64-byte alignment and didn't get
> AVFrame.data pointers that are 64-byte aligned.
>
> For example, see https://github.com/sekrit-twc/zimg/issues/212
>
> Although the zscale issue has already been resolved by other means
> it would still be prudent to improve the behavior of av_frame_get_buffer
> to fix any unknown and future instances of similar issues.
>
> Co-authored-by: James Almer <jamrial at gmail.com>
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  doc/APIchanges      |  4 ++++
>  libavutil/frame.c   | 21 ++++++++++++++++-----
>  libavutil/frame.h   |  7 ++++---
>  libavutil/version.h |  2 +-
>  4 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 15606cafac..d477904856 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,10 @@ The last version increases of all libraries were on
> 2024-03-07
>
>  API changes, most recent first:
>
> +2024-11-16 - xxxxxxxxxx - lavu 59.47.101 - frame.h
> +  av_frame_get_buffer() now also aligns the data pointers according to
> +  the requested alignment.
> +
>  2024-11-13 - xxxxxxxxxx - lavu 59.47.100 - channel_layout.h
>    Add AV_CHAN_BINAURAL_LEFT, AV_CHAN_BINAURAL_RIGHT
>    Add AV_CH_BINAURAL_LEFT, AV_CH_BINAURAL_RIGHT
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 093853b173..5fb47dbaa6 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -210,7 +210,7 @@ static int get_video_buffer(AVFrame *frame, int align)
>                                           padded_height, linesizes)) < 0)
>          return ret;
>
> -    total_size = 4*plane_padding;
> +    total_size = 4 * plane_padding + 4 * align;
>      for (int i = 0; i < 4; i++) {
>          if (sizes[i] > SIZE_MAX - total_size)
>              return AVERROR(EINVAL);
> @@ -230,6 +230,7 @@ static int get_video_buffer(AVFrame *frame, int align)
>      for (int i = 1; i < 4; i++) {
>          if (frame->data[i])
>              frame->data[i] += i * plane_padding;
> +        frame->data[i] = (uint8_t *)FFALIGN((uintptr_t)frame->data[i],
> align);
>      }
>
>      frame->extended_data = frame->data;
> @@ -244,6 +245,7 @@ static int get_audio_buffer(AVFrame *frame, int align)
>  {
>      int planar   = av_sample_fmt_is_planar(frame->format);
>      int channels, planes;
> +    size_t size;
>      int ret;
>
>      channels = frame->ch_layout.nb_channels;
> @@ -256,6 +258,9 @@ static int get_audio_buffer(AVFrame *frame, int align)
>              return ret;
>      }
>
> +    if (align <= 0)
> +        align = ALIGN;
> +
>      if (planes > AV_NUM_DATA_POINTERS) {
>          frame->extended_data = av_calloc(planes,
>                                            sizeof(*frame->extended_data));
> @@ -270,21 +275,27 @@ static int get_audio_buffer(AVFrame *frame, int
> align)
>      } else
>          frame->extended_data = frame->data;
>
> +    if (frame->linesize[0] > SIZE_MAX - align)
> +        return AVERROR(EINVAL);
> +    size = frame->linesize[0] + (size_t)align;
> +
>      for (int i = 0; i < FFMIN(planes, AV_NUM_DATA_POINTERS); i++) {
> -        frame->buf[i] = av_buffer_alloc(frame->linesize[0]);
> +        frame->buf[i] = av_buffer_alloc(size);
>          if (!frame->buf[i]) {
>              av_frame_unref(frame);
>              return AVERROR(ENOMEM);
>          }
> -        frame->extended_data[i] = frame->data[i] = frame->buf[i]->data;
> +        frame->extended_data[i] = frame->data[i] =
> +            (uint8_t *)FFALIGN((uintptr_t)frame->buf[i]->data, align);
>      }
>      for (int i = 0; i < planes - AV_NUM_DATA_POINTERS; i++) {
> -        frame->extended_buf[i] = av_buffer_alloc(frame->linesize[0]);
> +        frame->extended_buf[i] = av_buffer_alloc(size);
>          if (!frame->extended_buf[i]) {
>              av_frame_unref(frame);
>              return AVERROR(ENOMEM);
>          }
> -        frame->extended_data[i + AV_NUM_DATA_POINTERS] =
> frame->extended_buf[i]->data;
> +        frame->extended_data[i + AV_NUM_DATA_POINTERS] =
> +            (uint8_t *)FFALIGN((uintptr_t)frame->extended_buf[i]->data,
> align);
>      }
>      return 0;
>
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index f7806566d5..c107f43bc0 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -887,9 +887,10 @@ void av_frame_move_ref(AVFrame *dst, AVFrame *src);
>   *           cases.
>   *
>   * @param frame frame in which to store the new buffers.
> - * @param align Required buffer size alignment. If equal to 0, alignment
> will be
> - *              chosen automatically for the current CPU. It is highly
> - *              recommended to pass 0 here unless you know what you are
> doing.
> + * @param align Required buffer size and data pointer alignment. If equal
> to 0,
> + *              alignment will be chosen automatically for the current
> CPU.
> + *              It is highly recommended to pass 0 here unless you know
> what
> + *              you are doing.
>   *
>   * @return 0 on success, a negative AVERROR on error.
>   */
> diff --git a/libavutil/version.h b/libavutil/version.h
> index c1878a49ad..6a4abcf7f5 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -80,7 +80,7 @@
>
>  #define LIBAVUTIL_VERSION_MAJOR  59
>  #define LIBAVUTIL_VERSION_MINOR  47
> -#define LIBAVUTIL_VERSION_MICRO 100
> +#define LIBAVUTIL_VERSION_MICRO 101
>
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
> --
> 2.47.0
>
>

is something blocking this patch?