Audio: TDFB: Add enum and switch control to beamformer #4055

singalsu · 2021-04-19T16:11:05Z

This patch set updates the tools, topology, processing component for beam on/off and angle control.

Note: The enum is currently commented out to check CI results. The topology can't load without multiple controls kernel patch.

singalsu · 2021-04-19T16:12:44Z

This is the same patch set as #3780 but updated for main branch.

lgirdwood

@ShriramShastry can you review the matlab code. Thanks.

src/audio/tdfb/tdfb.c

src/include/kernel/abi.h

src/include/user/tdfb.h

ShriramShastry · 2021-04-20T03:58:26Z

@ShriramShastry can you review the matlab code. Thanks.

@lgirdwood . Thanks for the request. In the moment , I don't see the changes I have worked with @singalsu .
I will go over the file changes.

singalsu · 2021-04-20T10:43:34Z

@ShriramShastry Thanks, I'll next re-base your patch over this and get it back to you. It needs plenty of testing by both of us. It can be a separate PR by you after this is merged.

ShriramShastry · 2021-04-20T11:03:45Z

@ShriramShastry Thanks, I'll next re-base your patch over this and get it back to you. It needs plenty of testing by both of us. It can be a separate PR by you after this is merged.

Thanks a lot .
For Testing I would need DUT . I request @lgirdwood , @mengdonglin to help me with a notebook with two microphones

singalsu · 2021-04-22T13:36:54Z

Added topology sof-cml-rt1011-rt5682-tdfb_68mm_2ch.tplg for testing, no other changes in previous update.

cujomalainey

will take another deeper review on next push, just did a quick look this time

tools/tune/tdfb/bf_merge.m

src/include/kernel/abi.h

src/audio/tdfb/tdfb.c

src/include/sof/audio/tdfb/tdfb_comp.h

cujomalainey

the on/off type as enum is fine for now but please leave a TODO or issue to track the change to switch type

src/audio/tdfb/tdfb.c

lgirdwood

@ShriramShastry can you take a look at the matlab code.

src/audio/tdfb/tdfb.c

lgirdwood · 2021-06-16T11:17:02Z

src/audio/tdfb/tdfb.c

this then becomes

(void *)coefp += sizeof(*coef_data) + sizeof(int16_t) * coef_data->length;

since this data contains a container array with embedded data arrays..

and here then coefp = coef_data->coef + coef_data->length;

and here then coefp = coef_data->coef + coef_data->length;

doesn't this miss out the struct sof_fir_coef_data container size addition ?

I think @lyakh 's proposal is correct. Coef is the last flexible array member. Please correct me if this is not safe to do.

src/audio/tdfb/tdfb.c

src/include/kernel/abi.h

src/audio/tdfb/tdfb.c

lyakh · 2021-06-17T07:08:59Z

src/audio/tdfb/tdfb.c

and here then coefp = coef_data->coef + coef_data->length;

src/audio/tdfb/tdfb.c

src/include/user/tdfb.h

lyakh · 2021-06-17T07:47:11Z

tools/testbench/topology.c

unless priv_data is only allocated for one of controls, i.e. if there's one and only one SND_SOC_TPLG_CTL_BYTES control, you'll be overwriting process_ipc here and leaking memory? And below you're using process_ipc from the last iteration? Can .num_kcontrols be 0, then you'd be dereferencing NULL below?

Good catch! I recall I had earlier problem here with multiple same type controls. Then tplg was changed into three different control types and I forgot about it. This this likely hits again, need to test.

@lyakh I've hopefully now addressed your findings. As limitation this code can handle only one control private data append to new() IPC. A proper solution would need similar only cmd() based control passing to firmware. I feel it's too large task to do withing beamformer update and needs a separate PR.

This handler seems to work OK for testbench usage with the 5 controls presented in topology for the TDFB component.

tools/topology/m4/tdfb.m4

lyakh

Thanks for the update! Just two pretty minor remarks.

lyakh · 2021-07-09T06:33:55Z

src/audio/tdfb/tdfb.c

the whole function uses goto for cleanup... better add a label between cd_fail and fail below and jump to it. Or in fact you can even just jump to cd_fail - comp_data_blob_handler_free(cd->model_handler); won't hurt

lyakh · 2021-07-09T07:07:01Z

tools/testbench/topology.c

ret is already equal to 0 from ret = tplg_load_one_control(&ctl, &priv_data, file); above

Done, thanks for improvements!

