[PATCH 1/4] lavc/flacenc: add sse4 version of the 16-bit lpc encoder
From 1.8 to 2.4 times faster. Runtime is reduced by 2 to 39%. The speed-up generally increases with compression_level.
This lpc encoder is not used with levels < 3 so it provides no speed-up in these cases. --- LICENSE | 1 + libavcodec/flacenc.c | 2 +- libavcodec/x86/Makefile | 3 + libavcodec/x86/flac_dsp_gpl.asm | 79 +++++++++++++++++++++++++++++++++++++++ libavcodec/x86/flacdsp_init.c | 4 ++ 5 files changed, 88 insertions(+), 1 deletions(-) create mode 100644 libavcodec/x86/flac_dsp_gpl.asm diff --git a/LICENSE b/LICENSE index 1f757aa..c194087 100644 --- a/LICENSE +++ b/LICENSE @@ -15,6 +15,7 @@ Specifically, the GPL parts of FFmpeg are - libpostproc - libmpcodecs - optional x86 optimizations in the files + libavcodec/x86/flac_dsp_gpl.asm libavcodec/x86/idct_mmx.c - libutvideo encoding/decoding wrappers in libavcodec/libutvideo*.cpp diff --git a/libavcodec/flacenc.c b/libavcodec/flacenc.c index 1fc8c4c..e958cd8 100644 --- a/libavcodec/flacenc.c +++ b/libavcodec/flacenc.c @@ -80,7 +80,7 @@ typedef struct FlacSubframe { int shift; RiceContext rc; int32_t samples[FLAC_MAX_BLOCKSIZE]; - int32_t residual[FLAC_MAX_BLOCKSIZE+1]; + int32_t residual[FLAC_MAX_BLOCKSIZE+3]; } FlacSubframe; typedef struct FlacFrame { diff --git a/libavcodec/x86/Makefile b/libavcodec/x86/Makefile index 0d3594f..374b1d2 100644 --- a/libavcodec/x86/Makefile +++ b/libavcodec/x86/Makefile @@ -76,6 +76,9 @@ YASM-OBJS-$(CONFIG_DSPUTIL) += x86/dsputil.o \ YASM-OBJS-$(CONFIG_ENCODERS) += x86/dsputilenc.o YASM-OBJS-$(CONFIG_FFT) += x86/fft.o YASM-OBJS-$(CONFIG_FLAC_DECODER) += x86/flacdsp.o +ifdef CONFIG_GPL +YASM-OBJS-$(CONFIG_FLAC_ENCODER) += x86/flac_dsp_gpl.o +endif YASM-OBJS-$(CONFIG_H263DSP) += x86/h263_loopfilter.o YASM-OBJS-$(CONFIG_H264CHROMA) += x86/h264_chromamc.o \ x86/h264_chromamc_10bit.o diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm new file mode 100644 index 0000000..cc07194 --- /dev/null +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -0,0 +1,79 @@ +;***************************************************************************** +;* FLAC DSP functions +;* +;* Copyright (c) 2014 James Darnley <james.darnley@gmail.com> +;* +;* This file is part of FFmpeg. +;* +;* FFmpeg is free software; you can redistribute it and/or modify +;* it under the terms of the GNU General Public License as published by +;* the Free Software Foundation; either version 2 of the License, or +;* (at your option) any later version. +;* +;* FFmpeg is distributed in the hope that it will be useful, +;* but WITHOUT ANY WARRANTY; without even the implied warranty of +;* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;* GNU General Public License for more details. +;* +;* You should have received a copy of the GNU General Public License along +;* with FFmpeg; if not, write to the Free Software Foundation, Inc., +;* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +;****************************************************************************** + +%include "libavutil/x86/x86util.asm" + +SECTION_TEXT + +INIT_XMM sse4 +%if ARCH_X86_64 + cglobal flac_enc_lpc_16, 5, 7, 4, 0, res, smp, len, order, coefs, posj, negj + %define length r2d + + movsxd orderq, orderd +%else + cglobal flac_enc_lpc_16, 5, 6, 4, 0, res, smp, posj, order, coefs, negj + %define length r2mp +%endif + +; Is it worth looping correctly over the first samples? The most that ever need +; to be copied is 32 so we might as well just unroll the loop and do all 32. +%assign iter 0 +%rep 32/(mmsize/4) + movu m0, [smpq+iter] + movu [resq+iter], m0 + %assign iter iter+mmsize +%endrep + +lea resq, [resq+orderq*4] +lea smpq, [smpq+orderq*4] +lea coefsq, [coefsq+orderq*4] +sub length, orderd +movd m3, r5m +neg orderq + +.looplen: + pxor m0, m0 + mov posjq, orderq + xor negjq, negjq + + .looporder: + movd m2, [coefsq+posjq*4] ; c = coefs[j] + SPLATD m2 + movu m1, [smpq+negjq*4-4] ; s = smp[i-j-1] + pmulld m1, m2 + paddd m0, m1 ; p += c * s + + dec negjq + inc posjq + jnz .looporder + + psrad m0, m3 ; p >>= shift + movu m1, [smpq] + psubd m1, m0 ; smp[i] - p + movu [resq], m1 ; res[i] = smp[i] - (p >> shift) + + add resq, mmsize + add smpq, mmsize + sub length, mmsize/4 +jg .looplen +RET diff --git a/libavcodec/x86/flacdsp_init.c b/libavcodec/x86/flacdsp_init.c index a071b3d..ad88e5b 100644 --- a/libavcodec/x86/flacdsp_init.c +++ b/libavcodec/x86/flacdsp_init.c @@ -27,6 +27,8 @@ void ff_flac_lpc_32_sse4(int32_t *samples, const int coeffs[32], int order, void ff_flac_lpc_32_xop(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); +void ff_flac_enc_lpc_16_sse4(int32_t *, const int32_t *, int, int, const int32_t *,int); + av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, int bps) { @@ -36,6 +38,8 @@ av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, if (EXTERNAL_SSE4(cpu_flags)) { if (bps > 16 && CONFIG_FLAC_DECODER) c->lpc = ff_flac_lpc_32_sse4; + if (bps == 16 && CONFIG_FLAC_ENCODER && CONFIG_GPL) + c->lpc_encode = ff_flac_enc_lpc_16_sse4; } if (EXTERNAL_XOP(cpu_flags)) { if (bps > 16 && CONFIG_FLAC_DECODER) -- 1.7.9
It now does 12 samples per iteration, up from 4.
From 1.8 to 3.2 times faster again. 3.6 to 5.7 times faster overall. Runtime is reduced by a further 2 to 18%. Overall runtime reduced by 4 to 50%.
Same conditions as before apply. --- libavcodec/flacenc.c | 2 +- libavcodec/x86/flac_dsp_gpl.asm | 26 +++++++++++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/libavcodec/flacenc.c b/libavcodec/flacenc.c index e958cd8..88f57e6 100644 --- a/libavcodec/flacenc.c +++ b/libavcodec/flacenc.c @@ -80,7 +80,7 @@ typedef struct FlacSubframe { int shift; RiceContext rc; int32_t samples[FLAC_MAX_BLOCKSIZE]; - int32_t residual[FLAC_MAX_BLOCKSIZE+3]; + int32_t residual[FLAC_MAX_BLOCKSIZE+11]; } FlacSubframe; typedef struct FlacFrame { diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm index cc07194..326ba12 100644 --- a/libavcodec/x86/flac_dsp_gpl.asm +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -26,12 +26,12 @@ SECTION_TEXT INIT_XMM sse4 %if ARCH_X86_64 - cglobal flac_enc_lpc_16, 5, 7, 4, 0, res, smp, len, order, coefs, posj, negj + cglobal flac_enc_lpc_16, 5, 7, 8, 0, res, smp, len, order, coefs, posj, negj %define length r2d movsxd orderq, orderd %else - cglobal flac_enc_lpc_16, 5, 6, 4, 0, res, smp, posj, order, coefs, negj + cglobal flac_enc_lpc_16, 5, 6, 8, 0, res, smp, posj, order, coefs, negj %define length r2mp %endif @@ -53,6 +53,8 @@ neg orderq .looplen: pxor m0, m0 + pxor m4, m4 + pxor m6, m6 mov posjq, orderq xor negjq, negjq @@ -60,20 +62,34 @@ neg orderq movd m2, [coefsq+posjq*4] ; c = coefs[j] SPLATD m2 movu m1, [smpq+negjq*4-4] ; s = smp[i-j-1] + movu m5, [smpq+negjq*4-4+mmsize] + movu m7, [smpq+negjq*4-4+mmsize*2] pmulld m1, m2 + pmulld m5, m2 + pmulld m7, m2 paddd m0, m1 ; p += c * s + paddd m4, m5 + paddd m6, m7 dec negjq inc posjq jnz .looporder psrad m0, m3 ; p >>= shift + psrad m4, m3 + psrad m6, m3 movu m1, [smpq] + movu m5, [smpq+mmsize] + movu m7, [smpq+mmsize*2] psubd m1, m0 ; smp[i] - p + psubd m5, m4 + psubd m7, m6 movu [resq], m1 ; res[i] = smp[i] - (p >> shift) + movu [resq+mmsize], m5 + movu [resq+mmsize*2], m7 - add resq, mmsize - add smpq, mmsize - sub length, mmsize/4 + add resq, 3*mmsize + add smpq, 3*mmsize + sub length, (3*mmsize)/4 jg .looplen RET -- 1.7.9
From 1.3 to 2.5 times faster. Runtime reduced by 4 to 58%. As with the 16-bit version the speed-up generally increases with compression_level.
Also like the 16-bit version, it is not used with levels less than 3. --- libavcodec/x86/flac_dsp_gpl.asm | 95 +++++++++++++++++++++++++++++++++++++++ libavcodec/x86/flacdsp_init.c | 5 ++ 2 files changed, 100 insertions(+), 0 deletions(-) diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm index 326ba12..ff63539 100644 --- a/libavcodec/x86/flac_dsp_gpl.asm +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -22,6 +22,12 @@ %include "libavutil/x86/x86util.asm" +SECTION_RODATA + +pd_0_int_min: times 2 dd 0, -2147483648 +pq_int_min: times 2 dq -2147483648 +pq_int_max: times 2 dq 2147483647 + SECTION_TEXT INIT_XMM sse4 @@ -93,3 +99,92 @@ neg orderq sub length, (3*mmsize)/4 jg .looplen RET + +%macro PMINSQ 3 + pcmpgtq %3, %2, %1 + pand %1, %3 + pandn %3, %2 + por %1, %3 +%endmacro + +%macro PMAXSQ 3 + pcmpgtq %3, %1, %2 + pand %1, %3 + pandn %3, %2 + por %1, %3 +%endmacro + +%macro CLIPQ 4 ; reg, min, max, tmp + PMAXSQ %1, %2, %4 + PMINSQ %1, %3, %4 +%endmacro + +%macro HACK_PSRAQ 4 ; dst, src (shift), sign extend mask, tmp + pxor %4, %4 ; zero + pcmpgtq %4, %1 ; mask where 0 > dst + pand %4, %3 ; mask & sign extend mask + psrlq %1, %2 ; dst >>= shift + por %1, %4 ; dst | mask +%endmacro + +INIT_XMM sse42 +%if ARCH_X86_64 + cglobal flac_enc_lpc_32, 5, 7, 4, mmsize, res, smp, len, order, coefs, posj, negj + %define length r2d + + movsxd orderq, orderd +%else + cglobal flac_enc_lpc_32, 5, 6, 4, mmsize, res, smp, posj, order, coefs, negj + %define length r2mp +%endif + +; Is it worth looping correctly over the first samples? The most that ever need +; to be copied is 32 so we might as well just unroll the loop and do all 32. +%assign iter 0 +%rep 32/(mmsize/4) + movu m0, [smpq+iter] + movu [resq+iter], m0 + %assign iter iter+mmsize +%endrep + +lea resq, [resq+orderq*4] +lea smpq, [smpq+orderq*4] +lea coefsq, [coefsq+orderq*4] +sub length, orderd +movd m3, r5m +neg orderq + +movu m4, [pd_0_int_min] ; load 1 bit +psrad m4, m3 ; turn that into shift+1 bits +pslld m4, 1 ; reduce that +mova [rsp], m4 ; save sign extend mask + +.looplen: + pxor m0, m0 + mov posjq, orderq + xor negjq, negjq + + .looporder: + movd m2, [coefsq+posjq*4] ; c = coefs[j] + SPLATD m2 + movh m1, [smpq+negjq*4-4] ; s = smp[i-j-1] + pshufd m1, m1, q3130 + pmuldq m1, m2 + paddq m0, m1 ; p += c * s + + dec negjq + inc posjq + jnz .looporder + + HACK_PSRAQ m0, m3, [rsp], m2 ; p >>= shift + CLIPQ m0, [pq_int_min], [pq_int_max], m2 ; clip(p >> shift) + pshufd m0, m0, q0020 ; pack into first 2 dwords + movh m1, [smpq] + psubd m1, m0 ; smp[i] - p + movh [resq], m1 ; res[i] = smp[i] - (p >> shift) + + add resq, mmsize/2 + add smpq, mmsize/2 + sub length, mmsize/8 +jg .looplen +RET diff --git a/libavcodec/x86/flacdsp_init.c b/libavcodec/x86/flacdsp_init.c index ad88e5b..976ea2c 100644 --- a/libavcodec/x86/flacdsp_init.c +++ b/libavcodec/x86/flacdsp_init.c @@ -28,6 +28,7 @@ void ff_flac_lpc_32_xop(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); void ff_flac_enc_lpc_16_sse4(int32_t *, const int32_t *, int, int, const int32_t *,int); +void ff_flac_enc_lpc_32_sse42(int32_t *, const int32_t *, int, int, const int32_t *,int); av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, int bps) @@ -45,5 +46,9 @@ av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, if (bps > 16 && CONFIG_FLAC_DECODER) c->lpc = ff_flac_lpc_32_xop; } + if (EXTERNAL_SSE42(cpu_flags)) { + if (bps > 16 && CONFIG_FLAC_ENCODER && CONFIG_GPL) + c->lpc_encode = ff_flac_enc_lpc_32_sse42; + } #endif } -- 1.7.9
Now does 6 samples per iteration, up from 2.
From 1.6 to 2.1 times faster again. 2.5 to 3.9 times faster overall. Runtime is reduced by a further 4 to 17%. Reduced by 9 to 65% overall.
Same conditions as previously. --- libavcodec/x86/flac_dsp_gpl.asm | 32 +++++++++++++++++++++++++++----- 1 files changed, 27 insertions(+), 5 deletions(-) diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm index ff63539..c935c79 100644 --- a/libavcodec/x86/flac_dsp_gpl.asm +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -129,12 +129,12 @@ RET INIT_XMM sse42 %if ARCH_X86_64 - cglobal flac_enc_lpc_32, 5, 7, 4, mmsize, res, smp, len, order, coefs, posj, negj + cglobal flac_enc_lpc_32, 5, 7, 8, mmsize, res, smp, len, order, coefs, posj, negj %define length r2d movsxd orderq, orderd %else - cglobal flac_enc_lpc_32, 5, 6, 4, mmsize, res, smp, posj, order, coefs, negj + cglobal flac_enc_lpc_32, 5, 6, 8, mmsize, res, smp, posj, order, coefs, negj %define length r2mp %endif @@ -161,6 +161,8 @@ mova [rsp], m4 ; save sign extend mask .looplen: pxor m0, m0 + pxor m4, m4 + pxor m6, m6 mov posjq, orderq xor negjq, negjq @@ -168,23 +170,43 @@ mova [rsp], m4 ; save sign extend mask movd m2, [coefsq+posjq*4] ; c = coefs[j] SPLATD m2 movh m1, [smpq+negjq*4-4] ; s = smp[i-j-1] + movh m5, [smpq+negjq*4-4+mmsize/2] + movh m7, [smpq+negjq*4-4+mmsize] pshufd m1, m1, q3130 + pshufd m5, m5, q3130 + pshufd m7, m7, q3130 pmuldq m1, m2 + pmuldq m5, m2 + pmuldq m7, m2 paddq m0, m1 ; p += c * s + paddq m4, m5 + paddq m6, m7 dec negjq inc posjq jnz .looporder HACK_PSRAQ m0, m3, [rsp], m2 ; p >>= shift + HACK_PSRAQ m4, m3, [rsp], m2 + HACK_PSRAQ m6, m3, [rsp], m2 CLIPQ m0, [pq_int_min], [pq_int_max], m2 ; clip(p >> shift) + CLIPQ m4, [pq_int_min], [pq_int_max], m2 + CLIPQ m6, [pq_int_min], [pq_int_max], m2 pshufd m0, m0, q0020 ; pack into first 2 dwords + pshufd m4, m4, q0020 + pshufd m6, m6, q0020 movh m1, [smpq] + movh m5, [smpq+mmsize/2] + movh m7, [smpq+mmsize] psubd m1, m0 ; smp[i] - p + psubd m5, m4 + psubd m7, m6 movh [resq], m1 ; res[i] = smp[i] - (p >> shift) + movh [resq+mmsize/2], m5 + movh [resq+mmsize], m7 - add resq, mmsize/2 - add smpq, mmsize/2 - sub length, mmsize/8 + add resq, (3*mmsize)/2 + add smpq, (3*mmsize)/2 + sub length, (3*mmsize)/8 jg .looplen RET -- 1.7.9
On Wed, Mar 12, 2014 at 01:03:03PM +0100, James Darnley wrote:
From 1.8 to 2.4 times faster. Runtime is reduced by 2 to 39%. The speed-up generally increases with compression_level.
This lpc encoder is not used with levels < 3 so it provides no speed-up in these cases. --- LICENSE | 1 + libavcodec/flacenc.c | 2 +- libavcodec/x86/Makefile | 3 + libavcodec/x86/flac_dsp_gpl.asm | 79 +++++++++++++++++++++++++++++++++++++++ libavcodec/x86/flacdsp_init.c | 4 ++ 5 files changed, 88 insertions(+), 1 deletions(-) create mode 100644 libavcodec/x86/flac_dsp_gpl.asm
diff --git a/LICENSE b/LICENSE index 1f757aa..c194087 100644 --- a/LICENSE +++ b/LICENSE @@ -15,6 +15,7 @@ Specifically, the GPL parts of FFmpeg are - libpostproc - libmpcodecs - optional x86 optimizations in the files + libavcodec/x86/flac_dsp_gpl.asm libavcodec/x86/idct_mmx.c - libutvideo encoding/decoding wrappers in libavcodec/libutvideo*.cpp diff --git a/libavcodec/flacenc.c b/libavcodec/flacenc.c index 1fc8c4c..e958cd8 100644 --- a/libavcodec/flacenc.c +++ b/libavcodec/flacenc.c @@ -80,7 +80,7 @@ typedef struct FlacSubframe { int shift; RiceContext rc; int32_t samples[FLAC_MAX_BLOCKSIZE]; - int32_t residual[FLAC_MAX_BLOCKSIZE+1]; + int32_t residual[FLAC_MAX_BLOCKSIZE+3]; } FlacSubframe;
typedef struct FlacFrame { diff --git a/libavcodec/x86/Makefile b/libavcodec/x86/Makefile index 0d3594f..374b1d2 100644 --- a/libavcodec/x86/Makefile +++ b/libavcodec/x86/Makefile @@ -76,6 +76,9 @@ YASM-OBJS-$(CONFIG_DSPUTIL) += x86/dsputil.o \ YASM-OBJS-$(CONFIG_ENCODERS) += x86/dsputilenc.o YASM-OBJS-$(CONFIG_FFT) += x86/fft.o YASM-OBJS-$(CONFIG_FLAC_DECODER) += x86/flacdsp.o +ifdef CONFIG_GPL +YASM-OBJS-$(CONFIG_FLAC_ENCODER) += x86/flac_dsp_gpl.o +endif YASM-OBJS-$(CONFIG_H263DSP) += x86/h263_loopfilter.o YASM-OBJS-$(CONFIG_H264CHROMA) += x86/h264_chromamc.o \ x86/h264_chromamc_10bit.o diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm new file mode 100644 index 0000000..cc07194 --- /dev/null +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -0,0 +1,79 @@ +;***************************************************************************** +;* FLAC DSP functions +;* +;* Copyright (c) 2014 James Darnley <james.darnley@gmail.com> +;* +;* This file is part of FFmpeg. +;* +;* FFmpeg is free software; you can redistribute it and/or modify +;* it under the terms of the GNU General Public License as published by +;* the Free Software Foundation; either version 2 of the License, or +;* (at your option) any later version. +;* +;* FFmpeg is distributed in the hope that it will be useful, +;* but WITHOUT ANY WARRANTY; without even the implied warranty of +;* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;* GNU General Public License for more details. +;* +;* You should have received a copy of the GNU General Public License along +;* with FFmpeg; if not, write to the Free Software Foundation, Inc., +;* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +;****************************************************************************** + +%include "libavutil/x86/x86util.asm" + +SECTION_TEXT + +INIT_XMM sse4 +%if ARCH_X86_64 + cglobal flac_enc_lpc_16, 5, 7, 4, 0, res, smp, len, order, coefs, posj, negj + %define length r2d + + movsxd orderq, orderd +%else + cglobal flac_enc_lpc_16, 5, 6, 4, 0, res, smp, posj, order, coefs, negj + %define length r2mp +%endif + +; Is it worth looping correctly over the first samples? The most that ever need +; to be copied is 32 so we might as well just unroll the loop and do all 32.
implementations should not make assumtations on their use except what is documented in the API or the other way around if some limitation is always true and you want to write an implementation that takes advantage of the limitation for optimization then this limitation should be documented in the API first (in this case of FLACDSPContext / lpc_encode) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
On 2014-03-15 00:01, Michael Niedermayer wrote:
On Wed, Mar 12, 2014 at 01:03:03PM +0100, James Darnley wrote:
+; Is it worth looping correctly over the first samples? The most that ever need +; to be copied is 32 so we might as well just unroll the loop and do all 32.
implementations should not make assumtations on their use except what is documented in the API or the other way around if some limitation is always true and you want to write an implementation that takes advantage of the limitation for optimization then this limitation should be documented in the API first (in this case of FLACDSPContext / lpc_encode)
So... I've been bored lately and thought I'd come back to this. I've got a changed version which copies these samples in a loop. You can see the changes in these two links:
https://gitorious.org/ffmpeg/jdarnley-ffmpeg/commit/9604911fbbe864cd0f670bda... https://gitorious.org/ffmpeg/jdarnley-ffmpeg/commit/837879d34100113105f09999...
These two are just about the same but apply to 16 and 32-bit. Should I try to measure the difference between the two? Or should I just submit one version, possibly with suitable documentation?
On Mon, Jul 21, 2014 at 12:32:23AM +0200, James Darnley wrote:
On 2014-03-15 00:01, Michael Niedermayer wrote:
On Wed, Mar 12, 2014 at 01:03:03PM +0100, James Darnley wrote:
+; Is it worth looping correctly over the first samples? The most that ever need +; to be copied is 32 so we might as well just unroll the loop and do all 32.
implementations should not make assumtations on their use except what is documented in the API or the other way around if some limitation is always true and you want to write an implementation that takes advantage of the limitation for optimization then this limitation should be documented in the API first (in this case of FLACDSPContext / lpc_encode)
So... I've been bored lately and thought I'd come back to this. I've got a changed version which copies these samples in a loop. You can see the changes in these two links:
https://gitorious.org/ffmpeg/jdarnley-ffmpeg/commit/9604911fbbe864cd0f670bda... https://gitorious.org/ffmpeg/jdarnley-ffmpeg/commit/837879d34100113105f09999...
These two are just about the same but apply to 16 and 32-bit.
Should I try to measure the difference between the two? Or should I just submit one version, possibly with suitable documentation?
fastest is best, and docs must match implementation but docs can be changed for internal API Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
On 2014-07-21 01:48, Michael Niedermayer wrote:
On Mon, Jul 21, 2014 at 12:32:23AM +0200, James Darnley wrote:
On 2014-03-15 00:01, Michael Niedermayer wrote:
On Wed, Mar 12, 2014 at 01:03:03PM +0100, James Darnley wrote:
+; Is it worth looping correctly over the first samples? The most that ever need +; to be copied is 32 so we might as well just unroll the loop and do all 32.
implementations should not make assumtations on their use except what is documented in the API or the other way around if some limitation is always true and you want to write an implementation that takes advantage of the limitation for optimization then this limitation should be documented in the API first (in this case of FLACDSPContext / lpc_encode)
So... I've been bored lately and thought I'd come back to this. I've got a changed version which copies these samples in a loop. You can see the changes in these two links:
https://gitorious.org/ffmpeg/jdarnley-ffmpeg/commit/9604911fbbe864cd0f670bda... https://gitorious.org/ffmpeg/jdarnley-ffmpeg/commit/837879d34100113105f09999...
These two are just about the same but apply to 16 and 32-bit.
Should I try to measure the difference between the two? Or should I just submit one version, possibly with suitable documentation?
fastest is best, and docs must match implementation but docs can be changed for internal API
Testing showed no reliable difference. To be specific: the new code, when measured, showed a runtime decrease of about 0.2% (+/- 0.2), yet the function took a little more time to run (also with a similarly large error). Having done this I will submit the patches again using the old code but with some small changes and documentation of its limits. I will also add some further documentation about the C code because its unrolled function (used in the not CONFIG_SMALL case) also assumes a maximum order of 32.
From 1.8 to 2.4 times faster. Runtime is reduced by 2 to 39%. The speed-up generally increases with compression_level.
This lpc encoder is not used with levels < 3 so it provides no speed-up in these cases. --- LICENSE.md | 1 + libavcodec/flacenc.c | 2 +- libavcodec/x86/Makefile | 3 + libavcodec/x86/flac_dsp_gpl.asm | 85 +++++++++++++++++++++++++++++++++++++++ libavcodec/x86/flacdsp_init.c | 4 ++ 5 files changed, 94 insertions(+), 1 deletions(-) create mode 100644 libavcodec/x86/flac_dsp_gpl.asm diff --git a/LICENSE.md b/LICENSE.md index 04decbf..e78d932 100644 --- a/LICENSE.md +++ b/LICENSE.md @@ -15,6 +15,7 @@ Specifically, the GPL parts of FFmpeg are: - libpostproc - libmpcodecs - optional x86 optimizations in the files + libavcodec/x86/flac_dsp_gpl.asm libavcodec/x86/idct_mmx.c - libutvideo encoding/decoding wrappers in libavcodec/libutvideo*.cpp diff --git a/libavcodec/flacenc.c b/libavcodec/flacenc.c index 3a3b2ae..f37bab8 100644 --- a/libavcodec/flacenc.c +++ b/libavcodec/flacenc.c @@ -80,7 +80,7 @@ typedef struct FlacSubframe { int shift; RiceContext rc; int32_t samples[FLAC_MAX_BLOCKSIZE]; - int32_t residual[FLAC_MAX_BLOCKSIZE+1]; + int32_t residual[FLAC_MAX_BLOCKSIZE+3]; } FlacSubframe; typedef struct FlacFrame { diff --git a/libavcodec/x86/Makefile b/libavcodec/x86/Makefile index 53e643b..4298262 100644 --- a/libavcodec/x86/Makefile +++ b/libavcodec/x86/Makefile @@ -83,6 +83,9 @@ YASM-OBJS-$(CONFIG_DIRAC_DECODER) += x86/diracdsp_mmx.o x86/diracdsp_yasm.o YASM-OBJS-$(CONFIG_DNXHD_ENCODER) += x86/dnxhdenc.o YASM-OBJS-$(CONFIG_FFT) += x86/fft.o YASM-OBJS-$(CONFIG_FLAC_DECODER) += x86/flacdsp.o +ifdef CONFIG_GPL +YASM-OBJS-$(CONFIG_FLAC_ENCODER) += x86/flac_dsp_gpl.o +endif YASM-OBJS-$(CONFIG_H263DSP) += x86/h263_loopfilter.o YASM-OBJS-$(CONFIG_H264CHROMA) += x86/h264_chromamc.o \ x86/h264_chromamc_10bit.o diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm new file mode 100644 index 0000000..1f28be1 --- /dev/null +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -0,0 +1,85 @@ +;****************************************************************************** +;* FLAC DSP functions +;* +;* Copyright (c) 2014 James Darnley <james.darnley@gmail.com> +;* +;* This file is part of FFmpeg. +;* +;* FFmpeg is free software; you can redistribute it and/or modify +;* it under the terms of the GNU General Public License as published by +;* the Free Software Foundation; either version 2 of the License, or +;* (at your option) any later version. +;* +;* FFmpeg is distributed in the hope that it will be useful, +;* but WITHOUT ANY WARRANTY; without even the implied warranty of +;* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;* GNU General Public License for more details. +;* +;* You should have received a copy of the GNU General Public License along +;* with FFmpeg; if not, write to the Free Software Foundation, Inc., +;* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +;****************************************************************************** + +%include "libavutil/x86/x86util.asm" + +SECTION_TEXT + +INIT_XMM sse4 +%if ARCH_X86_64 + cglobal flac_enc_lpc_16, 5, 7, 4, 0, res, smp, len, order, coefs + DECLARE_REG_TMP 5, 6 + %define length r2d + + movsxd orderq, orderd +%else + cglobal flac_enc_lpc_16, 5, 6, 4, 0, res, smp, len, order, coefs + DECLARE_REG_TMP 2, 5 + %define length r2mp +%endif + +; Here we assume that the maximum order value is 32. This means that we only +; need to copy a maximum of 32 samples. Therefore we let the preprocessor +; unroll this loop and copy all 32. +%assign iter 0 +%rep 32/(mmsize/4) + movu m0, [smpq+iter] + movu [resq+iter], m0 + %assign iter iter+mmsize +%endrep + +lea resq, [resq+orderq*4] +lea smpq, [smpq+orderq*4] +lea coefsq, [coefsq+orderq*4] +sub length, orderd +movd m3, r5m +neg orderq + +%define posj t0q +%define negj t1q + +.looplen: + pxor m0, m0 + mov posj, orderq + xor negj, negj + + .looporder: + movd m2, [coefsq+posj*4] ; c = coefs[j] + SPLATD m2 + movu m1, [smpq+negj*4-4] ; s = smp[i-j-1] + pmulld m1, m2 + paddd m0, m1 ; p += c * s + + dec negj + inc posj + jnz .looporder + + psrad m0, m3 ; p >>= shift + movu m1, [smpq] + psubd m1, m0 ; smp[i] - p + movu [resq], m1 ; res[i] = smp[i] - (p >> shift) + + add resq, mmsize + add smpq, mmsize + sub length, mmsize/4 +jg .looplen +RET diff --git a/libavcodec/x86/flacdsp_init.c b/libavcodec/x86/flacdsp_init.c index a071b3d..ad88e5b 100644 --- a/libavcodec/x86/flacdsp_init.c +++ b/libavcodec/x86/flacdsp_init.c @@ -27,6 +27,8 @@ void ff_flac_lpc_32_sse4(int32_t *samples, const int coeffs[32], int order, void ff_flac_lpc_32_xop(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); +void ff_flac_enc_lpc_16_sse4(int32_t *, const int32_t *, int, int, const int32_t *,int); + av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, int bps) { @@ -36,6 +38,8 @@ av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, if (EXTERNAL_SSE4(cpu_flags)) { if (bps > 16 && CONFIG_FLAC_DECODER) c->lpc = ff_flac_lpc_32_sse4; + if (bps == 16 && CONFIG_FLAC_ENCODER && CONFIG_GPL) + c->lpc_encode = ff_flac_enc_lpc_16_sse4; } if (EXTERNAL_XOP(cpu_flags)) { if (bps > 16 && CONFIG_FLAC_DECODER) -- 1.7.9
It now does 12 samples per iteration, up from 4.
From 1.8 to 3.2 times faster again. 3.6 to 5.7 times faster overall. Runtime is reduced by a further 2 to 18%. Overall runtime reduced by 4 to 50%.
Same conditions as before apply. --- libavcodec/flacenc.c | 2 +- libavcodec/x86/flac_dsp_gpl.asm | 26 +++++++++++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/libavcodec/flacenc.c b/libavcodec/flacenc.c index f37bab8..3b72888 100644 --- a/libavcodec/flacenc.c +++ b/libavcodec/flacenc.c @@ -80,7 +80,7 @@ typedef struct FlacSubframe { int shift; RiceContext rc; int32_t samples[FLAC_MAX_BLOCKSIZE]; - int32_t residual[FLAC_MAX_BLOCKSIZE+3]; + int32_t residual[FLAC_MAX_BLOCKSIZE+11]; } FlacSubframe; typedef struct FlacFrame { diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm index 1f28be1..cedf083 100644 --- a/libavcodec/x86/flac_dsp_gpl.asm +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -26,13 +26,13 @@ SECTION_TEXT INIT_XMM sse4 %if ARCH_X86_64 - cglobal flac_enc_lpc_16, 5, 7, 4, 0, res, smp, len, order, coefs + cglobal flac_enc_lpc_16, 5, 7, 8, 0, res, smp, len, order, coefs DECLARE_REG_TMP 5, 6 %define length r2d movsxd orderq, orderd %else - cglobal flac_enc_lpc_16, 5, 6, 4, 0, res, smp, len, order, coefs + cglobal flac_enc_lpc_16, 5, 6, 8, 0, res, smp, len, order, coefs DECLARE_REG_TMP 2, 5 %define length r2mp %endif @@ -59,6 +59,8 @@ neg orderq .looplen: pxor m0, m0 + pxor m4, m4 + pxor m6, m6 mov posj, orderq xor negj, negj @@ -66,20 +68,34 @@ neg orderq movd m2, [coefsq+posj*4] ; c = coefs[j] SPLATD m2 movu m1, [smpq+negj*4-4] ; s = smp[i-j-1] + movu m5, [smpq+negj*4-4+mmsize] + movu m7, [smpq+negj*4-4+mmsize*2] pmulld m1, m2 + pmulld m5, m2 + pmulld m7, m2 paddd m0, m1 ; p += c * s + paddd m4, m5 + paddd m6, m7 dec negj inc posj jnz .looporder psrad m0, m3 ; p >>= shift + psrad m4, m3 + psrad m6, m3 movu m1, [smpq] + movu m5, [smpq+mmsize] + movu m7, [smpq+mmsize*2] psubd m1, m0 ; smp[i] - p + psubd m5, m4 + psubd m7, m6 movu [resq], m1 ; res[i] = smp[i] - (p >> shift) + movu [resq+mmsize], m5 + movu [resq+mmsize*2], m7 - add resq, mmsize - add smpq, mmsize - sub length, mmsize/4 + add resq, 3*mmsize + add smpq, 3*mmsize + sub length, (3*mmsize)/4 jg .looplen RET -- 1.7.9
On Tue, Aug 12, 2014 at 11:22:03PM +0200, James Darnley wrote:
It now does 12 samples per iteration, up from 4.
From 1.8 to 3.2 times faster again. 3.6 to 5.7 times faster overall. Runtime is reduced by a further 2 to 18%. Overall runtime reduced by 4 to 50%.
Same conditions as before apply.
applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once" - "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
From 1.3 to 2.5 times faster. Runtime reduced by 4 to 58%. As with the 16-bit version the speed-up generally increases with compression_level.
Also like the 16-bit version, it is not used with levels less than 3. --- libavcodec/x86/flac_dsp_gpl.asm | 101 +++++++++++++++++++++++++++++++++++++++ libavcodec/x86/flacdsp_init.c | 5 ++ 2 files changed, 106 insertions(+), 0 deletions(-) diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm index cedf083..7a49fae 100644 --- a/libavcodec/x86/flac_dsp_gpl.asm +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -22,6 +22,12 @@ %include "libavutil/x86/x86util.asm" +SECTION_RODATA + +pd_0_int_min: times 2 dd 0, -2147483648 +pq_int_min: times 2 dq -2147483648 +pq_int_max: times 2 dq 2147483647 + SECTION_TEXT INIT_XMM sse4 @@ -99,3 +105,98 @@ neg orderq sub length, (3*mmsize)/4 jg .looplen RET + +%macro PMINSQ 3 + pcmpgtq %3, %2, %1 + pand %1, %3 + pandn %3, %2 + por %1, %3 +%endmacro + +%macro PMAXSQ 3 + pcmpgtq %3, %1, %2 + pand %1, %3 + pandn %3, %2 + por %1, %3 +%endmacro + +%macro CLIPQ 4 ; reg, min, max, tmp + PMAXSQ %1, %2, %4 + PMINSQ %1, %3, %4 +%endmacro + +%macro HACK_PSRAQ 4 ; dst, src (shift), sign extend mask, tmp + pxor %4, %4 ; zero + pcmpgtq %4, %1 ; mask where 0 > dst + pand %4, %3 ; mask & sign extend mask + psrlq %1, %2 ; dst >>= shift + por %1, %4 ; dst | mask +%endmacro + +INIT_XMM sse42 +%if ARCH_X86_64 + cglobal flac_enc_lpc_32, 5, 7, 4, mmsize, res, smp, len, order, coefs + DECLARE_REG_TMP 5, 6 + %define length r2d + + movsxd orderq, orderd +%else + cglobal flac_enc_lpc_32, 5, 6, 4, mmsize, res, smp, len, order, coefs + DECLARE_REG_TMP 2, 5 + %define length r2mp +%endif + +; Here we assume that the maximum order value is 32. This means that we only +; need to copy a maximum of 32 samples. Therefore we let the preprocessor +; unroll this loop and copy all 32. +%assign iter 0 +%rep 32/(mmsize/4) + movu m0, [smpq+iter] + movu [resq+iter], m0 + %assign iter iter+mmsize +%endrep + +lea resq, [resq+orderq*4] +lea smpq, [smpq+orderq*4] +lea coefsq, [coefsq+orderq*4] +sub length, orderd +movd m3, r5m +neg orderq + +movu m4, [pd_0_int_min] ; load 1 bit +psrad m4, m3 ; turn that into shift+1 bits +pslld m4, 1 ; reduce that +mova [rsp], m4 ; save sign extend mask + +%define posj t0q +%define negj t1q + +.looplen: + pxor m0, m0 + mov posj, orderq + xor negj, negj + + .looporder: + movd m2, [coefsq+posj*4] ; c = coefs[j] + SPLATD m2 + movh m1, [smpq+negj*4-4] ; s = smp[i-j-1] + pshufd m1, m1, q3130 + pmuldq m1, m2 + paddq m0, m1 ; p += c * s + + dec negj + inc posj + jnz .looporder + + HACK_PSRAQ m0, m3, [rsp], m2 ; p >>= shift + CLIPQ m0, [pq_int_min], [pq_int_max], m2 ; clip(p >> shift) + pshufd m0, m0, q0020 ; pack into first 2 dwords + movh m1, [smpq] + psubd m1, m0 ; smp[i] - p + movh [resq], m1 ; res[i] = smp[i] - (p >> shift) + + add resq, mmsize/2 + add smpq, mmsize/2 + sub length, mmsize/8 +jg .looplen +RET diff --git a/libavcodec/x86/flacdsp_init.c b/libavcodec/x86/flacdsp_init.c index ad88e5b..976ea2c 100644 --- a/libavcodec/x86/flacdsp_init.c +++ b/libavcodec/x86/flacdsp_init.c @@ -28,6 +28,7 @@ void ff_flac_lpc_32_xop(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); void ff_flac_enc_lpc_16_sse4(int32_t *, const int32_t *, int, int, const int32_t *,int); +void ff_flac_enc_lpc_32_sse42(int32_t *, const int32_t *, int, int, const int32_t *,int); av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, int bps) @@ -45,5 +46,9 @@ av_cold void ff_flacdsp_init_x86(FLACDSPContext *c, enum AVSampleFormat fmt, if (bps > 16 && CONFIG_FLAC_DECODER) c->lpc = ff_flac_lpc_32_xop; } + if (EXTERNAL_SSE42(cpu_flags)) { + if (bps > 16 && CONFIG_FLAC_ENCODER && CONFIG_GPL) + c->lpc_encode = ff_flac_enc_lpc_32_sse42; + } #endif } -- 1.7.9
Now does 6 samples per iteration, up from 2.
From 1.6 to 2.1 times faster again. 2.5 to 3.9 times faster overall. Runtime is reduced by a further 4 to 17%. Reduced by 9 to 65% overall.
Same conditions as previously. --- libavcodec/x86/flac_dsp_gpl.asm | 32 +++++++++++++++++++++++++++----- 1 files changed, 27 insertions(+), 5 deletions(-) diff --git a/libavcodec/x86/flac_dsp_gpl.asm b/libavcodec/x86/flac_dsp_gpl.asm index 7a49fae..f0674d9 100644 --- a/libavcodec/x86/flac_dsp_gpl.asm +++ b/libavcodec/x86/flac_dsp_gpl.asm @@ -135,13 +135,13 @@ RET INIT_XMM sse42 %if ARCH_X86_64 - cglobal flac_enc_lpc_32, 5, 7, 4, mmsize, res, smp, len, order, coefs + cglobal flac_enc_lpc_32, 5, 7, 8, mmsize, res, smp, len, order, coefs DECLARE_REG_TMP 5, 6 %define length r2d movsxd orderq, orderd %else - cglobal flac_enc_lpc_32, 5, 6, 4, mmsize, res, smp, len, order, coefs + cglobal flac_enc_lpc_32, 5, 6, 8, mmsize, res, smp, len, order, coefs DECLARE_REG_TMP 2, 5 %define length r2mp %endif @@ -173,6 +173,8 @@ mova [rsp], m4 ; save sign extend mask .looplen: pxor m0, m0 + pxor m4, m4 + pxor m6, m6 mov posj, orderq xor negj, negj @@ -180,23 +182,43 @@ mova [rsp], m4 ; save sign extend mask movd m2, [coefsq+posj*4] ; c = coefs[j] SPLATD m2 movh m1, [smpq+negj*4-4] ; s = smp[i-j-1] + movh m5, [smpq+negj*4-4+mmsize/2] + movh m7, [smpq+negj*4-4+mmsize] pshufd m1, m1, q3130 + pshufd m5, m5, q3130 + pshufd m7, m7, q3130 pmuldq m1, m2 + pmuldq m5, m2 + pmuldq m7, m2 paddq m0, m1 ; p += c * s + paddq m4, m5 + paddq m6, m7 dec negj inc posj jnz .looporder HACK_PSRAQ m0, m3, [rsp], m2 ; p >>= shift + HACK_PSRAQ m4, m3, [rsp], m2 + HACK_PSRAQ m6, m3, [rsp], m2 CLIPQ m0, [pq_int_min], [pq_int_max], m2 ; clip(p >> shift) + CLIPQ m4, [pq_int_min], [pq_int_max], m2 + CLIPQ m6, [pq_int_min], [pq_int_max], m2 pshufd m0, m0, q0020 ; pack into first 2 dwords + pshufd m4, m4, q0020 + pshufd m6, m6, q0020 movh m1, [smpq] + movh m5, [smpq+mmsize/2] + movh m7, [smpq+mmsize] psubd m1, m0 ; smp[i] - p + psubd m5, m4 + psubd m7, m6 movh [resq], m1 ; res[i] = smp[i] - (p >> shift) + movh [resq+mmsize/2], m5 + movh [resq+mmsize], m7 - add resq, mmsize/2 - add smpq, mmsize/2 - sub length, mmsize/8 + add resq, (3*mmsize)/2 + add smpq, (3*mmsize)/2 + sub length, (3*mmsize)/8 jg .looplen RET -- 1.7.9
--- libavcodec/flacdsp_lpc_template.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/libavcodec/flacdsp_lpc_template.c b/libavcodec/flacdsp_lpc_template.c index acdac04..5d532e0 100644 --- a/libavcodec/flacdsp_lpc_template.c +++ b/libavcodec/flacdsp_lpc_template.c @@ -154,6 +154,6 @@ static void FUNC(flac_lpc_encode_c)(int32_t *res, const int32_t *smp, int len, * * The CONFIG_SMALL code above simplifies to this, in the case of SAMPLE_SIZE * not being equal to 32 (at the present time that means for 16-bit audio). The - * code above does 2 samples per iteration. Commit bfdd5bc ( made all the way + * code above does 2 samples per iteration. Commit bfdd5bc (made all the way * back in 2007) says that way is faster. */ -- 1.7.9
On Tue, Aug 12, 2014 at 11:22:06PM +0200, James Darnley wrote:
--- libavcodec/flacdsp_lpc_template.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */ void (*lpc_encode)(int32_t *res, const int32_t *smp, int len, int order, const int32_t *coefs, int shift); } FLACDSPContext; -- 1.7.9
On Tue, Aug 12, 2014 at 11:22:07PM +0200, James Darnley wrote:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */
sounds like printf() on fridays with SSE4 this is limited to 27 characters a function either should have a limit or not have one, it should not depend on other factors People using this function must be able to tell in what cases they can use it and People optimizing the function need to know which cases their optimized code must support the API defines both [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB During times of universal deceit, telling the truth becomes a revolutionary act. -- George Orwell
On 2014-08-13 04:50, Michael Niedermayer wrote:
On Tue, Aug 12, 2014 at 11:22:07PM +0200, James Darnley wrote:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */
sounds like
printf() on fridays with SSE4 this is limited to 27 characters
a function either should have a limit or not have one, it should not depend on other factors
People using this function must be able to tell in what cases they can use it
and People optimizing the function need to know which cases their optimized code must support
the API defines both
I don't get it. FLAC only allows upto 32-order LPC. This was, apprarently, an acceptable assumption to make for the unrolled C code yet somehow I can't make the same assumption for assembly.
+ * which also assumes the maximum value of order is 0. ^^^ Is it this bit that is confusing? Because I only now see that typo. Of course that should say "32".
On Wed, Aug 13, 2014 at 10:46:45AM +0200, James Darnley wrote:
On 2014-08-13 04:50, Michael Niedermayer wrote:
On Tue, Aug 12, 2014 at 11:22:07PM +0200, James Darnley wrote:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */
sounds like
printf() on fridays with SSE4 this is limited to 27 characters
a function either should have a limit or not have one, it should not depend on other factors
People using this function must be able to tell in what cases they can use it
and People optimizing the function need to know which cases their optimized code must support
the API defines both
I don't get it. FLAC only allows upto 32-order LPC. This was, apprarently, an acceptable assumption to make for the unrolled C code yet somehow I can't make the same assumption for assembly.
theres some kind of misunderatanding here its perfectly fine to say /** * This function supports upto a order of 32 */ what i think is a really bad idea is to make the API conditional on cpu features and build flags. What if someone wants to add a SSE2 optimization that works just up to order 32 ? he cannot do it without changing the API and checking that all uses of the API are safe with the new limitation
+ * which also assumes the maximum value of order is 0. ^^^ Is it this bit that is confusing? Because I only now see that typo. Of course that should say "32".
that too [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are best at talking, realize last or never when they are wrong.
On 2014-08-13 14:05, Michael Niedermayer wrote:
On Wed, Aug 13, 2014 at 10:46:45AM +0200, James Darnley wrote:
On 2014-08-13 04:50, Michael Niedermayer wrote:
On Tue, Aug 12, 2014 at 11:22:07PM +0200, James Darnley wrote:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */
sounds like
printf() on fridays with SSE4 this is limited to 27 characters
a function either should have a limit or not have one, it should not depend on other factors
People using this function must be able to tell in what cases they can use it
and People optimizing the function need to know which cases their optimized code must support
the API defines both
I don't get it. FLAC only allows upto 32-order LPC. This was, apprarently, an acceptable assumption to make for the unrolled C code yet somehow I can't make the same assumption for assembly.
theres some kind of misunderatanding here
its perfectly fine to say
/** * This function supports upto a order of 32 */
what i think is a really bad idea is to make the API conditional on cpu features and build flags. What if someone wants to add a SSE2 optimization that works just up to order 32 ? he cannot do it without changing the API and checking that all uses of the API are safe with the new limitation
Okay I understand that. I thought that if someone wanted to re-use the function in some other encoder which might allow 64 order (for example), I should document where the limitations are. I can change the patch to state that it supports up to 32 but should I also still mention where that is assumed? What about Christophe's suggestion of changing the function definition by using "int coeffs[32]"? Personally I am in favour of something more verbose that just stating one limit.
+ * which also assumes the maximum value of order is 0. ^^^ Is it this bit that is confusing? Because I only now see that typo. Of course that should say "32".
that too
Sorry about that.
On Wed, Aug 13, 2014 at 02:30:20PM +0200, James Darnley wrote:
On 2014-08-13 14:05, Michael Niedermayer wrote:
On Wed, Aug 13, 2014 at 10:46:45AM +0200, James Darnley wrote:
On 2014-08-13 04:50, Michael Niedermayer wrote:
On Tue, Aug 12, 2014 at 11:22:07PM +0200, James Darnley wrote:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */
sounds like
printf() on fridays with SSE4 this is limited to 27 characters
a function either should have a limit or not have one, it should not depend on other factors
People using this function must be able to tell in what cases they can use it
and People optimizing the function need to know which cases their optimized code must support
the API defines both
I don't get it. FLAC only allows upto 32-order LPC. This was, apprarently, an acceptable assumption to make for the unrolled C code yet somehow I can't make the same assumption for assembly.
theres some kind of misunderatanding here
its perfectly fine to say
/** * This function supports upto a order of 32 */
what i think is a really bad idea is to make the API conditional on cpu features and build flags. What if someone wants to add a SSE2 optimization that works just up to order 32 ? he cannot do it without changing the API and checking that all uses of the API are safe with the new limitation
Okay I understand that.
I thought that if someone wanted to re-use the function in some other encoder which might allow 64 order (for example), I should document where the limitations are.
I can change the patch to state that it supports up to 32 but should I also still mention where that is assumed?
Its ok to mention but the difference between API = interface specification and interface implementation should be clear
What about Christophe's suggestion of changing the function definition by using "int coeffs[32]"?
thats a good idea too [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact
State that the maximum value of order is 32. This limit is used in both C and x86 assebly code. --- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..e8487c8 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function supports a maximum order of 32. + * + * This limit is used: + * - when CONFIG_SMALL is 0 to unroll a loop in the C template. + * - when SSE4 (or newer) is available on x86 to unroll a copy loop. + */ void (*lpc_encode)(int32_t *res, const int32_t *smp, int len, int order, const int32_t *coefs, int shift); } FLACDSPContext; -- 1.7.9
This should help to clarify the API. --- libavcodec/flacdsp.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index e8487c8..23d1ae4 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -35,7 +35,7 @@ typedef struct FLACDSPContext { * - when SSE4 (or newer) is available on x86 to unroll a copy loop. */ void (*lpc_encode)(int32_t *res, const int32_t *smp, int len, int order, - const int32_t *coefs, int shift); + const int32_t coefs[32], int shift); } FLACDSPContext; void ff_flacdsp_init(FLACDSPContext *c, enum AVSampleFormat fmt, int bps); -- 1.7.9
Hi, 2014-08-14 17:14 GMT+02:00 James Darnley <james.darnley@gmail.com>:
This should help to clarify the API.
OK. -- Christophe
On Thu, Aug 14, 2014 at 06:01:49PM +0200, Christophe Gisquet wrote:
Hi,
2014-08-14 17:14 GMT+02:00 James Darnley <james.darnley@gmail.com>:
This should help to clarify the API.
OK.
applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB DNS cache poisoning attacks, popular search engine, Google internet authority dont be evil, please
Hi, 2014-08-14 17:14 GMT+02:00 James Darnley <james.darnley@gmail.com>:
+ * This limit is used: + * - when CONFIG_SMALL is 0 to unroll a loop in the C template. + * - when SSE4 (or newer) is available on x86 to unroll a copy loop.
Does the implementer of a new dsp version really has a need for this info? Knowing that order can be anything from 1 to 32 is sufficient. I'd move this part to the commit message. On the other hand, he probably needs to know that the coefficients after the last one are 0s (ie he doesn't need to do it himself on his simd vectors). -- Christophe
Hi, 2014-08-12 23:22 GMT+02:00 James Darnley <james.darnley@gmail.com>:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */ void (*lpc_encode)(int32_t *res, const int32_t *smp, int len, int order, const int32_t *coefs, int shift); } FLACDSPContext;
Some comments: - The decoder passes int coeffs[32]; I don't see how order > 32 would then work - Why not do the same for lpc_encode, and document that maximum order value is 32 - The decoder actually reads the order (see decode_subframe and flacdec.c:366) on 6 bits (there's a type >= 32 check though but I don't see what I'm missing) - The encoder has min and max bounds in (min,max)_prediction_order in arrays (see flacenc.c:312) that are below 32 so the encoder should never use an order above 32 (as seen with again the coding on 6bits on 1126) I guess some of your comments in the asm and here could be dropped then ? -- Christophe
On 2014-08-13 09:21, Christophe Gisquet wrote:
Hi,
2014-08-12 23:22 GMT+02:00 James Darnley <james.darnley@gmail.com>:
--- libavcodec/flacdsp.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/libavcodec/flacdsp.h b/libavcodec/flacdsp.h index 272cf2a..36cd904 100644 --- a/libavcodec/flacdsp.h +++ b/libavcodec/flacdsp.h @@ -27,6 +27,13 @@ typedef struct FLACDSPContext { int len, int shift); void (*lpc)(int32_t *samples, const int coeffs[32], int order, int qlevel, int len); + /** + * This function has some limitations with various configurations: + * - when CONFIG_SMALL is 0 there is an unrolled loop which assumes the + * maximum value of order is 32. + * - when SSE4 (or newer) is available on x86 there is an unrolled copy + * which also assumes the maximum value of order is 0. + */ void (*lpc_encode)(int32_t *res, const int32_t *smp, int len, int order, const int32_t *coefs, int shift); } FLACDSPContext;
Some comments: - The decoder passes int coeffs[32]; I don't see how order > 32 would then work - Why not do the same for lpc_encode, and document that maximum order value is 32 - The decoder actually reads the order (see decode_subframe and flacdec.c:366) on 6 bits (there's a type >= 32 check though but I don't see what I'm missing) - The encoder has min and max bounds in (min,max)_prediction_order in arrays (see flacenc.c:312) that are below 32 so the encoder should never use an order above 32 (as seen with again the coding on 6bits on 1126)
I guess some of your comments in the asm and here could be dropped then ?
You are correct on these points. Please read my reply to Michael to see what I was thinking.
On Tue, Aug 12, 2014 at 11:22:02PM +0200, James Darnley wrote:
From 1.8 to 2.4 times faster. Runtime is reduced by 2 to 39%. The speed-up generally increases with compression_level.
This lpc encoder is not used with levels < 3 so it provides no speed-up in these cases. --- LICENSE.md | 1 + libavcodec/flacenc.c | 2 +- libavcodec/x86/Makefile | 3 + libavcodec/x86/flac_dsp_gpl.asm | 85 +++++++++++++++++++++++++++++++++++++++ libavcodec/x86/flacdsp_init.c | 4 ++ 5 files changed, 94 insertions(+), 1 deletions(-) create mode 100644 libavcodec/x86/flac_dsp_gpl.asm
applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When you are offended at any man's fault, turn to yourself and study your own failings. Then you will forget your anger. -- Epictetus
participants (3)
-
Christophe Gisquet -
James Darnley -
Michael Niedermayer