Repository navigation
Conversation
e41d0e0 to
30ecf40
Compare
|
I just landed commit 8207ebb that does something similar with Neon assembly. Not sure what arch you're targeting (if any in particular), but I suspect the asm/intrinsics route may just be the way to go instead. I suspect it shouldn't be too hard to do for SSE. |
The de-emphasis filter is a first-order IIR that runs per channel, each with its own filter memory, so the scalar loop is latency-bound: every output waits for the multiply-add that produced the previous memory value. Expand the recurrence over blocks of four samples instead. The serial chain is one multiply-add per sample, `m = coef0 * (x[j] + m)`. After the expansion only the carry `coef0^4 * m` is left on it, one per four samples. The rest becomes independent work the compiler can pipeline and vectorize. Remaining samples are handled by the existing scalar loop. The transform relies on `a*(x+m)` being interchangeable with `a*x + a*m` which does not hold in fixed point due to saturation so this is float-only. The result is not bit-exact but the pole is contractive: * over 120 s of speech 30 of 5.76M samples differ * over 64 s of stereo music 612 of 6.19M in both cases by one LSB at most. `opus_demo -d` on x86-64 with gcc -O3, split by the mode in the TOC byte: 120 s of speech at 48 kHz mono: | mode | deemphasis | total | | ------------- | ------------ | ----- | | SILK-only, WB | n/a, the CELT decoder does not run in this mode | | | hybrid, FB | 8.9% -> 2.9% | -5.8% | | CELT-only, FB | 8.2% -> 3.2% | -4.9% | 120 s of speech at 48 kHz stereo: | mode | deemphasis | total | | ------------- | ------------- | ----- | | hybrid, FB | 12.1% -> 3.8% | -7.4% | 64 s of music at 48 kHz stereo: | mode | deemphasis | total | | ------------- | ------------ | ----- | | CELT-only, FB | 5.4% -> 3.7% | -1.6% | Bit-identical in fixed-point (obviously)
30ecf40 to
4e48728
Compare
There was a problem hiding this comment.
Rebased, results still stand and I had Claude give the SSE a try because why not.
Hybrid stereo mode (which happens in the wild world of WebRTC I am aiming at) doesn't change at all vs this change alone. CELT-only gets an additional -0.5% (mono) to 0.9% (stereo).
For CELT-only stereo that is half of what this yeilds (-1.8%) so might be worth it for a 30 line function.
| int k; | ||
| m = deemphasis_block(t, x+j, m, coef0); | ||
| for (k=0;k<DEEMPH_BLOCK;k++) | ||
| y[(j+k)*C] = ADD_RES(y[(j+k)*C], SIG2RES(t[k])); |
There was a problem hiding this comment.
specializing C for 1 or 2 would let the compiler unroll this but yielded <1% at the expense of duplicate code so I didn't do it.
| VARDECL(celt_sig, scratch); | ||
| SAVE_STACK; | ||
| #if !defined(CUSTOM_MODES) && !defined(ENABLE_OPUS_CUSTOM_API) && !defined(ENABLE_QEXT) | ||
| /* Short version for common case. */ |
There was a problem hiding this comment.
drive-by: "common" here is celt-only stereo, notably not hybrid. opus_decoder.c:370 sets celt_accum = (mode != MODE_CELT_ONLY).
Dropping !accum would have been ~6% but becomes vanishingly small with this patch (which the neon assembly might have gone after?)
|
I'd say my main issue here is just how it makes the deemphasis code even harder to follow than the mess it already was. Any way you can make this more self-contained? Also, just curious, how does your code compare to the Neon version that @cyanreg just landed? |
|
Switching to math writing, the old form is basically which in this change becomes The neon code does the same but writes it slightly differently with Updated in the vector carry commit. I like it slightly less but that is just because my brain is already wired for the other way of writing it. Now LLMs are great because they don't complain when asked to do a line-by-line annotation of that: which shows one can align the two even more (in a separate commit; lines above refer to the second one)
The |
The de-emphasis filter is a first-order IIR that runs per channel, each
with its own filter memory, so the scalar loop is latency-bound: every
output waits for the multiply-add that produced the previous memory
value.
Expand the recurrence over blocks of four samples instead. The serial
chain is one multiply-add per sample,
m = coef0 * (x[j] + m). Afterthe expansion only the carry
coef0^4 * mis left on it, one per foursamples. The rest becomes independent work the compiler can pipeline and
vectorize. Remaining samples are handled by the existing scalar loop.
The transform relies on
a*(x+m)being interchangeable witha*x + a*mwhich does not hold in fixed point due to saturation so this is
float-only. The result is not bit-exact but the pole is contractive:
in both cases by one LSB at most.
opus_demo -don x86-64 with gcc -O3, split by the mode in the TOCbyte:
120 s of speech at 48 kHz mono:
64 s of music at 48 kHz stereo:
Bit-identical in fixed-point (obviously)