[FFmpeg-devel] [PATCH 1/4] avcodec/aom_film_grain: add AOM film grain synthesis

[Andreas Rheinhardt]

Niklas Haas:
> +#if BIT_DEPTH > 8
> +# define entry int16_t
> +# define bitdepth_max ((1 << bitdepth) - 1)
> +# define HBD_DECL , const int bitdepth
> +# define HBD_CALL , bitdepth
> +# define SCALING_SIZE 4096
> +#else
> +# define entry int8_t
> +# define bitdepth 8
> +# define bitdepth_max UINT8_MAX

Why are you using signed types, but unsigned max values?

> +# define HBD_DECL
> +# define HBD_CALL
> +# define SCALING_SIZE 256
> +#endif
> +
> +// Symbols that do not depend on bit depth
> +#ifndef PXSTRIDE
> +# define PXSTRIDE(x) ((x) / sizeof(pixel))

There are several things that are wrong with this pattern:
1. sizeof is size_t and this has a conversion rank >= int/ptrdiff_t on
all systems I am aware of, therefore the division will be performed with
unsigned types, i.e. it is a logical right shift. And this will just not
work with negative linesizes (but given that pointer arithmetic via a
pixel* commonly involves a multiplication by sizeof(pixel), it often
happens to work in practice, but e.g. UBSan warns about this).
2. It presumes that linesize is always a multiple of the sizeof of the
underlying type. This need not be so. Imagine a system where there are
no alignment requirements whatsoever (i.e. the required alignment of
every type is 1 and vector instructions (if existing) also have no
alignment requirement). Then it is legal for our callers to use frames
where linesize can be anything.
3. Even if linesize is always a multiple of sizeof(pixel), the compiler
does not know this and therefore will have to mask the low bits. So
instead of
src_row + (y) * PXSTRIDE(stride) + (x) + bx
use
(const pixel*)((const char*)src_row + (y) * stride) + (x) + bx

(Please don't cast const away on intermediate pointers when changing this.)

> +# define GRAIN_WIDTH 82
> +# define GRAIN_HEIGHT 73
> +# define SUB_GRAIN_WIDTH 44
> +# define SUB_GRAIN_HEIGHT 38
> +# define FG_BLOCK_SIZE 32
> +#endif
> +

...

> +    // Copy over the non-modified planes
> +    if (!data->num_y_points) {
> +        av_image_copy_plane(out_frame->data[0], out_frame->linesize[0],
> +                            in_frame->data[0], in_frame->linesize[0],
> +                            out_frame->width * sizeof(pixel), out_frame->height);
> +    }
> +    for (int uv = 0; uv < 2; uv++) {
> +        if (!data->num_uv_points[uv]) {
> +            av_image_copy_plane(out_frame->data[1+uv], out_frame->linesize[1+uv],
> +                                in_frame->data[1+uv], in_frame->linesize[1+uv],
> +                                AV_CEIL_RSHIFT(out_frame->width, subx) * sizeof(pixel),
> +                                AV_CEIL_RSHIFT(out_frame->height, suby));
> +        }
> +    }
> +

This is generic and can be moved out of the template.