[PATCH 0/5] Scalar product, take 2
Following the previous comments, I have opted for padding of the buffers in order to use the DSP functions. The troublesome cases for wma lossless are still untested. Patches 3 and 4 still try to address them, 3 being more secure than 4 (which is the shape of the best solution in my opinion). TAK decoder was modified in the same fashion. Christophe Gisquet (5): ra144: use scalarproduct_int16 wma lossless: reuse scalarproduct_and_madd_int16 wma lossless: switching back to C dsp function wma lossless: pad coeff buffer with 0 takdec: pad filter coeff buffer for DSP functions libavcodec/dsputil.c | 4 +-- libavcodec/dsputil.h | 3 +++ libavcodec/ra144.c | 14 ++++------ libavcodec/ra144.h | 6 ++++- libavcodec/ra144dec.c | 3 +++ libavcodec/ra144enc.c | 6 +++-- libavcodec/takdec.c | 33 +++++++----------------- libavcodec/wmalosslessdec.c | 63 ++++++++++++++++++++++++--------------------- 8 files changed, 65 insertions(+), 67 deletions(-) -- 1.8.0.msysgit.0
The buffer holding the coefficients must be padded with 0 so as to use DSP functions that may overread. Currently, the SSE2/3 versions is an example, as they process batches of 16 bytes. --- libavcodec/ra144.c | 14 +++++--------- libavcodec/ra144.h | 6 +++++- libavcodec/ra144dec.c | 3 +++ libavcodec/ra144enc.c | 6 ++++-- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/libavcodec/ra144.c b/libavcodec/ra144.c index fe9a5bc..9929721 100644 --- a/libavcodec/ra144.c +++ b/libavcodec/ra144.c @@ -1681,12 +1681,9 @@ unsigned int ff_rescale_rms(unsigned int rms, unsigned int energy) } /** inverse root mean square */ -int ff_irms(const int16_t *data) +int ff_irms(DSPContext *dsp, const int16_t *data) { - unsigned int i, sum = 0; - - for (i=0; i < BLOCKSIZE; i++) - sum += data[i] * data[i]; + unsigned int sum = dsp->scalarproduct_int16(data, data, BLOCKSIZE); if (sum == 0) return 0; /* OOPS - division by zero */ @@ -1698,14 +1695,13 @@ void ff_subblock_synthesis(RA144Context *ractx, const int16_t *lpc_coefs, int cba_idx, int cb1_idx, int cb2_idx, int gval, int gain) { - int16_t buffer_a[BLOCKSIZE]; int16_t *block; int m[3]; if (cba_idx) { cba_idx += BLOCKSIZE/2 - 1; - ff_copy_and_dup(buffer_a, ractx->adapt_cb, cba_idx); - m[0] = (ff_irms(buffer_a) * gval) >> 12; + ff_copy_and_dup(ractx->buffer_a, ractx->adapt_cb, cba_idx); + m[0] = (ff_irms(&ractx->dsp, ractx->buffer_a) * gval) >> 12; } else { m[0] = 0; } @@ -1716,7 +1712,7 @@ void ff_subblock_synthesis(RA144Context *ractx, const int16_t *lpc_coefs, block = ractx->adapt_cb + BUFFERSIZE - BLOCKSIZE; - add_wav(block, gain, cba_idx, m, cba_idx? buffer_a: NULL, + add_wav(block, gain, cba_idx, m, cba_idx? ractx->buffer_a: NULL, ff_cb1_vects[cb1_idx], ff_cb2_vects[cb2_idx]); memcpy(ractx->curr_sblock, ractx->curr_sblock + BLOCKSIZE, diff --git a/libavcodec/ra144.h b/libavcodec/ra144.h index 426fea3..2dd7ec2 100644 --- a/libavcodec/ra144.h +++ b/libavcodec/ra144.h @@ -25,6 +25,7 @@ #include <stdint.h> #include "lpc.h" #include "audio_frame_queue.h" +#include "dsputil.h" #define NBLOCKS 4 ///< number of subblocks within a block #define BLOCKSIZE 40 ///< subblock size in 16-bit words @@ -35,6 +36,7 @@ typedef struct RA144Context { AVCodecContext *avctx; + DSPContext dsp; LPCContext lpc_ctx; AudioFrameQueue afq; int last_frame; @@ -57,6 +59,8 @@ typedef struct RA144Context { /** Adaptive codebook, its size is two units bigger to avoid a * buffer overflow. */ int16_t adapt_cb[146+2]; + + DECLARE_ALIGNED(16, int16_t, buffer_a)[FFALIGN(BLOCKSIZE,16)]; } RA144Context; void ff_copy_and_dup(int16_t *target, const int16_t *source, int offset); @@ -68,7 +72,7 @@ unsigned int ff_rms(const int *data); int ff_interp(RA144Context *ractx, int16_t *out, int a, int copyold, int energy); unsigned int ff_rescale_rms(unsigned int rms, unsigned int energy); -int ff_irms(const int16_t *data); +int ff_irms(DSPContext *dsp, const int16_t *data/*align 16*/); void ff_subblock_synthesis(RA144Context *ractx, const int16_t *lpc_coefs, int cba_idx, int cb1_idx, int cb2_idx, int gval, int gain); diff --git a/libavcodec/ra144dec.c b/libavcodec/ra144dec.c index 63f77e0..748eba1 100644 --- a/libavcodec/ra144dec.c +++ b/libavcodec/ra144dec.c @@ -34,9 +34,12 @@ static av_cold int ra144_decode_init(AVCodecContext * avctx) RA144Context *ractx = avctx->priv_data; ractx->avctx = avctx; + ff_dsputil_init(&ractx->dsp, avctx); ractx->lpc_coef[0] = ractx->lpc_tables[0]; ractx->lpc_coef[1] = ractx->lpc_tables[1]; + + AV_ZERO128(ractx->buffer_a+BLOCKSIZE); avctx->channels = 1; avctx->channel_layout = AV_CH_LAYOUT_MONO; diff --git a/libavcodec/ra144enc.c b/libavcodec/ra144enc.c index 2eac343..cbbde02 100644 --- a/libavcodec/ra144enc.c +++ b/libavcodec/ra144enc.c @@ -60,7 +60,9 @@ static av_cold int ra144_encode_init(AVCodecContext * avctx) ractx = avctx->priv_data; ractx->lpc_coef[0] = ractx->lpc_tables[0]; ractx->lpc_coef[1] = ractx->lpc_tables[1]; + AV_ZERO128(ractx->buffer_a+BLOCKSIZE); ractx->avctx = avctx; + ff_dsputil_init(&ractx->dsp, avctx); ret = ff_lpc_init(&ractx->lpc_ctx, avctx->frame_size, LPC_ORDER, FF_LPC_TYPE_LEVINSON); if (ret < 0) @@ -334,7 +336,7 @@ static void ra144_encode_subblock(RA144Context *ractx, float data[BLOCKSIZE] = { 0 }, work[LPC_ORDER + BLOCKSIZE]; float coefs[LPC_ORDER]; float zero[BLOCKSIZE], cba[BLOCKSIZE], cb1[BLOCKSIZE], cb2[BLOCKSIZE]; - int16_t cba_vect[BLOCKSIZE]; + LOCAL_ALIGNED(16, int16_t, cba_vect, [BLOCKSIZE]); int cba_idx, cb1_idx, cb2_idx, gain; int i, n; unsigned m[3]; @@ -374,7 +376,7 @@ static void ra144_encode_subblock(RA144Context *ractx, memcpy(cba, work + LPC_ORDER, sizeof(cba)); ff_copy_and_dup(cba_vect, ractx->adapt_cb, cba_idx + BLOCKSIZE / 2 - 1); - m[0] = (ff_irms(cba_vect) * rms) >> 12; + m[0] = (ff_irms(&ractx->dsp, cba_vect) * rms) >> 12; } fixed_cb_search(work + LPC_ORDER, coefs, data, cba_idx, &cb1_idx, &cb2_idx); for (i = 0; i < BLOCKSIZE; i++) { -- 1.8.0.msysgit.0
2013/5/10 Christophe Gisquet <christophe.gisquet@gmail.com>:
@@ -374,7 +376,7 @@ static void ra144_encode_subblock(RA144Context *ractx, memcpy(cba, work + LPC_ORDER, sizeof(cba));
ff_copy_and_dup(cba_vect, ractx->adapt_cb, cba_idx + BLOCKSIZE / 2 - 1); - m[0] = (ff_irms(cba_vect) * rms) >> 12; + m[0] = (ff_irms(&ractx->dsp, cba_vect) * rms) >> 12; } fixed_cb_search(work + LPC_ORDER, coefs, data, cba_idx, &cb1_idx, &cb2_idx); for (i = 0; i < BLOCKSIZE; i++) {
I think this hunk is wrong. What's the name of the fate test for encoding ra144? I've run fate-ra-144 fate-lossless-wma fate-lossless-tak so far. -- Christophe
On Fri, May 10, 2013 at 11:39:04AM +0200, Christophe Gisquet wrote:
2013/5/10 Christophe Gisquet <christophe.gisquet@gmail.com>:
@@ -374,7 +376,7 @@ static void ra144_encode_subblock(RA144Context *ractx, memcpy(cba, work + LPC_ORDER, sizeof(cba));
ff_copy_and_dup(cba_vect, ractx->adapt_cb, cba_idx + BLOCKSIZE / 2 - 1); - m[0] = (ff_irms(cba_vect) * rms) >> 12; + m[0] = (ff_irms(&ractx->dsp, cba_vect) * rms) >> 12; } fixed_cb_search(work + LPC_ORDER, coefs, data, cba_idx, &cb1_idx, &cb2_idx); for (i = 0; i < BLOCKSIZE; i++) {
I think this hunk is wrong. What's the name of the fate test for encoding ra144?
fate-acodec-ra144 should do that also you can find the names of tests by greping the output of make fate-list [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
On Fri, May 10, 2013 at 09:00:11PM +0200, Michael Niedermayer wrote:
On Fri, May 10, 2013 at 11:39:04AM +0200, Christophe Gisquet wrote:
2013/5/10 Christophe Gisquet <christophe.gisquet@gmail.com>:
@@ -374,7 +376,7 @@ static void ra144_encode_subblock(RA144Context *ractx, memcpy(cba, work + LPC_ORDER, sizeof(cba));
ff_copy_and_dup(cba_vect, ractx->adapt_cb, cba_idx + BLOCKSIZE / 2 - 1); - m[0] = (ff_irms(cba_vect) * rms) >> 12; + m[0] = (ff_irms(&ractx->dsp, cba_vect) * rms) >> 12; } fixed_cb_search(work + LPC_ORDER, coefs, data, cba_idx, &cb1_idx, &cb2_idx); for (i = 0; i < BLOCKSIZE; i++) {
I think this hunk is wrong. What's the name of the fate test for encoding ra144?
fate-acodec-ra144 should do that
also you can find the names of tests by greping the output of make fate-list
and it seems you are correct, it breaks the test: TEST acodec-ra144 stddev: 6452.80 PSNR: 20.13 MAXDIFF:53031 bytes: 96000/ 96000 Test acodec-ra144 failed. Look at tests/data/fate/acodec-ra144.err for details. make: *** [fate-acodec-ra144] Error 1 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
Hi, this is an old thread, but here's a fixed patch that passes fate-ra-144 fate-acodec-ra144 for win32 and win64 for CPUFLAGS in sse/sse2/sse4.2 -- Christophe
On Sun, Feb 09, 2014 at 10:31:08PM +0100, Christophe Gisquet wrote:
Hi,
this is an old thread, but here's a fixed patch that passes fate-ra-144 fate-acodec-ra144 for win32 and win64 for CPUFLAGS in sse/sse2/sse4.2
-- Christophe
ra144.c | 14 +++++--------- ra144.h | 6 +++++- ra144dec.c | 3 +++ ra144enc.c | 7 ++++--- 4 files changed, 17 insertions(+), 13 deletions(-) c0451943ba2c49a140501e0f5339b670ad362af1 0001-ra144-use-scalarproduct_int16.patch From 2501ca3ce45767351abc2b721b60fd833be52734 Mon Sep 17 00:00:00 2001 From: Christophe Gisquet <christophe.gisquet@gmail.com> Date: Sun, 4 Mar 2012 13:28:16 +0100 Subject: [PATCH 1/2] ra144: use scalarproduct_int16
The buffer holding the coefficients must be padded with 0 so as to use DSP functions that may overread. Currently, the SSE2/3 versions is an example, as they process batches of 16 bytes.
applied and ZERO128 removed, the context should be zeroed on allocation thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
Approximate relative speedup depending on instruction set: plain C: -6% mmxext: 51% sse2: 54% --- libavcodec/wmalosslessdec.c | 48 +++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c index 90a0109..e566943 100644 --- a/libavcodec/wmalosslessdec.c +++ b/libavcodec/wmalosslessdec.c @@ -29,6 +29,7 @@ #include "internal.h" #include "get_bits.h" #include "put_bits.h" +#include "dsputil.h" #include "wma.h" #include "wma_common.h" @@ -66,6 +67,7 @@ typedef struct { typedef struct WmallDecodeCtx { /* generic decoder variables */ AVCodecContext *avctx; + DSPContext dsp; ///< accelerated DSP functions AVFrame frame; uint8_t frame_data[MAX_FRAMESIZE + FF_INPUT_BUFFER_PADDING_SIZE]; ///< compressed frame data PutBitContext pb; ///< context for filling the frame_data buffer @@ -141,9 +143,9 @@ typedef struct WmallDecodeCtx { int scaling; int coefsend; int bitsend; - int16_t coefs[MAX_ORDER]; - int16_t lms_prevvalues[MAX_ORDER * 2]; - int16_t lms_updates[MAX_ORDER * 2]; + DECLARE_ALIGNED(16, int16_t, coefs)[MAX_ORDER]; + DECLARE_ALIGNED(16, int16_t, lms_prevvalues)[MAX_ORDER * 2]; + DECLARE_ALIGNED(16, int16_t, lms_updates)[MAX_ORDER * 2]; int recent; } cdlms[WMALL_MAX_CHANNELS][9]; @@ -179,6 +181,7 @@ static av_cold int decode_init(AVCodecContext *avctx) int i, log2_max_num_subframes; s->avctx = avctx; + ff_dsputil_init(&s->dsp, avctx); init_put_bits(&s->pb, s->frame_data, MAX_FRAMESIZE); if (avctx->extradata_size >= 18) { @@ -684,35 +687,11 @@ static void revert_mclms(WmallDecodeCtx *s, int tile_size) } } -static int lms_predict(WmallDecodeCtx *s, int ich, int ilms) +static void lms_update(WmallDecodeCtx *s, int ich, int ilms, int input) { - int pred = 0, icoef; - int recent = s->cdlms[ich][ilms].recent; - - for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++) - pred += s->cdlms[ich][ilms].coefs[icoef] * - s->cdlms[ich][ilms].lms_prevvalues[icoef + recent]; - - return pred; -} - -static void lms_update(WmallDecodeCtx *s, int ich, int ilms, - int input, int residue) -{ - int icoef; int recent = s->cdlms[ich][ilms].recent; int range = 1 << s->bits_per_sample - 1; - if (residue < 0) { - for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++) - s->cdlms[ich][ilms].coefs[icoef] -= - s->cdlms[ich][ilms].lms_updates[icoef + recent]; - } else if (residue > 0) { - for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++) - s->cdlms[ich][ilms].coefs[icoef] += - s->cdlms[ich][ilms].lms_updates[icoef + recent]; - } - if (recent) recent--; else { @@ -773,6 +752,9 @@ static void use_normal_update_speed(WmallDecodeCtx *s, int ich) s->update_speed[ich] = 8; } +/** Get sign of integer (1 for positive, -1 for negative and 0 for zero) */ +#define WMASIGN(x) ((x > 0) - (x < 0)) + static void revert_cdlms(WmallDecodeCtx *s, int ch, int coef_begin, int coef_end) { @@ -783,9 +765,15 @@ static void revert_cdlms(WmallDecodeCtx *s, int ch, for (icoef = coef_begin; icoef < coef_end; icoef++) { pred = 1 << (s->cdlms[ch][ilms].scaling - 1); residue = s->channel_residues[ch][icoef]; - pred += lms_predict(s, ch, ilms); + pred += s->dsp.scalarproduct_and_madd_int16(s->cdlms[ch][ilms].coefs, + s->cdlms[ch][ilms].lms_prevvalues + + s->cdlms[ch][ilms].recent, + s->cdlms[ch][ilms].lms_updates + + s->cdlms[ch][ilms].recent, + s->cdlms[ch][ilms].order, + WMASIGN(residue)); input = residue + (pred >> s->cdlms[ch][ilms].scaling); - lms_update(s, ch, ilms, input, residue); + lms_update(s, ch, ilms, input); s->channel_residues[ch][icoef] = input; } } -- 1.8.0.msysgit.0
The DSP function only warrants to process batches of 16 bytes, but this codec may only guarantees multiples of 8. However, no such sample is known to date, so rather than not use the optimized DSP function, whenever such a troublesome case is encountered, the DSP function pointer is set back to the C one. --- libavcodec/dsputil.c | 4 ++-- libavcodec/dsputil.h | 3 +++ libavcodec/wmalosslessdec.c | 8 ++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/libavcodec/dsputil.c b/libavcodec/dsputil.c index b434589..a6a54a8 100644 --- a/libavcodec/dsputil.c +++ b/libavcodec/dsputil.c @@ -2562,7 +2562,7 @@ static int32_t scalarproduct_int16_c(const int16_t * v1, const int16_t * v2, int return res; } -static int32_t scalarproduct_and_madd_int16_c(int16_t *v1, const int16_t *v2, const int16_t *v3, int order, int mul) +int32_t ff_scalarproduct_and_madd_int16_c(int16_t *v1, const int16_t *v2, const int16_t *v3, int order, int mul) { int res = 0; while (order--) { @@ -2877,7 +2877,7 @@ av_cold void ff_dsputil_init(DSPContext* c, AVCodecContext *avctx) c->vector_clipf = vector_clipf_c; c->scalarproduct_int16 = scalarproduct_int16_c; - c->scalarproduct_and_madd_int16 = scalarproduct_and_madd_int16_c; + c->scalarproduct_and_madd_int16 = ff_scalarproduct_and_madd_int16_c; c->apply_window_int16 = apply_window_int16_c; c->vector_clip_int32 = vector_clip_int32_c; diff --git a/libavcodec/dsputil.h b/libavcodec/dsputil.h index 07a95af..faf84e2 100644 --- a/libavcodec/dsputil.h +++ b/libavcodec/dsputil.h @@ -320,6 +320,9 @@ attribute_deprecated void dsputil_init(DSPContext* c, AVCodecContext *avctx); int ff_check_alignment(void); +int32_t ff_scalarproduct_and_madd_int16_c(int16_t *v1, const int16_t *v2, + const int16_t *v3, int order, int mul); + void ff_set_cmp(DSPContext* c, me_cmp_func *cmp, int type); void ff_dsputil_init_alpha(DSPContext* c, AVCodecContext *avctx); diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c index e566943..ae366ef 100644 --- a/libavcodec/wmalosslessdec.c +++ b/libavcodec/wmalosslessdec.c @@ -453,6 +453,14 @@ static int decode_cdlms(WmallDecodeCtx *s) s->cdlms[0][0].order = 0; return AVERROR_INVALIDDATA; } + if(s->cdlms[c][i].order & 8) { + static int warned; + s->dsp.scalarproduct_and_madd_int16 = ff_scalarproduct_and_madd_int16_c; + if(!warned) + av_log_ask_for_sample(s->avctx, "CSLMS of order %d", + s->cdlms[c][i].order); + warned = 1; + } } for (i = 0; i < s->cdlms_ttl[c]; i++) -- 1.8.0.msysgit.0
This allows using unmodified SIMD functions requiring batches of 16 elements. --- libavcodec/wmalosslessdec.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c index ae366ef..f95a0f7 100644 --- a/libavcodec/wmalosslessdec.c +++ b/libavcodec/wmalosslessdec.c @@ -45,6 +45,7 @@ #define WMALL_BLOCK_MAX_SIZE (1 << WMALL_BLOCK_MAX_BITS) ///< maximum block size #define WMALL_BLOCK_SIZES (WMALL_BLOCK_MAX_BITS - WMALL_BLOCK_MIN_BITS + 1) ///< possible block sizes +#define WMALL_COEFF_PAD_SIZE 16 ///< pad coef buffers with 0 for use with SIMD functions /** * @brief frame-specific decoder context for a single channel @@ -486,6 +487,12 @@ static int decode_cdlms(WmallDecodeCtx *s) (get_bits(&s->gb, s->cdlms[c][i].bitsend) << shift_l) >> shift_r; } } + + for (i = 0; i < s->cdlms_ttl[c]; i++) { + memset(s->cdlms[c][i].coefs + s->cdlms[c][i].order, 0, + (s->cdlms[c][i].order % WMALL_COEFF_PAD_SIZE) + * sizeof(s->cdlms[c][i].coefs)); + } } return 0; -- 1.8.0.msysgit.0
On Fri, May 10, 2013 at 09:32:57AM +0000, Christophe Gisquet wrote:
This allows using unmodified SIMD functions requiring batches of 16 elements. --- libavcodec/wmalosslessdec.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c index ae366ef..f95a0f7 100644 --- a/libavcodec/wmalosslessdec.c +++ b/libavcodec/wmalosslessdec.c @@ -45,6 +45,7 @@ #define WMALL_BLOCK_MAX_SIZE (1 << WMALL_BLOCK_MAX_BITS) ///< maximum block size #define WMALL_BLOCK_SIZES (WMALL_BLOCK_MAX_BITS - WMALL_BLOCK_MIN_BITS + 1) ///< possible block sizes
+#define WMALL_COEFF_PAD_SIZE 16 ///< pad coef buffers with 0 for use with SIMD functions
/** * @brief frame-specific decoder context for a single channel @@ -486,6 +487,12 @@ static int decode_cdlms(WmallDecodeCtx *s) (get_bits(&s->gb, s->cdlms[c][i].bitsend) << shift_l) >> shift_r; } } + + for (i = 0; i < s->cdlms_ttl[c]; i++) { + memset(s->cdlms[c][i].coefs + s->cdlms[c][i].order, 0, + (s->cdlms[c][i].order % WMALL_COEFF_PAD_SIZE) + * sizeof(s->cdlms[c][i].coefs));
% 16 should be replaced by & 15, otherwise performance would depend on the compiler making that optimization and a av_assert could be added to ensure (order & 7) == 0 I also suggest to keep av_log_ask_for_sample() for the order & 8 case [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle
Hi, 2013-05-10 21:52 GMT+02:00 Michael Niedermayer <michaelni@gmx.at>:
% 16 should be replaced by & 15, otherwise performance would depend on the compiler making that optimization
Replace by & (WMALL_COEFF_PAD_SIZE-1) which is the same but clearer.
and a av_assert could be added to ensure (order & 7) == 0
This is not possible because of this line: s->cdlms[c][i].order = (get_bits(&s->gb, 7) + 1) * 8; so no assert
I also suggest to keep av_log_ask_for_sample() for the order & 8
Use avpriv_request_sample as it seems to be the non-deprecated way of doing this. My usage of it may be incorrect. The changes passes fate-lossless-wma for win32 and win64 and CPUFLAGS in sse2/sse4.2. Christophe
2014-02-09 22:38 GMT+01:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Replace by & (WMALL_COEFF_PAD_SIZE-1) which is the same but clearer.
Which is not correct anyway. New patch fixing the issue, but which may be a bit assuming on the possible order values (multiples of 8). -- Christophe
On Mon, Feb 10, 2014 at 09:10:52AM +0100, Christophe Gisquet wrote:
2014-02-09 22:38 GMT+01:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Replace by & (WMALL_COEFF_PAD_SIZE-1) which is the same but clearer.
Which is not correct anyway. New patch fixing the issue, but which may be a bit assuming on the possible order values (multiples of 8).
%16 &15 &8 all are the same for unsigned values which are a multiply of 8 if values could be something else then the code can write out of array and is potentially exploitable for example if one changes MAX_ORDER to 255 from 256 you have exploitable code if someone finds and reverseensgeer an extra bit somewhere that allows files with order % 8 != 0 it would be exploitable too I suggest to make the code a bit more defensive and not write out of array in such cases, even though they are not possible with the current code [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 2 "100% positive feedback" - "All either got their money back or didnt complain" "Best seller ever, very honest" - "Seller refunded buyer after failed scam"
Hi, 2014-02-10 Michael Niedermayer <michaelni@gmx.at>:
%16 &15 &8 all are the same for unsigned values which are a multiply of 8
if values could be something else then the code can write out of array and is potentially exploitable
Errm... Sorry, I wasn't clear here. The actual issue was an incorrect sizeof. And the commit message was wrong (batch of 8 whereas it is 16) and thus confusing on what was happening here.
I suggest to make the code a bit more defensive and not write out of array in such cases, even though they are not possible with the current code
Well, this is the only location where the order is set as far as I see. If you see something elsewhere that should be done, I'm missing it, so please be specific. Anyway, I don't see what I can do safer than setting the remaining of the buffer to 0, which is done in the attached patch. This is a bit exaggerated as, last I checked, the order was 16 or 32, and MAX_ORDER is 256.
From the code, though, zeroing "FFALIGN(s->cdlms[c][i].order,16) - s->cdlms[c][i].order" elements (0 or 8 with the current code) would be sufficient.
-- Christophe
On Mon, Feb 10, 2014 at 09:20:22PM +0100, Christophe Gisquet wrote:
Hi,
2014-02-10 Michael Niedermayer <michaelni@gmx.at>:
%16 &15 &8 all are the same for unsigned values which are a multiply of 8
if values could be something else then the code can write out of array and is potentially exploitable
Errm... Sorry, I wasn't clear here. The actual issue was an incorrect sizeof. And the commit message was wrong (batch of 8 whereas it is 16) and thus confusing on what was happening here.
I suggest to make the code a bit more defensive and not write out of array in such cases, even though they are not possible with the current code
Well, this is the only location where the order is set as far as I see.
yes
If you see something elsewhere that should be done, I'm missing it, so please be specific. Anyway, I don't see what I can do safer than setting the remaining of the buffer to 0, which is done in the attached patch.
this is safe but not the most efficint I should have been more specific, what i suggest is for example to make the buffer that is written into, 8 elements larger then adding 8 element padding should always be safe independant of what order or max order is [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data
Resurrecting an old thread... 2014-02-10 21:43 GMT+01:00 Michael Niedermayer <michaelni@gmx.at>:
this is safe but not the most efficint I should have been more specific, what i suggest is for example to make the buffer that is written into, 8 elements larger then adding 8 element padding should always be safe independant of what order or max order is
I'm attaching 2 patches: 1) split back apedsp into a llauddsp (modelled after the ugly llviddsp) 2) use this new dsp in wma lossless Patch 2 does what you suggest: declare a larger than needed array to simplify the memsetting. While the needed padding size is known, this makes the array kind of ugly. I could have written " + 8" instead (but magic numbers are bad) or "+ WMALL_COEFF_PAD_SIZE" (but this is incorrect). Also I used memset instead of AV_ZERO128 because the later expects an aligned address. This is the case currently. Maybe using 2 AV_ZERO64 then? -- Christophe
2014-05-30 19:51 GMT+02:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Also I used memset instead of AV_ZERO128 because the later expects an aligned address. This is the case currently. Maybe using 2 AV_ZERO64 then?
Except when I don't refresh the patch. -- Christophe
Hi, 2014-05-30 20:06 GMT+02:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Except when I don't refresh the patch.
Could you check the other patch I attached to the parent mail? This is the actual use for wmalossless. -- Christophe
On Fri, Jun 06, 2014 at 08:55:20AM +0200, Christophe Gisquet wrote:
Hi,
2014-05-30 20:06 GMT+02:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Except when I don't refresh the patch.
Could you check the other patch I attached to the parent mail? This is the actual use for wmalossless.
applied thanks [...] -- 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
On Fri, May 30, 2014 at 07:51:29PM +0200, Christophe Gisquet wrote:
Resurrecting an old thread...
2014-02-10 21:43 GMT+01:00 Michael Niedermayer <michaelni@gmx.at>:
this is safe but not the most efficint I should have been more specific, what i suggest is for example to make the buffer that is written into, 8 elements larger then adding 8 element padding should always be safe independant of what order or max order is
I'm attaching 2 patches: 1) split back apedsp into a llauddsp (modelled after the ugly llviddsp) 2) use this new dsp in wma lossless
Patch 2 does what you suggest: declare a larger than needed array to simplify the memsetting.
While the needed padding size is known, this makes the array kind of ugly. I could have written " + 8" instead (but magic numbers are bad) or "+ WMALL_COEFF_PAD_SIZE" (but this is incorrect).
Also I used memset instead of AV_ZERO128 because the later expects an aligned address. This is the case currently. Maybe using 2 AV_ZERO64 then?
-- Christophe [...]
@@ -16,6 +16,7 @@ OBJS-$(CONFIG_H264PRED) += arm/h264pred_init_arm.o OBJS-$(CONFIG_H264QPEL) += arm/h264qpel_init_arm.o OBJS-$(CONFIG_HPELDSP) += arm/hpeldsp_init_arm.o \ arm/hpeldsp_arm.o
+OBJS-$(CONFIG_LLAUDDSP) += arm/lossless_audidsp_init_arm.o
audIdsp ? [...]
--- a/libavcodec/ppc/apedsp_altivec.c +++ b/libavcodec/ppc/lossless_audiodsp_altivec.c @@ -25,7 +25,7 @@
#include "libavutil/attributes.h" #include "libavutil/ppc/types_altivec.h" -#include "libavcodec/apedsp.h" +#include "libavcodec/llauddsp.h"
this file doesnt exist [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus
Hi, 2014-05-31 4:25 GMT+02:00 Michael Niedermayer <michaelni@gmx.at>:
+OBJS-$(CONFIG_LLAUDDSP) += arm/lossless_audidsp_init_arm.o
audIdsp ?
[...]
--- a/libavcodec/ppc/apedsp_altivec.c +++ b/libavcodec/ppc/lossless_audiodsp_altivec.c @@ -25,7 +25,7 @@
#include "libavutil/attributes.h" #include "libavutil/ppc/types_altivec.h" -#include "libavcodec/apedsp.h" +#include "libavcodec/llauddsp.h"
this file doesnt exist
Patch updated. -- Christophe
2014-05-31 10:16 GMT+02:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Patch updated.
Ping? -- Christophe
On Thu, Jun 05, 2014 at 07:42:24PM +0200, Christophe Gisquet wrote:
2014-05-31 10:16 GMT+02:00 Christophe Gisquet <christophe.gisquet@gmail.com>:
Patch updated.
Ping?
applied thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates
Currently, a part of the loop has to be handled in plain C, because of DSP function requirements of having batches of 16 elements to process. Instead, pad the buffer with 0 where the DSP functions are overreading. --- libavcodec/takdec.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/libavcodec/takdec.c b/libavcodec/takdec.c index f36cec3..8004d7b 100644 --- a/libavcodec/takdec.c +++ b/libavcodec/takdec.c @@ -372,7 +372,7 @@ static int decode_subframe(TAKDecContext *s, int32_t *decoded, { GetBitContext *gb = &s->gb; int tmp, x, y, i, j, ret = 0; - int dshift, size, filter_quant, filter_order; + int dshift, size, filter_quant, filter_order, filter_order16; int tfilter[MAX_PREDICTORS]; if (!get_bits1(gb)) @@ -445,6 +445,10 @@ static int decode_subframe(TAKDecContext *s, int32_t *decoded, tfilter[i] = s->predictors[i] << 6; } + filter_order16 = FFALIGN(filter_order, 16); + AV_ZERO128(s->filter + filter_order16 - 16); + AV_ZERO128(s->filter + filter_order16 - 8); + x = 1 << (32 - (15 - filter_quant)); y = 1 << ((15 - filter_quant) - 1); for (i = 0, j = filter_order - 1; i < filter_order / 2; i++, j--) { @@ -468,15 +472,8 @@ static int decode_subframe(TAKDecContext *s, int32_t *decoded, for (i = 0; i < tmp; i++) { int v = 1 << (filter_quant - 1); - if (filter_order & -16) - v += s->dsp.scalarproduct_int16(&s->residues[i], s->filter, - filter_order & -16); - for (j = filter_order & -16; j < filter_order; j += 4) { - v += s->residues[i + j + 3] * s->filter[j + 3] + - s->residues[i + j + 2] * s->filter[j + 2] + - s->residues[i + j + 1] * s->filter[j + 1] + - s->residues[i + j ] * s->filter[j ]; - } + v += s->dsp.scalarproduct_int16(&s->residues[i], s->filter, + filter_order16); v = (av_clip(v >> filter_quant, -8192, 8191) << dshift) - *decoded; *decoded++ = v; s->residues[filter_order + i] = v >> dshift; @@ -597,6 +594,7 @@ static int decorrelate(TAKDecContext *s, int c1, int c2, int length) dval1 = get_bits1(gb); dval2 = get_bits1(gb); + AV_ZERO128(s->filter + 8); for (i = 0; i < filter_order; i++) { if (!(i & 3)) code_size = 14 - get_bits(gb, 3); @@ -639,19 +637,8 @@ static int decorrelate(TAKDecContext *s, int c1, int c2, int length) for (i = 0; i < tmp; i++) { int v = 1 << 9; - if (filter_order == 16) { - v += s->dsp.scalarproduct_int16(&s->residues[i], s->filter, - filter_order); - } else { - v += s->residues[i + 7] * s->filter[7] + - s->residues[i + 6] * s->filter[6] + - s->residues[i + 5] * s->filter[5] + - s->residues[i + 4] * s->filter[4] + - s->residues[i + 3] * s->filter[3] + - s->residues[i + 2] * s->filter[2] + - s->residues[i + 1] * s->filter[1] + - s->residues[i ] * s->filter[0]; - } + v += s->dsp.scalarproduct_int16(&s->residues[i], + s->filter, 16); v = (av_clip(v >> 10, -8192, 8191) << dshift) - *p1; *p1++ = v; -- 1.8.0.msysgit.0
On 5/10/13, Christophe Gisquet <christophe.gisquet@gmail.com> wrote:
Currently, a part of the loop has to be handled in plain C, because of DSP function requirements of having batches of 16 elements to process.
Instead, pad the buffer with 0 where the DSP functions are overreading. --- libavcodec/takdec.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)
Have you compared performance before and after?
Hi, 2013/5/10 Paul B Mahol <onemda@gmail.com>:
Have you compared performance before and after?
I didn't, thinking it was an obvious win. The second loop is almost without effect anyway: from what I've seen, it seems to run like once every 5s of audio. But the first isn't faster. Even replacing your loop with a duff's device doesn't help. And since this is such a fringe codec, let's drop this patch. -- Christophe
participants (3)
-
Christophe Gisquet -
Michael Niedermayer -
Paul B Mahol