[FFmpeg-devel] [PATCH] avfilter: add wpsnr video filter

[Tomas Härdin]

fre 2021-10-29 klockan 17:00 +0200 skrev Paul B Mahol:
> On Fri, Oct 29, 2021 at 4:46 PM Tomas Härdin <tjoppen at acc.umu.se>
> wrote:
> 
> > fre 2021-10-29 klockan 15:33 +0200 skrev Paul B Mahol:
> > > On Fri, Oct 29, 2021 at 3:12 PM Tomas Härdin <tjoppen at acc.umu.se>
> > > wrote:
> > > > 
> > > > > +static double get_hx(const uint8_t *src, int linesize, int
> > > > > w,
> > > > > int h)
> > > > > +{
> > > > > +    int64_t sum = 0;
> > > > > +
> > > > > +    for (int y = 0; y < h; y++) {
> > > > > +        for (int x = 0; x < w; x++) {
> > > > > +            sum += 12 * src[x] -
> > > > > +                    2 * (src[x-1] + src[x+1] +
> > > > > +                         src[x + linesize] +
> > > > > +                         src[x - linesize]) -
> > > > > +                    1 * (src[x - 1 - linesize] +
> > > > > +                         src[x + 1 - linesize] +
> > > > > +                         src[x - 1 + linesize] +
> > > > > +                         src[x + 1 + linesize]);
> > > > > +        }
> > > > > +
> > > > > +        src += linesize;
> > > > > +    }
> > > > > +
> > > > > +    return fabs(sum * 0.25);
> > > > > +}
> > > > > +
> > > > > +static double get_hx16(const uint8_t *ssrc, int linesize,
> > > > > int w,
> > > > > int
> > > > > h)
> > > > > +{
> > > > > +    const uint16_t *src = (const uint16_t *)ssrc;
> > > > 
> > > > This is not -fstrict-aliasing safe
> > > > 
> > > 
> > > How so? I get no warnings at all, and similar is used everywhere
> > > else.
> > 
> > Then those places should be fixed. We have macros like AV_RB16()
> > for a
> > reason. That gcc doesn't warn about this doesn't mean it isn't free
> > to
> > assume ssrc and src points to different non-overlapping regions of
> > memory.
> > 
> > 
> That is sub optimal and unacceptable solution.

Did you do measurements to come to this conclusion?

> Review remark ignored.

Undefined behavior is *not* acceptable. If you want this file
specifically to be compiled with strict aliasing disabled then you must
at the very least change the build system accordingly

Type punning via union seems to be defined as of C99, so that should
also be acceptable

/Tomas