This patch adds the enums set/get controls for beam direction (azimuth). The switch control provides a beam-off bypass from the configured microphones to channels. It is used when directional capture is not desired. The configuration blob is updated to pass filters settings for multiple angles, a bypass coefficients set, and microphones (x, y, z) coordinates for direction enum get (implemented later). Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch prevents topology parse failure when there are multiple controls, e.g. enum controls. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch improves the beamformer performance (vs. tap count) by shifting the extract window location for IFFT output by mean of all impulse responses center. Previously the first filter was centered and other filters were more towards start or end of impulse response. The change helps to cope with smaller filter length with larger size arrays with steer angles those spread the impulse response further away time-wise from each other. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The plots help to understand beamformer channel filters characteristic. Frequency response is usually all-pass type and group delays match the delays need to time-align the waveforms from steer angle. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch fixes the incorrect delaying of simulation input signal to microphones. Instead of delaying the old code advanced the input the more the larger the input is. E.g. It caused the tests for line arrays to show a mirrored beam pattern. The num_filters is replaced by mic_n (microphones count). It is the correct count to use here though usually filters count is the same. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch essentially adds support for steer_az/steer_el to be vector instead of scalar. The beam off preset is added by default automatically for N-channel in N-channel out configurations. Microphone (x,y,z) coordinates are passed in to blob for direction of arrival reporting. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Matlab does not support isfile(), so another command is used to check if a file exists. The new way works both in Octave and Matlab. The bf_defaults.m is fixed to properly initialize empty strings to cell arrays. It's needed with minimal command like usage as shown in SOF Docs examples. In bf_design.m the num_filters determination is moved after array helpers because other than line and circle do not use mic_n parameter. The bug resulted to zero num_filters. In bf_merge.m the unused bf3 and bf4 are removed. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The delay-and-sum beamformer has highest white noise gain (WNG) performance. It's good id suppressing microphone self noise. The beam is steered with pure delay adjust in the all-pass filter bank. Default type is still super directive. Use DSB or SDB as beamformer type to select. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Topologies 1.0 location has been changed. This update allows the tool to create the files into correct path. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This saves disk space from unnecessary data. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch contains updates to add beam on/off and direction controls. The common m4 tasks to add controls are placed to include files tdfb_controls.m4, tdfb_defines.m4, tdfb_undefines.m4. The single beam examples are replaced by blobs with 0, +/-30, and +/-90 degree angled beams. The Arrays are 50m, 68mm spaced for typical notebook microphones. The 28mm example is for four microphones. A four microphones for notebook with microphones at 0, 36, 146, and 182mm line locations. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

lgirdwood reviewed Apr 19, 2021

View reviewed changes

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

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

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

lgirdwood requested review from ShriramShastry, cujomalainey and johnylin76 April 19, 2021 19:57

cujomalainey reviewed Apr 19, 2021

View reviewed changes

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

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

src/include/kernel/abi.h Outdated Show resolved Hide resolved

src/include/user/tdfb.h Outdated Show resolved Hide resolved

singalsu force-pushed the tdfb_add_enum_control branch 2 times, most recently from 2ab37fc to 69eb110 Compare April 22, 2021 13:34

cujomalainey suggested changes Apr 23, 2021

View reviewed changes

tools/tune/tdfb/bf_merge.m Outdated Show resolved Hide resolved

src/include/kernel/abi.h Outdated Show resolved Hide resolved

lgirdwood added this to the v1.8 milestone Apr 27, 2021

singalsu force-pushed the tdfb_add_enum_control branch from 69eb110 to ea4e982 Compare April 28, 2021 14:11

cujomalainey reviewed Apr 29, 2021

View reviewed changes

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

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

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

src/include/sof/audio/tdfb/tdfb_comp.h Outdated Show resolved Hide resolved

singalsu force-pushed the tdfb_add_enum_control branch from ea4e982 to e471061 Compare April 30, 2021 11:14

singalsu marked this pull request as ready for review April 30, 2021 12:45

singalsu requested review from a team, dbaluta, lbetlej, mmaka1, plbossart and ranj063 as code owners April 30, 2021 12:45

singalsu requested review from cujomalainey and lgirdwood April 30, 2021 12:45

cujomalainey suggested changes Apr 30, 2021

View reviewed changes

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

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

lgirdwood modified the milestones: v1.8, v1.9 May 10, 2021

singalsu force-pushed the tdfb_add_enum_control branch from e471061 to 47fd722 Compare May 11, 2021 16:45

singalsu force-pushed the tdfb_add_enum_control branch from 47fd722 to a55db2e Compare May 12, 2021 09:59

singalsu requested a review from cujomalainey May 12, 2021 10:13

lgirdwood reviewed May 24, 2021

View reviewed changes

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

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

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

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

singalsu force-pushed the tdfb_add_enum_control branch 2 times, most recently from a461fc4 to ba6e81c Compare June 16, 2021 09:12

lgirdwood reviewed Jun 16, 2021

View reviewed changes

lyakh requested changes Jun 17, 2021

View reviewed changes

singalsu force-pushed the tdfb_add_enum_control branch 4 times, most recently from 7dd1413 to 62c8f38 Compare July 8, 2021 15:20

singalsu requested review from lgirdwood and lyakh July 8, 2021 15:24

lyakh reviewed Jul 9, 2021

View reviewed changes

singalsu added 11 commits July 9, 2021 14:17

Testbench: Fix for multiple controls parsing for process components

158b663

This patch prevents topology parse failure when there are multiple controls, e.g. enum controls. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Tune: Update TDFB topology include files location

375daed

Topologies 1.0 location has been changed. This update allows the tool to create the files into correct path. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Tune: TDFB: Do not include plot figures to saved .mat files

5a7dcac

This saves disk space from unnecessary data. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the tdfb_add_enum_control branch from 62c8f38 to b5b7663 Compare July 9, 2021 12:02

singalsu requested a review from lyakh July 9, 2021 12:06

lgirdwood approved these changes Jul 14, 2021

View reviewed changes

lgirdwood merged commit 0fa9f7d into thesofproject:main Jul 14, 2021

singalsu deleted the tdfb_add_enum_control branch July 21, 2021 12:03

Audio: TDFB: Add enum and switch control to beamformer #4055

Audio: TDFB: Add enum and switch control to beamformer #4055

Uh oh!

Conversation

singalsu commented Apr 19, 2021

Uh oh!

singalsu commented Apr 19, 2021

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ShriramShastry commented Apr 20, 2021

Uh oh!

singalsu commented Apr 20, 2021

Uh oh!

ShriramShastry commented Apr 20, 2021

Uh oh!

singalsu commented Apr 22, 2021

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment