| Message ID | 20220428134211.4786-3-jdek@itanimul.li |
|---|---|
| State | Accepted |
| Commit | 2e832be322eb456e44b1e928904fa470a0b00a67 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavc/aarch64: fix hevc sao band filter | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
On Thu, 28 Apr 2022, J. Dekker wrote: > bench on AWS Graviton: > > hevc_sao_edge_8x8_8_c: 516.0 > hevc_sao_edge_8x8_8_neon: 81.0 > > Signed-off-by: J. Dekker <jdek@itanimul.li> > --- > libavcodec/aarch64/hevcdsp_init_aarch64.c | 3 ++ > libavcodec/aarch64/hevcdsp_sao_neon.S | 51 +++++++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c b/libavcodec/aarch64/hevcdsp_init_aarch64.c > index df521bb083..2002530266 100644 > --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c > +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c > @@ -59,6 +59,8 @@ void ff_hevc_sao_band_filter_8x8_8_neon(uint8_t *_dst, uint8_t *_src, > int width, int height); > void ff_hevc_sao_edge_filter_16x16_8_neon(uint8_t *dst, uint8_t *src, ptrdiff_t stride_dst, > int16_t *sao_offset_val, int eo, int width, int height); > +void ff_hevc_sao_edge_filter_8x8_8_neon(uint8_t *dst, uint8_t *src, ptrdiff_t stride_dst, > + int16_t *sao_offset_val, int eo, int width, int height); > > av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth) > { > @@ -80,6 +82,7 @@ av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth) > c->sao_band_filter[2] = > c->sao_band_filter[3] = > c->sao_band_filter[4] = ff_hevc_sao_band_filter_8x8_8_neon; > + c->sao_edge_filter[0] = ff_hevc_sao_edge_filter_8x8_8_neon; > c->sao_edge_filter[1] = > c->sao_edge_filter[2] = > c->sao_edge_filter[3] = > diff --git a/libavcodec/aarch64/hevcdsp_sao_neon.S b/libavcodec/aarch64/hevcdsp_sao_neon.S > index 0315c479df..efd8112af4 100644 > --- a/libavcodec/aarch64/hevcdsp_sao_neon.S > +++ b/libavcodec/aarch64/hevcdsp_sao_neon.S > @@ -140,3 +140,54 @@ function ff_hevc_sao_edge_filter_16x16_8_neon, export=1 > // no lines to filter > ret > endfunc > + > +// ff_hevc_sao_edge_filter_8x8_8_neon(char *dst, char *src, ptrdiff stride_dst, > +// int16 *sao_offset_val, int eo, int width, int height) > +function ff_hevc_sao_edge_filter_8x8_8_neon, export=1 > + adr x7, .Lsao_edge_pos > + ldr w4, [x7, w4, uxtw #2] > + ld1 {v3.8h}, [x3] > + mov v3.h[7], v3.h[0] > + mov v3.h[0], v3.h[1] > + mov v3.h[1], v3.h[2] > + mov v3.h[2], v3.h[7] > + uzp2 v1.16b, v3.16b, v3.16b > + uzp1 v0.16b, v3.16b, v3.16b > + movi v2.16b, #2 > + add x16, x0, x2 > + lsl x2, x2, #1 > + mov x15, #192 > + mov x8, x1 > + sub x9, x1, x4 > + add x10, x1, x4 > + lsr w17, w6, #1 Compared with the previously applied (and reverted) patch, here, you previously had "mov x17, #4". I guess that'd mean the function only ever produced 8 output rows, while it now uses the real height parameter? Was this change a no-op (height is always 8?) or was this another hidden bug in the previous implementation? > +1: ld1 {v3.d}[0], [ x8], x15 > + ld1 {v4.d}[0], [ x9], x15 > + ld1 {v5.d}[0], [x10], x15 > + ld1 {v3.d}[1], [ x8], x15 > + ld1 {v4.d}[1], [ x9], x15 > + ld1 {v5.d}[1], [x10], x15 > + cmhi v16.16b, v4.16b, v3.16b > + cmhi v17.16b, v3.16b, v4.16b > + cmhi v18.16b, v5.16b, v3.16b > + cmhi v19.16b, v3.16b, v5.16b > + sub v20.16b, v16.16b, v17.16b > + sub v21.16b, v18.16b, v19.16b > + add v20.16b, v20.16b, v21.16b > + add v20.16b, v20.16b, v2.16b > + tbl v16.16b, {v0.16b}, v20.16b > + tbl v17.16b, {v1.16b}, v20.16b > + uxtl v20.8h, v3.8b > + uxtl2 v21.8h, v3.16b > + zip1 v18.16b, v16.16b, v17.16b > + zip2 v19.16b, v16.16b, v17.16b > + sqadd v20.8h, v18.8h, v20.8h > + sqadd v21.8h, v19.8h, v21.8h > + sqxtun v6.8b, v20.8h > + sqxtun v7.8b, v21.8h > + st1 {v6.8b}, [ x0], x2 > + st1 {v7.8b}, [x16], x2 > + subs x17, x17, #1 This could be "subs w6, w6, #2" and you wouldn't need the lsr instruction at all. And you could place the subs before the two st1 instructions to reduce latency between them a little. (The same thing goes for moving subs further away from the branch that uses its outcome in the previous patch too.) But as this is just a reapply of a previously committed and reverted patch, I guess it's fine this way too... The patchset otherwise looks good to me, modulo the question about the difference to the previous patchset above. // Martin
On 28 Apr 2022, at 21:50, Martin Storsjö wrote: > [...] > Compared with the previously applied (and reverted) patch, here, you previously had "mov x17, #4". I guess that'd mean the function only ever produced 8 output rows, while it now uses the real height parameter? Was this change a no-op (height is always 8?) or was this another hidden bug in the previous implementation? > Yes, this was another bug in a previous implementation which I've fixed in both of the newer versions. >> [...] >> + sqxtun v6.8b, v20.8h >> + sqxtun v7.8b, v21.8h >> + st1 {v6.8b}, [ x0], x2 >> + st1 {v7.8b}, [x16], x2 >> + subs x17, x17, #1 > > This could be "subs w6, w6, #2" and you wouldn't need the lsr instruction at all. And you could place the subs before the two st1 instructions to reduce latency between them a little. (The same thing goes for moving subs further away from the branch that uses its outcome in the previous patch too.) But as this is just a reapply of a previously committed and reverted patch, I guess it's fine this way too... Will do before apply if you're fine with it, not too complex change. > The patchset otherwise looks good to me, modulo the question about the difference to the previous patchset above. -- J. Dekker
diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c b/libavcodec/aarch64/hevcdsp_init_aarch64.c index df521bb083..2002530266 100644 --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c @@ -59,6 +59,8 @@ void ff_hevc_sao_band_filter_8x8_8_neon(uint8_t *_dst, uint8_t *_src, int width, int height); void ff_hevc_sao_edge_filter_16x16_8_neon(uint8_t *dst, uint8_t *src, ptrdiff_t stride_dst, int16_t *sao_offset_val, int eo, int width, int height); +void ff_hevc_sao_edge_filter_8x8_8_neon(uint8_t *dst, uint8_t *src, ptrdiff_t stride_dst, + int16_t *sao_offset_val, int eo, int width, int height); av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth) { @@ -80,6 +82,7 @@ av_cold void ff_hevc_dsp_init_aarch64(HEVCDSPContext *c, const int bit_depth) c->sao_band_filter[2] = c->sao_band_filter[3] = c->sao_band_filter[4] = ff_hevc_sao_band_filter_8x8_8_neon; + c->sao_edge_filter[0] = ff_hevc_sao_edge_filter_8x8_8_neon; c->sao_edge_filter[1] = c->sao_edge_filter[2] = c->sao_edge_filter[3] = diff --git a/libavcodec/aarch64/hevcdsp_sao_neon.S b/libavcodec/aarch64/hevcdsp_sao_neon.S index 0315c479df..efd8112af4 100644 --- a/libavcodec/aarch64/hevcdsp_sao_neon.S +++ b/libavcodec/aarch64/hevcdsp_sao_neon.S @@ -140,3 +140,54 @@ function ff_hevc_sao_edge_filter_16x16_8_neon, export=1 // no lines to filter ret endfunc + +// ff_hevc_sao_edge_filter_8x8_8_neon(char *dst, char *src, ptrdiff stride_dst, +// int16 *sao_offset_val, int eo, int width, int height) +function ff_hevc_sao_edge_filter_8x8_8_neon, export=1 + adr x7, .Lsao_edge_pos + ldr w4, [x7, w4, uxtw #2] + ld1 {v3.8h}, [x3] + mov v3.h[7], v3.h[0] + mov v3.h[0], v3.h[1] + mov v3.h[1], v3.h[2] + mov v3.h[2], v3.h[7] + uzp2 v1.16b, v3.16b, v3.16b + uzp1 v0.16b, v3.16b, v3.16b + movi v2.16b, #2 + add x16, x0, x2 + lsl x2, x2, #1 + mov x15, #192 + mov x8, x1 + sub x9, x1, x4 + add x10, x1, x4 + lsr w17, w6, #1 +1: ld1 {v3.d}[0], [ x8], x15 + ld1 {v4.d}[0], [ x9], x15 + ld1 {v5.d}[0], [x10], x15 + ld1 {v3.d}[1], [ x8], x15 + ld1 {v4.d}[1], [ x9], x15 + ld1 {v5.d}[1], [x10], x15 + cmhi v16.16b, v4.16b, v3.16b + cmhi v17.16b, v3.16b, v4.16b + cmhi v18.16b, v5.16b, v3.16b + cmhi v19.16b, v3.16b, v5.16b + sub v20.16b, v16.16b, v17.16b + sub v21.16b, v18.16b, v19.16b + add v20.16b, v20.16b, v21.16b + add v20.16b, v20.16b, v2.16b + tbl v16.16b, {v0.16b}, v20.16b + tbl v17.16b, {v1.16b}, v20.16b + uxtl v20.8h, v3.8b + uxtl2 v21.8h, v3.16b + zip1 v18.16b, v16.16b, v17.16b + zip2 v19.16b, v16.16b, v17.16b + sqadd v20.8h, v18.8h, v20.8h + sqadd v21.8h, v19.8h, v21.8h + sqxtun v6.8b, v20.8h + sqxtun v7.8b, v21.8h + st1 {v6.8b}, [ x0], x2 + st1 {v7.8b}, [x16], x2 + subs x17, x17, #1 + b.ne 1b + ret +endfunc
bench on AWS Graviton: hevc_sao_edge_8x8_8_c: 516.0 hevc_sao_edge_8x8_8_neon: 81.0 Signed-off-by: J. Dekker <jdek@itanimul.li> --- libavcodec/aarch64/hevcdsp_init_aarch64.c | 3 ++ libavcodec/aarch64/hevcdsp_sao_neon.S | 51 +++++++++++++++++++++++ 2 files changed, 54 insertions(+)