sof: drc: Add DRC component with floating-point calculations #3415

johnylin76 · 2020-09-15T10:53:05Z

This commit adds Dynamic Range Compression (DRC) to the list of SOF
components. DRC in audio processing is intentional to reduce the
volume of loud sounds as compressing an audio signal's dynamic range.

This is still WIP.

lgirdwood

LGTM. @singalsu any comments.

src/audio/drc/drc.c

johnylin76 · 2020-09-17T10:19:35Z

I updated a compile-ok version of commit with the following modifications:

As the improvement from last version, hidden heavy parameter pre-calculation from component initialization. They should be calculated by CPU and passed by component config.
Implemented drc_func (process one source buffer and produce one sink buffer) in the manner of floating point. This is absolutely not a submit-able implementation however it could be worked first for verifying component architecture.

src/audio/Kconfig

singalsu · 2020-09-21T12:20:13Z

src/audio/drc/drc.c

Probably OK, but this would be a very small value as linear :)

src/audio/drc/drc.c

singalsu · 2020-09-21T13:03:20Z

src/audio/drc/drc_generic.c

I'm guessing this is already in your list of things to do but probably good idea to write own fractional version of logf() to avoid the C float math library.

singalsu · 2020-09-21T13:06:21Z

src/audio/drc/drc_generic.c

Do you plan to keep the floats? It might be OK with HiFi3 already but better to check impact to code size and speed.

I don't think we have the hifi spec handy, would need help managing that implementation

@cujomalainey I'll help to get the spec to you. It's very useful.

src/audio/drc/drc_generic.c

singalsu · 2020-09-21T13:26:54Z

src/audio/drc/drc_generic.c

If you plan to keep floats, would fast-math optimization avoid need for isnormal() check?

we are planning to remove floats. just doing floats initially for "get it up and running" then will move to fixed point

singalsu · 2020-09-21T13:31:28Z

src/audio/drc/drc_generic.c

sinf() looks expensive computationally. Is this executed when processing copy()?

I think it is called from copy

Unfortunately all math functions in the latest code are called from copy, and will be called multiple times.
We will try to implement the approximated cheaper alternatives.

singalsu · 2020-09-21T13:34:06Z

src/audio/drc/drc_generic.c

Would exponential settling of a recursive filter (IIR) be more economical to compute?

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

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

singalsu · 2020-09-21T13:42:56Z

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

There's a version in sof/math/decibels.h. Though I'm not sure if fit your needs. To achieve fast speed it's not as accurate expf(). The other way for lin2db is missing from there. If you plan to do a fractional version contributing there is welcome!

johnylin76 · 2020-09-22T10:54:04Z

Hi @singalsu , thanks for the detailed reviews.
For C float math library usage, I think our plan is to avoid all of those (at least avoid from processing copy()). The implementation right now could help us on verifying the overall architecture by testbench in the first hand, then reducing those heavy arithmetical computations.

johnylin76 · 2020-09-25T12:01:58Z

The PR is updated. I have verified the functionality of DRC implementation by testbench, comparing input and output stream samples with the golden model - DRC kernel in ChromeOS user space.

Note that this DRC firmware is still floating-point calculations. So of course the next step is to implement the fixed-point arithmetic version upon it.

lgirdwood · 2020-10-06T09:51:35Z

@cujomalainey @johnylin76 should we merge the DRC once it's functionally correct and then incrementally merge the optimisations as they are ready ? I'm thinking this may reduce the technical debt that has to be carried and reduce the reviewer load ?

cujomalainey · 2020-10-06T16:58:00Z

I'm ok with merging floating point if you are @lgirdwood, i think that would be a good way to keep the review clean.

lgirdwood · 2020-10-06T21:06:42Z

I'm ok with merging floating point if you are @lgirdwood, i think that would be a good way to keep the review clean.

@singalsu any comment. This would mean it would run on testbench (and would keep the review straight forward), not sure what other changes it would need to run on xtensa floating point though ?

cujomalainey · 2020-10-06T21:08:13Z

My guess is little to none, but it would likely be so slow that it would overrun every system we currently support

johnylin76 · 2020-10-07T00:55:30Z

@lgirdwood Currently I use a fake UUID since we haven't decide the UUID of DRC yet. Should we determine the UUID of DRC before merging?

johnylin76 · 2020-10-07T08:10:44Z

I also have implemented the delay-only mode which is used when DRC is disabled. Please refer to:
abee491

singalsu · 2020-10-07T10:14:26Z

@lgirdwood Also my +1 for merging floating point with testbench execute capability when OK for Johnny.

It reduces effort to work with optimized version. I've noticed that recent platforms (TGL) have quite a lot more processing power, even gcc built beamformer survives in real-time, so even some light optimization might allow starting real time tests with DRC.

lgirdwood · 2020-10-07T10:40:59Z

@lgirdwood Currently I use a fake UUID since we haven't decide the UUID of DRC yet. Should we determine the UUID of DRC before merging?

Yes please, just use a UUID generation tool and attach it. We can change it when significant updates are made. Please also change PR from draft.

This commit adds Dynamic Range Compression (DRC) to the list of SOF components. DRC in audio processing is intentional to reduce the volume of loud sounds and amplify the silent sounds as compressing an audio signal's dynamic range. This is the intermediate implementation with floating-point calculations. Signed-off-by: Pin-chih Lin <johnylin@google.com>

This commit adds the topology files for the drc component. The control bytes are generated by the tools in tune/drc. Signed-off-by: Pin-chih Lin <johnylin@google.com>

This commit adds the tools to generate the control bytes for the drc component. To generate the control bytes, run the example_drc.m script. To tweak the parameters modify the values in example_drc.m and run it. This is still WIP. A fixed set of coefficients is temporarily used in drc_generate_config.m Signed-off-by: Pin-chih Lin <johnylin@google.com>

Signed-off-by: Pin-chih Lin <johnylin@google.com>

Delay the input sample only and don't do other processing. This is used when the DRC is disabled. We want to do this to match the processing delay of other bands in multi-band DRC kernel case. Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 · 2020-10-08T09:49:47Z

@lgirdwood Currently I use a fake UUID since we haven't decide the UUID of DRC yet. Should we determine the UUID of DRC before merging?

Yes please, just use a UUID generation tool and attach it. We can change it when significant updates are made. Please also change PR from draft.

Thanks. Done. Is is then for me to merge.

lgirdwood · 2020-10-08T10:51:52Z

@johnylin76 merged, now good for the next phase !

lgirdwood added this to the v1.7 milestone Sep 15, 2020

lgirdwood reviewed Sep 15, 2020

View reviewed changes

src/audio/drc/drc.c Outdated Show resolved Hide resolved

lgirdwood requested review from cujomalainey and singalsu September 15, 2020 12:46

cujomalainey suggested changes Sep 15, 2020

View reviewed changes

cujomalainey reviewed Sep 16, 2020

View reviewed changes

src/audio/drc/drc.c Outdated Show resolved Hide resolved

johnylin76 force-pushed the drc_develop branch from 44d7e40 to b11230b Compare September 17, 2020 10:09