Re: [FFmpeg-devel] [PATCH] avcodec/mlp*: improvements

[Paul B Mahol <onemda@gmail.com>]

On Wed, Oct 25, 2023 at 8:39 PM Tomas Härdin <git@haerdin.se> wrote:

>
> >             if (c) {
> >                 e[0] = 1 << 14;
> >                 e[1] = 0 << 14;
> >                 e[2] = v[1];
> >                 e[3] = v[0];
> >             } else {
> >                 e[0] = v[0];
> >                 e[1] = v[1];
> >                 e[2] = 0 << 14;
> >                 e[3] = 1 << 14;
> >             }
> >
> >             if (invert2x2(e, d)) {
> >                 sum = UINT64_MAX;
> >                 goto next;
> >             }
> >
>
> You can make use of the properties of e to simplify calculating the
> inverse. The determinant is always v[0]<<14, so you can just do if
> (!v[0]) continue; and skip the determinant check altogether.
>
> >                 if (d[i] != av_clip_intp2(d[i], 15)) {
>
> d[i] < INT16_MIN || d[i] > INT16_MAX is more clear and probably faster
>
> > +                    lt = ((lm * e[0]) >> 14) + ((rm * e[1]) >> 14);
> > +                    rt = ((lm * e[2]) >> 14) + ((rm * e[3]) >> 14);
>
> Result is implementation-defined. Use division by (1<<14). Also add
> then divide. The intermediate result is 49 bits so fits easily in 64
> bits.
>

Division by (1<<14)  will give incorrect results. been there done that,
you can check all your "reviews" validity by testing patches and that
results is bitexact, otherwise I'm just wasting time here.

Additions are done before not later, again check your comments validity
before commenting more. Thanks.


> You could also simplify this calculation by again making use of the
> properties of e.
>
> >                     if (c)
> >                         v += FFABS(rt);
> >                     else
> >                         v += FFABS(lt);
> >                     sum += v;
> >                     if (sum > best_sum)
> >                         goto next;
>
> Seems like this reduces to solving a linear program.
>
> >                     if ((((lt * d[0]) >> 14) + ((rt * d[1]) >> 14))
> > != lm) {
> >                         sum = UINT64_MAX;
> >                         goto next;
> >                     }
> >
> >                     if ((((lt * d[2]) >> 14) + ((rt * d[3]) >> 14))
> > != rm) {
> >                         sum = UINT64_MAX;
> >                         goto next;
> >                     }
>
> Looks like a massive hack. I'd prefer to formally verify that the
> arithmetic works out. Also again you can make use of the properties of
> e, or inv(e) as it were.
>

Arithmetic may not always work out.


>
> /Tomas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".