[FFmpeg-devel] [PATCH v3 1/3] avfilter/vf_colordetect: add new color range detection filter

[Kacper Michajlow]

On Fri, 18 Jul 2025 at 14:47, Niklas Haas <ffmpeg at haasn.xyz> wrote:
>
> On Fri, 18 Jul 2025 14:38:04 +0200 Kacper Michajlow <kasper93 at gmail.com>
wrote:
> > > +static inline int ff_detect_range_c(const uint8_t *data, ptrdiff_t
stride,
> > > +                                    ptrdiff_t width, ptrdiff_t
height,
> > > +                                    int mpeg_min, int mpeg_max)
> > > +{
> > > +    while (height--) {
> > > +        for (int x = 0; x < width; x++) {
> > > +            const uint8_t val = data[x];
> > > +            if (val < mpeg_min || val > mpeg_max)
> > > +                return 1;
> > > +        }
> > > +        data += stride;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> >
> > You could process width as a whole to allow better vectorization.
> > Assuming you don't process 10000x1 images, it will be faster on average.
>
> That's what I had in v1 of my patch, but it is significantly (50%) slower
> on GCC, which prefers the version I have written above.
>
> There is the not insignificant point that this C routine is also being
used
> to handle remaining elements that don't fit into a multiple of the SIMD
> kernel, for which the scalar code is actually preferred.

Interesting. It's my fault, I didn't check. GCC really doesn't like
bool/int there.

If that function is important you could try:
 {
+    uint8_t min = mpeg_min, max = mpeg_max;
     while (height--) {
+        uint8_t out_of_range = 0;
         for (int x = 0; x < width; x++) {
             const uint8_t val = data[x];
-            if (val < mpeg_min || val > mpeg_max)
-                return 1;
+            out_of_range |= val < min || val > max;
         }
+        if (out_of_range)
+            return 1;
         data += stride;
     }

Side note, if you change function prototype to `uint8_t mpeg_min, uint8_t
mpeg_max` directly,
clang goes down to 267.6 ( 1.00x). Unless it's because of UB, lol.

So, gcc scalar version is a bit slower in this case, but I think there is
value in it,
because it scales very nicely with  -ftree-vectorize, even with sse2 target.
Hopefully we will enable -ftree-vectorize by default soon, there is pending
patch for that.

before (gcc -fno-tree-vectorize)
detect_range_8_c:                                     5537.4 ( 1.00x)
detect_range_8_avx2:                                   149.7 (36.98x)
detect_range_8_avx512:                                 111.2 (49.80x)

after (gcc -fno-tree-vectorize)
detect_range_8_c:                                     7709.0 ( 1.00x)
detect_range_8_avx2:                                   137.6 (56.02x)
detect_range_8_avx512:                                 104.2 (73.97x)

after (gcc -ftree-vectorize --march=generic)
detect_range_8_c:                                      657.0 ( 1.00x)
detect_range_8_avx2:                                   161.7 ( 4.06x)
detect_range_8_avx512:                                 116.5 ( 5.64x)

after (gcc -ftree-vectorize --march=znver4)
detect_range_8_c:                                      285.6 ( 1.00x)
detect_range_8_avx2:                                   256.0 ( 1.12x)
detect_range_8_avx512:                                 107.6 ( 2.65x)

after (clang --march=generic)
detect_range_8_c:                                     1769.0 ( 1.00x)
detect_range_8_avx2:                                   231.8 ( 7.63x)
detect_range_8_avx512:                                  96.6 (18.32x)

after (clang --march=znver4)
detect_range_8_c:                                      952.9 ( 1.00x)
detect_range_8_avx2:                                   137.7 ( 6.92x)
detect_range_8_avx512:                                  95.9 ( 9.94x)

- Kacper