Re: [FFmpeg-devel] [PATCH v2 4/4] swscale/aarch64: Add rgb24 to yuv implementation

[Zhao Zhili <quinkblack@foxmail.com>]

> On Jun 7, 2024, at 17:09, Martin Storsjö <martin@martin.st> wrote:
> 
> On Fri, 7 Jun 2024, Zhao Zhili wrote:
> 
>> Note both tests use clang as compiler, which has vectorization
>> enabled by default with -O3.
> 
> FWIW, for more interesting benchmarks, you can configure the build with --optflags="-O3 -fno-vectorize".
> 
> (Although, the benchmarks against a compiler vectorized version still is relevant for knowing how much real-world benefit you get from the optimizations.)
> 
>> +#include "libavutil/aarch64/asm.S"
>> +
>> +.macro rgb24_to_yuv_load_rgb, src
>> +        ld3             { v16.16b, v17.16b, v18.16b }, [\src]
>> +        uxtl            v19.8h, v16.8b             // v19: r
>> +        uxtl            v20.8h, v17.8b             // v20: g
>> +        uxtl            v21.8h, v18.8b             // v21: b
>> +        uxtl2           v22.8h, v16.16b            // v22: r
>> +        uxtl2           v23.8h, v17.16b            // v23: g
>> +        uxtl2           v24.8h, v18.16b            // v24: b
>> +.endm
>> +
>> +.macro rgb24_to_yuv_product, r, g, b, dst1, dst2, dst, coef0, coef1, coef2, right_shift
>> +        mov             \dst1\().16b, v6.16b                    // dst1 = const_offset
>> +        mov             \dst2\().16b, v6.16b                    // dst2 = const_offset
>> +        smlal           \dst1\().4s, \coef0\().4h, \r\().4h     // dst1 += rx * r
>> +        smlal           \dst1\().4s, \coef1\().4h, \g\().4h     // dst1 += gx * g
>> +        smlal           \dst1\().4s, \coef2\().4h, \b\().4h     // dst1 += bx * b
>> +        smlal2          \dst2\().4s, \coef0\().8h, \r\().8h     // dst2 += rx * r
>> +        smlal2          \dst2\().4s, \coef1\().8h, \g\().8h     // dst2 += gx * g
>> +        smlal2          \dst2\().4s, \coef2\().8h, \b\().8h     // dst2 += bx * b
>> +        sqshrn          \dst\().4h, \dst1\().4s, \right_shift   // dst_lower_half = dst1 >> right_shift
>> +        sqshrn2         \dst\().8h, \dst2\().4s, \right_shift   // dst_higher_half = dst2 >> right_shift
>> +.endm
>> +
>> +function ff_rgb24ToY_neon, export=1
>> +        cmp             w4, #0                  // check width > 0
>> +        ldp             w10, w11, [x5]          // w10: ry, w11: gy
>> +        ldr             w12, [x5, #8]           // w12: by
>> +        b.le            3f
>> +
>> +        mov             w9, #256                // w9 = 1 << (RGB2YUV_SHIFT - 7)
>> +        movk            w9, #8, lsl #16         // w9 += 32 << (RGB2YUV_SHIFT - 1)
>> +        dup             v6.4s, w9               // w9: const_offset
>> +
>> +        cmp             w4, #16
>> +        dup             v0.8h, w10
>> +        dup             v1.8h, w11
>> +        dup             v2.8h, w12
>> +        b.lt            2f
>> +1:
>> +        rgb24_to_yuv_load_rgb x1
>> +        rgb24_to_yuv_product v19, v20, v21, v25, v26, v16, v0, v1, v2, #9
>> +        rgb24_to_yuv_product v22, v23, v24, v27, v28, v17, v0, v1, v2, #9
>> +        sub             w4, w4, #16             // width -= 16
>> +        add             x1, x1, #48             // src += 48
>> +        cmp             w4, #16                 // width >= 16 ?
>> +        stp             q16, q17, [x0], #32     // store to dst
>> +        b.ge            1b
>> +        cbz             x4, 3f
>> +2:
>> +        ldrb            w13, [x1]               // w13: r
>> +        ldrb            w14, [x1, #1]           // w14: g
>> +        ldrb            w15, [x1, #2]           // w15: b
>> +
>> +        smaddl          x13, w13, w10, x9       // x13 = ry * r + const_offset
>> +        smaddl          x13, w14, w11, x13      // x13 += gy * g
>> +        smaddl          x13, w15, w12, x13      // x13 += by * b
>> +        asr             w13, w13, #9            // x13 >>= 9
>> +        sub             w4, w4, #1              // i++
> 
> The comment needs to be updated to i--, or maybe rather width--
> 
> Also, when approaching zero, I would intuitively do subs, followed by b.gt (or b.ne), instead of sub + cbnz. But it's the same number of registers, so I'm not sure if it makes any practical difference. Intuitively it feels more efficient (as the condition gets calculated earlier) though.

I have checked Cortext A78:
1. cbnz and b.gt has the same execute latency and execution throughput
2. sub has execution throughput 4, while subs has throughput 3.

So I think sub + cbnz is more efficient than subs + b.gt.

On Cortext A57, there is no difference between the choice.

Bear with me if I misread the document. This is the first time I trying to understand the arch detail.

https://developer.arm.com/documentation/102160/latest/
> 
>> +        add             x1, x1, #3              // src += 3
>> +        strh            w13, [x0], #2           // store to dst
>> +        cbnz            w4, 2b
>> +3:
>> +        ret
>> +endfunc
>> +
>> +.macro rgb24_load_uv_coeff half
>> +        ldp             w10, w11, [x6, #12]     // w10: ru, w11: gu
>> +        ldp             w12, w13, [x6, #20]     // w12: bu, w13: rv
>> +        ldp             w14, w15, [x6, #28]     // w14: gv, w15: bv
>> +    .if \half
>> +        mov             w9, #512
>> +        movk            w9, #128, lsl #16       // w9: const_offset
>> +    .else
>> +        mov             w9, #256
>> +        movk            w9, #64, lsl #16        // w9: const_offset
>> +    .endif
>> +        dup             v0.8h, w10
>> +        dup             v1.8h, w11
>> +        dup             v2.8h, w12
>> +        dup             v3.8h, w13
>> +        dup             v4.8h, w14
>> +        dup             v5.8h, w15
>> +        dup             v6.4s, w9
>> +.endm
>> +
>> +function ff_rgb24ToUV_half_neon, export=1
>> +        cmp             w5, #0          // check width > 0
>> +        b.le            3f
>> +
>> +        cmp             w5, #8
>> +        rgb24_load_uv_coeff half=1
>> +        b.lt            2f
>> +1:
>> +        ld3             { v16.16b, v17.16b, v18.16b }, [x3]
>> +        uaddlp          v19.8h, v16.16b         // v19: r
>> +        uaddlp          v20.8h, v17.16b         // v20: g
>> +        uaddlp          v21.8h, v18.16b         // v21: b
>> +
>> +        rgb24_to_yuv_product v19, v20, v21, v22, v23, v16, v0, v1, v2, #10
>> +        rgb24_to_yuv_product v19, v20, v21, v24, v25, v17, v3, v4, v5, #10
>> +        sub             w5, w5, #8              // width -= 8
>> +        add             x3, x3, #48             // src += 48
>> +        cmp             w5, #8                  // width >= 8 ?
>> +        str             q16, [x0], #16          // store dst_u
>> +        str             q17, [x1], #16          // store dst_v
>> +        b.ge            1b
>> +        cbz             w5, 3f
>> +2:
>> +        ldrb            w2, [x3]                // w2: r1
>> +        ldrb            w4, [x3, #3]            // w4: r2
>> +        add             w2, w2, w4              // w2 = r1 + r2
>> +
>> +        ldrb            w4, [x3, #1]            // w4: g1
>> +        ldrb            w7, [x3, #4]            // w7: g2
>> +        add             w4, w4, w7              // w4 = g1 + g2
>> +
>> +        ldrb            w7, [x3, #2]            // w7: b1
>> +        ldrb            w8, [x3, #5]            // w8: b2
>> +        add             w7, w7, w8              // w7 = b1 + b2
>> +
>> +        smaddl          x8, w2, w10, x9         // dst_u = ru * r + const_offset
>> +        smaddl          x8, w4, w11, x8         // dst_u += gu * g
>> +        smaddl          x8, w7, w12, x8         // dst_u += bu * b
>> +        asr             x8, x8, #10             // dst_u >>= 10
>> +        strh            w8, [x0], #2            // store dst_u
>> +
>> +        smaddl          x8, w2, w13, x9         // dst_v = rv * r + const_offset
>> +        smaddl          x8, w4, w14, x8         // dst_v += gv * g
>> +        smaddl          x8, w7, w15, x8         // dst_v += bv * b
>> +        asr             x8, x8, #10             // dst_v >>= 10
>> +        sub             w5, w5, #1
>> +        add             x3, x3, #6              // src += 6
>> +        strh            w8, [x1], #2            // store dst_v
>> +        cbnz            w5, 2b
> 
> same thing about sub+cbnz vs subs+b.gt
> 
>> +3:
>> +        ret
>> +endfunc
>> +
>> +function ff_rgb24ToUV_neon, export=1
>> +        cmp             w5, #0                  // check width > 0
>> +        b.le            3f
>> +
>> +        cmp             w5, #16
>> +        rgb24_load_uv_coeff half=0
>> +        b.lt            2f
>> +1:
>> +        rgb24_to_yuv_load_rgb x3
>> +        rgb24_to_yuv_product v19, v20, v21, v25, v26, v16, v0, v1, v2, #9
>> +        rgb24_to_yuv_product v22, v23, v24, v27, v28, v17, v0, v1, v2, #9
>> +        rgb24_to_yuv_product v19, v20, v21, v25, v26, v18, v3, v4, v5, #9
>> +        rgb24_to_yuv_product v22, v23, v24, v27, v28, v19, v3, v4, v5, #9
>> +        sub             w5, w5, #16
>> +        add             x3, x3, #48             // src += 48
>> +        cmp             w5, #16
>> +        stp             q16, q17, [x0], #32      // store to dst_u
>> +        stp             q18, q19, [x1], #32      // store to dst_v
> 
> These comments seem to be misaligned with the rest of the comments
> 
>> +        b.ge            1b
>> +        cbz             w5, 3f
>> +2:
>> +        ldrb            w16, [x3]               // w16: r
>> +        ldrb            w17, [x3, #1]           // w17: g
>> +        ldrb            w4, [x3, #2]            // w4: b
>> +
>> +        smaddl          x8, w16, w10, x9        // x8 = ru * r + const_offset
>> +        smaddl          x8, w17, w11, x8        // x8 += gu * g
>> +        smaddl          x8, w4, w12, x8         // x8 += bu * b
>> +        asr             w8, w8, #9              // x8 >>= 9
>> +        strh            w8, [x0], #2            // store to dst_u
>> +
>> +        smaddl          x8, w16, w13, x9        // x8 = rv * r + const_offset
>> +        smaddl          x8, w17, w14, x8        // x8 += gv * g
>> +        smaddl          x8, w4, w15, x8         // x8 += bv * b
>> +        asr             w8, w8, #9              // x8 >>= 9
>> +        sub             w5, w5, #1              // width--
>> +        add             x3, x3, #3              // src += 3
>> +        strh            w8, [x1], #2            // store to dst_v
>> +        cbnz            w5, 2b
> 
> Same about the counter
> 
> Other than those very minor remarks, this looks quite good to me now, thanks!
> 
> // Martin
> 
> _______________________________________________
> 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".