On Thu, 29 Feb 2024 12:36:36 +0100 Andreas Rheinhardt
<andreas.rheinha...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
