Skip to content

mirror()-style boundary reflection reads out-of-bounds for width/height < 3 (motion feature, all backends) #1580

Description

@kjg0724

`mirror(int idx, int size)` reflects row/column indices at frame boundaries
for the 5-tap motion filter (radius 2):

static inline int mirror(int idx, int size)
{
    if (idx < 0) return -idx;
    if (idx >= size) return 2 * size - idx - 2;
    return idx;
}

For size < 3 this returns indices outside [0, size):

  • mirror(-2, 1)2 (valid range is only {0})
  • mirror(-2, 2)2 (valid range is {0, 1})
  • mirror(3, 2)-1

size >= 3 is safe — I checked every tap offset (i-2+k for i in
[0,size), k in [0,5)) at size=3 and all outputs land in [0,3).

This affects motion feature extraction on frames with width or height 1 or
2. The identical mirror() is independently duplicated in:

  • libvmaf/src/feature/integer_motion.c (scalar reference)
  • libvmaf/src/feature/x86/motion_avx2.c
  • libvmaf/src/feature/x86/motion_avx512.c
  • libvmaf/src/feature/arm64/motion_neon.c

Two related paths have the same underlying sub-3 problem but aren't
identical copies, so flagging separately rather than lumping them in:

  • libvmaf/src/feature/cuda/integer_motion/motion_score.cu has its own,
    differently-structured mirror helper — only unsafe at size 1, not size 2.
  • The float motion path's convolution boundary logic
    (common/convolution_internal.h) uses the same reflection formula and
    has the same sub-3 issue.

Found while adding the NEON motion implementation (#1570) — a parity test
comparing scalar vs. NEON output was occasionally flaky at width/height
1-2. My working theory (not something I've proven from the allocator
internals) is that the two independently-allocated picture buffers used by
the test happened to read different out-of-bounds memory at those sizes;
excluding those two sizes from the test made the flakiness go away. Either
way, scalar and NEON already do the identical out-of-bounds read, so this
isn't a NEON-specific bug and wasn't fixed as part of that PR.

Not proposing a specific fix here — a naive clamp could change the
intended reflection semantics rather than just guard the bounds, so it
probably needs some care. Happy to help investigate if useful.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions