[FFmpeg-devel] [PATCH 2/3] lavc: add a framework to fix alignment problems.

[Hendrik Leppkes]

On Sat, May 6, 2017 at 11:20 AM, Nicolas George <george at nsup.org> wrote:
> A lot of codecs require aligned frame data, but do not document it.
> It can result in crashes with perfectly valid uses of the API.
>
> TODO Implementation missing.
>
> Design rationale:
>
> - requiring frame data to be always aligned would be wasteful;
>
> - alignment requirements will evolve
>   (the 16->32 bump is still recent);
>
> - requiring applications to worry about alignment is not convenient.
>
> Signed-off-by: Nicolas George <george at nsup.org>
> ---
>  libavcodec/avcodec.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 35df4f6ced..51a7e2db21 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3644,6 +3644,16 @@ typedef struct AVCodecContext {
>       *             AVCodecContext.get_format callback)
>       */
>      int hwaccel_flags;
> +
> +    /**
> +     * Minimum alignment of frame data required by the codec.
> +     * All frame data pointers must have the alignment lower bits cleared,
> +     * i.e. be a multiple of 1<<alignment.
> +     * - encoding: set by the encoder and used by the framework
> +     * - decoding: unused
> +     */
> +    unsigned alignment;
> +
>  } AVCodecContext;
>

I don't think this is very practical. We have a bunch of generic DSPs
and if a codec uses those it would need to be constantly updated if
the requirements of those DSPs change. Today it may only use SSE and
be happy with 16-byte alignment, but what if someone adds an AVX2
implementation, or AVX512?
Going through all codecs that use some DSP and somehow set their
alignment value seems rather unfortunate - and on top of that, it'll
add arch-specific conditions, which leads to a lot of unnecessary
changes to a bunch of encoders (and filters) over time.

Or the alternative is that its initialized to some default value based
on the current CPU and only overriden in a minority of cases, which
still makes me wonder if the effort is worth it.

Right now the general consensus is that frames (and stride) should be
aligned to the maximum your CPU might require, ie. 32 byte on a AVX2
CPU, even if this is not documented.
I think it would be far better to officially document this. Making
applications aware of the alignment (which they likely are already
anyway, otherwise they would crash, even if they had to learn the hard
way) can make the entire process more efficient, since re-aligning can
be extremely expensive (ie. high res video) and should be avoided at
all cost if possible (which it not always is when you crop, for
example).

There is no particular reason we can't make the framework check the
alignment and re-align if required to the maximum CPU value (with a
loud warning, perhaps), but I don't think a per-codec or per-filter
alignment value is very practical.

Related, Libav introduced an API called av_cpu_max_align to query the
alignment requirements for the CPU, in case someone is not using
av_malloc to allocate their buffers, but their own allocation
mechanisms.

- Hendrik