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

[Niklas Haas]

On Thu, 29 Feb 2024 12:36:36 +0100 Andreas Rheinhardt <andreas.rheinhardt at outlook.com> wrote:
> 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?

`entry` is the type of a (signed) film grain LUT entry, which is
distinct from the maximum value for the image bit-depth.

Consider the canonical usage as:

  entry grain_lut[];
  pixel = av_clip( src_pixel + grain_lut[idx], 0, bitdepth_max );

> > +# 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.)

Thanks. The code was copied as-is from dav1d (which also supports
negative strides, but I guess they don't care about this particular UB?)

I will change it to the pattern you recommended.

> > +    // 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.

Done.