Audio: DRC: Change DRC to use lookup table based sine function #8491

singalsu · 2023-11-17T16:34:36Z

This change saves in TGL platform about 13 MPCS, from 83 to 70 MCPS. In MTL platform the saving is 12 MCPS, from 46 to 34 MCPS.

The .bss RAM usage increases by 1 kB from selecting CONFIG_MATH_LUT_TRIG_FIXED.

singalsu · 2023-11-17T16:37:18Z

src/math/lut_trig.c

+#define SOFM_LUT_SINE_SIZE (SOFM_LUT_SINE_NQUART + 1)
+
+/* An 1/4 period of sine wave as Q1.31 */
+const int32_t sofm_lut_sine_table[SOFM_LUT_SINE_SIZE] = {


TODO: Should test if this could be int16_t and size 257 for 16 bit sine.

one is check wether can use 16-bit， another thing is whether can shorten the size? do we really need 512?
such a small usage cost 2k bytes table, it is big cost.

Also, could you add comments for how this table calculated? sin(i/2pi)? then we can further figure out whether 512 is must.

to ensure the accuracy，and I guess maybe it is because the latency is 512 point.

I tried 256 size but the quality was worse than with default cordic algorithm based 16 bit sine. So, the table still has 512 elements but uint16_t type was sufficient, so the table is now half of previous.

I added also static.

lyakh · 2023-11-20T08:08:51Z

src/include/sof/audio/drc/drc_math.h

-	int32_t sin_val = sin_fixed_16b(denorm_x);
-
-	return sin_val << 16;
+	return sofm_lut_sin_fixed_32b(denorm_x);


some of the tooling might complain about a missing line between variable definitions and statements - same below

Yep, I didn't notice first. You are right.

btian1

do we have a float version drc source code? I want to first check float version, the fixed version, then optimized version.

btian1 · 2023-11-20T13:15:31Z

src/math/lut_trig.c

+#define SOFM_LUT_SINE_SIZE (SOFM_LUT_SINE_NQUART + 1)
+
+/* An 1/4 period of sine wave as Q1.31 */
+const int32_t sofm_lut_sine_table[SOFM_LUT_SINE_SIZE] = {


one is check wether can use 16-bit， another thing is whether can shorten the size? do we really need 512?
such a small usage cost 2k bytes table, it is big cost.

Also, could you add comments for how this table calculated? sin(i/2pi)? then we can further figure out whether 512 is must.

lgirdwood · 2023-11-23T16:55:11Z

src/math/Kconfig

@@ -9,6 +9,15 @@ config CORDIC_FIXED
 	  Select this to enable sin(), cos(), asin(), acos(),
 	  and cexp() functions as 16 bit and 32 bit versions.

+config LUT_TRIG_FIXED


We need a cleanup at some point where we have MATH_TRIG_ prefix and likewise convention for all maths APIs, macros, Kconfigs etc.

lgirdwood · 2023-11-23T16:55:52Z

src/math/lut_trig.c

+#define SOFM_LUT_SINE_SIZE (SOFM_LUT_SINE_NQUART + 1)
+
+/* An 1/4 period of sine wave as Q1.31 */
+const int32_t sofm_lut_sine_table[SOFM_LUT_SINE_SIZE] = {


lgirdwood · 2023-11-23T16:58:14Z

src/math/Kconfig

+	  Select this to enable sofm_lut_sin_fixed_32b() function. The
+	  calculation is using 1/4 wave lookup and interpolation.
+	  This option consumes 2052 bytes .bss RAM for the lookup
+	  table.


Can we offer advice on when each trig type should be used. i.e. I would expect we export the same public API for all maths, but the internal calculations will depend on which Kconfig is selected by the user at build time.

I added some text to Kconfig about preferring the lookup sine when used in hot code parts.

singalsu · 2024-01-09T17:46:14Z

do we have a float version drc source code? I want to first check float version, the fixed version, then optimized version.

There used to be long ago in git first version that was float C by Sebastiano and Johny but it was replaced by fixed point code when the work proceeded. A float version should be found from ChromeOS sources. The float code and fixed conversion work for this contribution is owned by team Google so we don't review it here.

I've used the scripts in tools/test/audio to evaluate objective steady signal audio parameters for DRC and we've not seen difference in team Intel's optimizations. DRC has complex transient signal characteristics, and we currently don't have other but subjective expert listening test method for that. It means for me to listen an album of music with this processing in DUT and try to spot any issues.

marc-hb · 2024-01-09T18:33:22Z

zephyr/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_MATH_LUT_TRIG_FIXED
+	${SOF_MATH_PATH}/lut_trig.c
+)
+


Please move this next to the other SOF_MATH_PATH (#8620)

Wrong PR? There's no change to CMakeLists.txt in that.

ShriramShastry

I have reviewed the changes and they appear to be in good standing. I hope that LUT's size is good to others.

ShriramShastry · 2024-01-10T02:08:26Z

src/audio/drc/Kconfig

@@ -3,6 +3,7 @@
 config COMP_DRC
 	bool "Dynamic Range Compressor component"
 	select CORDIC_FIXED
+        select MATH_LUT_TRIG_FIXED


Can it be MATH_LUT_SINE_FIXED instead of MATH_LUT_TRIG_FIXED?

Yep, I'll change the config name. There is no need for other functions now.

ShriramShastry · 2024-01-10T02:10:01Z

src/math/Kconfig

@@ -9,6 +9,16 @@ config CORDIC_FIXED
 	  Select this to enable sin(), cos(), asin(), acos(),
 	  and cexp() functions as 16 bit and 32 bit versions.

+config MATH_LUT_TRIG_FIXED


Can it be MATH_LUT_SINE_FIXED instead of MATH_LUT_TRIG_FIXED?

src/math/Kconfig

singalsu · 2024-01-10T10:56:03Z

I have reviewed the changes and they appear to be in good standing. I hope that LUT's size is good to others.

I tried 256 size LUT but the quality dropped a lot. With 512 the quality is a tiny bit better than in 16 bit cordic, so there should be no negative audio quality impact from this.

This patch adds function sofm_lut_sin_fixed_16b(). It was used earlier in SOF with name sin_fixed() but was remove at add of Cordic trigonometric library. This sine function can be used in hot code parts. Due to look-up table usage it consumes more .bss RAM than cordic version. Signed-off-by: Seppo Ingalsuo <[email protected]>

src/math/lut_trig.c

lyakh · 2024-01-10T11:17:24Z

src/math/lut_trig.c

+	/* Q4.28 x Q12.20 -> Q16.48 --> Q16.31*/
+	idx_tmp = ((int64_t)w * SOFM_LUT_SINE_C_Q20) >> 17;
+	idx = (idx_tmp >> 31); /* Shift to Q0 */
+	frac = (int32_t)(idx_tmp - (idx << 31)); /* Get fraction Q1.31*/


that seems to boil down to

idx_tmp - (idx_tmp & 0xffffffff80000000ULL) == idx_tmp & 0x7fffffff

would the compiler optimise that out by itself?

I was thinking that but it looks awkward in arithmetic that is not bit-banging to HW registers etc. The shifts have association to Qx.y format. But if that gives MCPS advantage I can change, and comment what happens, I'll try.

With this modification 69.976 to 69.925 MCPS, not worth it I think, because of bit-and awkwardness here. I think our perf measurement works down to 0.1 MCPS level, below that it's probably noise.

lyakh · 2024-01-10T11:21:24Z

test/cmocka/src/math/trig/lut_sin_16b_fixed.c

+	int theta;
+
+	for (theta = 0; theta < 360; ++theta) {
+		double rad = _M_PI * (theta / 180.0);


hopefully the compiler will calculate _M_PI / 180.0 at compile time, but parentheses might actually prevent it from doing that and make it a (redundant) run-time calculation

It's cmocka test code so we don't care about performance even if there would be emulated floats.

The test function is based on test function for the cordic sine function. The error tolerance is adjusted to just pass. Signed-off-by: Seppo Ingalsuo <[email protected]>

This change saves in TGL platform about 13 MPCS, from 83 to 70 MCPS. In MTL platform the saving is 12 MCPS, from 46 to 34 MCPS. The .bss RAM usage increases by 1 kB from selecting CONFIG_MATH_LUT_SINE_FIXED. Signed-off-by: Seppo Ingalsuo <[email protected]>

lgirdwood · 2024-01-11T12:36:33Z

@singalsu can you check CI. Thanks !

singalsu commented Nov 17, 2023

View reviewed changes

lyakh reviewed Nov 20, 2023

View reviewed changes

btian1 reviewed Nov 20, 2023

View reviewed changes

lgirdwood reviewed Nov 23, 2023

View reviewed changes

lgirdwood added this to the v2.9 milestone Nov 23, 2023

singalsu force-pushed the drc_use_lut_sine branch from 8bc42e4 to 02d7c32 Compare January 9, 2024 17:16

singalsu marked this pull request as ready for review January 9, 2024 17:20

singalsu requested review from marc-hb, a team, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners January 9, 2024 17:20

singalsu force-pushed the drc_use_lut_sine branch from 02d7c32 to 7c8ae5f Compare January 9, 2024 17:33

singalsu requested a review from ShriramShastry January 9, 2024 17:34

singalsu requested review from lyakh, lgirdwood, btian1 and andrula-song January 9, 2024 17:47

marc-hb reviewed Jan 9, 2024

View reviewed changes

ShriramShastry approved these changes Jan 10, 2024

View reviewed changes

btian1 reviewed Jan 10, 2024

View reviewed changes

src/math/Kconfig Show resolved Hide resolved

singalsu force-pushed the drc_use_lut_sine branch from 7c8ae5f to 13c1725 Compare January 10, 2024 11:07

singalsu requested review from btian1 and marc-hb January 10, 2024 11:07

lyakh reviewed Jan 10, 2024

View reviewed changes

lgirdwood approved these changes Jan 10, 2024

View reviewed changes

singalsu added 2 commits January 10, 2024 15:52

Test: Cmocka: Add test case for lookup table sine function

4ed988b

The test function is based on test function for the cordic sine function. The error tolerance is adjusted to just pass. Signed-off-by: Seppo Ingalsuo <[email protected]>

singalsu force-pushed the drc_use_lut_sine branch from 13c1725 to 3d1d453 Compare January 10, 2024 14:03

btian1 approved these changes Jan 11, 2024

View reviewed changes

kv2019i approved these changes Jan 11, 2024

View reviewed changes

lgirdwood merged commit 8d2fb32 into thesofproject:main Jan 11, 2024
42 of 44 checks passed

singalsu deleted the drc_use_lut_sine branch January 17, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio: DRC: Change DRC to use lookup table based sine function #8491

Audio: DRC: Change DRC to use lookup table based sine function #8491

singalsu commented Nov 17, 2023 •

edited

Loading

singalsu Nov 17, 2023

btian1 Nov 20, 2023

andrula-song Nov 21, 2023

lgirdwood Nov 23, 2023

singalsu Jan 9, 2024

lyakh Nov 20, 2023

singalsu Jan 9, 2024

btian1 left a comment

btian1 Nov 20, 2023

lgirdwood Nov 23, 2023

singalsu Jan 9, 2024

lgirdwood Nov 23, 2023

lgirdwood Nov 23, 2023

singalsu Jan 9, 2024

singalsu commented Jan 9, 2024

marc-hb Jan 9, 2024

singalsu Jan 10, 2024

ShriramShastry left a comment

ShriramShastry Jan 10, 2024

singalsu Jan 10, 2024

ShriramShastry Jan 10, 2024

singalsu commented Jan 10, 2024

lyakh Jan 10, 2024

singalsu Jan 10, 2024

singalsu Jan 10, 2024

lyakh Jan 10, 2024

singalsu Jan 10, 2024

lgirdwood commented Jan 11, 2024

Audio: DRC: Change DRC to use lookup table based sine function #8491

Audio: DRC: Change DRC to use lookup table based sine function #8491

Conversation

singalsu commented Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Jan 11, 2024

singalsu commented Nov 17, 2023 •

edited

Loading