[FFmpeg-devel] [PATCH 2/2] avcodec/sanm: avoid using k in left pxoff check

[Manuel Lauss]

Servus Michael,

On Wed, Jun 4, 2025 at 1:00 PM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> On Tue, Jun 03, 2025 at 12:30:40PM +0200, Manuel Lauss wrote:
> > Servus Michael,
> >
> > On Sat, May 31, 2025 at 12:51 AM Michael Niedermayer
> > <michael at niedermayer.cc> wrote:
> >
> > > > > >              /* smooth top and left block borders with neighbours */
> > > > > > -            if (((pxoff - p + k) < 0) || ((pxoff - p + k) >= maxpxo)
> > > > > > +            if (((pxoff - p + 0) < 0) || ((pxoff - p + k) >= maxpxo)
> > > > > >                  || ((pxoff + 3 * p) < 0) || ((pxoff + 3 * p) >= maxpxo)
> > > > > >                  || (i == 0) || (j == 0))
> > > > > >                  continue;
> > > >
> > > > Oops, yes, that change is correct.
> > >
> > > will apply
> > >
> > >
> > > > I think you should just rip the whole block-smoothing block out
> > > > entirely: it creates a "sawtooth" pattern
> > > > which is more annoying than the "blockiness" it tries to soften.
> > >
> > > probably, but i just wanted to fix the out of array access,
> > > ill leave improvment beyond that to others
> >
> > Is it OK if I just remove this block entirely?  I'd commit that along
> > with the other 2 sanm patches I have.
>
> i think you dont understand the bug my change is fixing
>
> Its an out of array access in:
>                 for (k = 0; k < 4; k++)
>                     *(dst + pxoff + k) = ((*(dst + pxoff + k) + *(dst + pxoff - p + k)) >> 1) & 0x7f;
>
> The hunk:
>             /* smooth top and left block borders with neighbours */
>             if (((pxoff - p + k) < 0) || ((pxoff - p + k) >= maxpxo)
>                 || ((pxoff + 3 * p) < 0) || ((pxoff + 3 * p) >= maxpxo)
>                 || (i == 0) || (j == 0))
>                 continue;
>
> skips this code. If the hunk is removed and nothing else is changed
> its MORE buggy as there will be more out of array accesses

I was thinking about this:

diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
index 00bfef00fe..5912a8c14b 100644
--- a/libavcodec/sanm.c
+++ b/libavcodec/sanm.c
@@ -732,25 +732,6 @@ static int old_codec4(SANMVideoContext *ctx,
GetByteContext *gb, int top, int le
                }
                pxo2 = pxo2 - 4 + p;
            }
-
-            /* smooth top and left block borders with neighbours */
-            if (((pxoff - p + k) < 0) || ((pxoff - p + k) >= maxpxo)
-                || ((pxoff + 3 * p) < 0) || ((pxoff + 3 * p) >= maxpxo)
-                || (i == 0) || (j == 0))
-                continue;
-            if (param & 0x80) {
-                for (k = 0; k < 4; k++)
-                    *(dst + pxoff + k) = ((*(dst + pxoff + k) + *(dst
+ pxoff - p + k)) >> 1) | 0x80;
-                *(dst + pxoff + 1 * p) = (*(dst + pxoff + 1 * p) +
*(dst + pxoff + 1 * p - 1)) >> 1 | 0x80;
-                *(dst + pxoff + 2 * p) = (*(dst + pxoff + 2 * p) +
*(dst + pxoff + 2 * p - 1)) >> 1 | 0x80;
-                *(dst + pxoff + 3 * p) = (*(dst + pxoff + 3 * p) +
*(dst + pxoff + 3 * p - 1)) >> 1 | 0x80;
-            } else {
-                for (k = 0; k < 4; k++)
-                    *(dst + pxoff + k) = ((*(dst + pxoff + k) + *(dst
+ pxoff - p + k)) >> 1) & 0x7f;
-                *(dst + pxoff + 1 * p) = (*(dst + pxoff + 1 * p) +
*(dst + pxoff + 1 * p - 1)) >> 1;
-                *(dst + pxoff + 2 * p) = (*(dst + pxoff + 2 * p) +
*(dst + pxoff + 2 * p - 1)) >> 1;
-                *(dst + pxoff + 3 * p) = (*(dst + pxoff + 3 * p) +
*(dst + pxoff + 3 * p - 1)) >> 1;
-            }
        }
    }
    return 0;


Manuel