Audio: DRC: Change DRC to use lookup table based sine function by singalsu · Pull Request #8491 · thesofproject/sof

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

 	  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

 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

 	  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 <seppo.ingalsuo@linux.intel.com>

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 <seppo.ingalsuo@linux.intel.com>

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 <seppo.ingalsuo@linux.intel.com>

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 a team, dbaluta, kv2019i, lbetlej, marc-hb, mmaka1 and plbossart 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 andrula-song, btian1, lgirdwood and lyakh 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 <seppo.ingalsuo@linux.intel.com>

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

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

Conversation

singalsu commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btian1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu commented Jan 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShriramShastry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singalsu commented Jan 10, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jan 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

singalsu commented Nov 17, 2023 •

edited

Loading