Audio: Enhancing audio quality tests for device testing #6056

ShriramShastry · 2022-07-28T12:26:15Z

For the purpose of remote device testing, this check-in adds a correction to the scripts that evaluate gain,
frequency responsiveness, total harmonic distortion plus noise, and dynamic range.

The threshold scale has been defined and the decibel full scale computation has been made easier using new scripts.

You require Octave or Matlab. The scripts now operate properly in both Matlab and octave.

The user account, audio device, audio format, and capture sound card audio device and audio format for the remote device must be modified in the configuration file sof test perf config.m.

Signed-off-by: shastry malladi.sastry@intel.com

tools/test/audio/sof_test_perf_config.m

lgirdwood · 2022-08-02T09:52:19Z

@singalsu pls review.

singalsu

Please split this PR to multiple commits. E.g. commit per test case and split common test framework changes to topics/issues.

tools/test/audio/process_test.m

tools/test/audio/sof_test_perf.m

tools/test/audio/std_utils/level_dbfs.m

tools/test/audio/test_utils/find_test_signal.m

tools/tune/crossover/crossover_gen_coefs.m

tools/tune/eq/example_spk_eq.m

lgirdwood · 2022-09-01T12:04:22Z

@btian1 pls also review

btian1 · 2022-09-08T12:51:15Z

tools/test/audio/sof_test_perf.m

possible have more var name? instead of use test always, it may make confused or generate unexpected result, what do you think?

btian1

Hi, Seppo, Malldi

I know those test's basic idea is get loopback data and compare with matlab reference(or input) with some point, but when look into it, still feel hard to fully understand.

My request is: can you pick one .m file and add detailed explain for the mechanism and each line to make us understand more better? sorry, just feel can't understand well and want to understand well, :)

Thanks
Tim

ShriramShastry · 2022-09-08T14:07:10Z

Hi, Seppo, Malldi

I know those test's basic idea is get loopback data and compare with matlab reference(or input) with some point, but when look into it, still feel hard to fully understand.

My request is: can you pick one .m file and add detailed explain for the mechanism and each line to make us understand more better? sorry, just feel can't understand well and want to understand well, :)

Thanks Tim

If you have a more detailed question, I'm happy to try to answer it. However, I recommend using debug breakpoints to help with comprehension.

lgirdwood

@singalsu I cant review the matab, but I will merge after you are happy with the PR.

singalsu · 2022-09-12T10:06:20Z

@ShriramShastry Fred's patches are now merged. Please check the conflicts in process_test.m.

ShriramShastry · 2022-09-12T14:07:21Z

@ShriramShastry Fred's patches are now merged. Please check the conflicts in process_test.m.

Sure, Thanks a lot

tools/test/audio/std_utils/fr_test_measure.m

lgirdwood

@ShriramShastry I cant really review matlab, my only comment is that this PR makes a lot of changes and I would break these changes down into several patches (probably one for each matlab feature/test/file). This way it's easier and faster for reviewers.

singalsu · 2022-10-19T07:52:59Z

Yep, please split this to topics. E.g. one topic is compatibility with Matlab for scripts those have been apparently used with Octave. One commit per bug fixed. One commit per test improvement etc.

For the purpose of remote device testing, this check-in adds a correction to the scripts that evaluate gain,frequency responsiveness, total harmonic distortion plus noise Signed-off-by: shastry <malladi.sastry@intel.com>

The threshold scale has been defined and the decibel full scale computation has been made easier using new scripts. The dynamic range test is included in the test suite Relax the marker position for fr and thdn. Signed-off-by: shastry <malladi.sastry@intel.com>

… testing The check-in overcomes Octave and Matlab compatibility issues. The performance of matlab and octave is now comparable. Signed-off-by: shastry <malladi.sastry@intel.com>

lgirdwood

@ShriramShastry this is all matlab and needs @singalsu to review. I can merge when you are both aligned.

singalsu

Comments for first commit

singalsu · 2022-11-04T12:02:59Z

tools/test/audio/process_test.m

-	n_pass = r.n_pass;
-	n_na = r.n_na;
-	if r.n_fail < 1 && r.n_pass < 1
-		fprintf('\nERROR: No test results.\n');


I'd like to keep the error message about no tests executed. What problem did this cause?

I don't believe there is a problem . Test not run should be the interpretation.
I am OK, I'II include back No test results case.

singalsu · 2022-11-04T12:27:30Z

tools/test/audio/std_utils/thdnf_test_measure.m

+if isempty(test.thdnf_max)
+    error('Set either thdnf_max or thdnf_mask_f & thdnf_mask_hi but not both');
+elseif isempty(test.thdnf_mask_hi)
+    error('thdnf_mask_hi must be set when thdnf_mask_f is defined');


There's many different changes in this commit "Audio: BugFix : Enhancing audio quality tests for device testing". I'd split this further to topics (process_test, perf_test, thd+n, ...) and explain in commit message why it's done.

This change needs an own commit and explanation. To me it looks like 2nd if is wrong. What is the problem that is being fixed here?

To me it looks like 2nd if is wrong.
Ahaah, Good catch . I will make the correction, This should be if instead of elseif

singalsu · 2022-11-04T12:29:49Z

tools/tune/crossover/crossover_coef_quant.m

-	crossover_quant.lp_coef(i) = [lp_a lp_b 0 16384];
-	crossover_quant.hp_coef(i) = [hp_a hp_b 0 16384];
+	crossover_quant.lp_coef{i} = [lp_a lp_b 0 16384];
+	crossover_quant.hp_coef{i} = [hp_a hp_b 0 16384];


This crossover chang seems to be for Matlab compatibility patch.

singalsu · 2022-11-04T12:30:54Z

tools/tune/eq/mls_freq_resp.m

-measfn = sprintf('mls-%s.wav', id);
-csvfn = sprintf('mls-%s.txt', id);
+measfn = sprintf('mls-%d.wav', id);
+csvfn = sprintf('mls-%d.txt', id);


id is supposed to be text, why number?

%s should have returned id, but what we get instead is a strange character, which suggests that the id character has been lost.

sprintf('mls-%s.wav', 'id') and NOT sprintf('mls-%s.wav', id) should be used if you are expecting a character id.

The second option is to set id = 'id' and then use sprintf("mls-%s.wav", id) to produce mls-id.wav output

I don't think having a name is useful, hence I think a number is preferable.

I preferred a name like id = 'notebook_x' to have more meaningful file names. Especially the .csv that is a report is useful to be an easy name to remember.

fine, I will provide a name . Thanks

singalsu · 2022-11-04T12:31:52Z

tools/tune/eq/mls_freq_resp.m

 y = [];
 if selftest
-	labels = cell(play_cfg.nch * rec_cfg.nch + 1, 1);
+	labels = cellstr(char(play_cfg.nch * rec_cfg.nch + 1, 1));


Next three look like Matlab compatibility changes

singalsu · 2022-11-04T12:37:30Z

tools/tune/eq/mls_freq_resp.m


 if selftest
-        idx = find(f < f_hi);
+        idx = f < f_hi;


I personally like more find to not let this confuse idx to be a boolean. What's the benefit, I don't think this caused issues?

Line Number 204, f(idx) is an array or `cell array' . The performance is improved using logical indexing instead of FIND. If we are not considering boolean or logical array indexing , then shape of the output vector f(idx) could change . To retain the same output shape, we should replace find(f(idx)) by f(:).

singalsu

Comments for 2nd commit

singalsu · 2022-11-04T12:56:56Z

tools/test/audio/sof_test_perf.m

 %% Get playback test play and capture configuration
 test = get_config(test);

 %% Run tests


The topic of this commit is too generic, this looks like "Add dynamic range test to sof_test_perf.m".

singalsu · 2022-11-04T12:58:14Z

tools/test/audio/std_utils/fr_test_input.m

 test.sm = 3; % Seek start marker from 3s from start
 test.em = 3; % Seek end marker from 3s from end
-test.mt = 0.1; % Error if marker positions delta is greater than 0.1s
+test.mt = 2; % Error if marker positions delta is greater than 2s


Put this to own commit and describe (add robustness for real-device test?).

singalsu · 2022-11-04T13:00:05Z

tools/test/audio/std_utils/fr_test_measure.m


 %% Check if upper and lower mask is defined
-if length(test.fr_mask_flo) || length(test.fr_mask_fhi)
+if ~isempty(test.fr_mask_flo) || ~isempty(test.fr_mask_fhi)


This to first cleanup patch if this is not fixing anything, does it? Personally I find length() easier to understand than "not is empty?"

singalsu · 2022-11-04T13:00:55Z

tools/test/audio/std_utils/fr_test_measure.m

 	test.fr_hi = 0;
 end
 if test.fr_lo > 0 || test.fr_hi > 0
-	test.fr_mask_flo = [];


This to separate commit, e.g. use default frequency response mask.

singalsu · 2022-11-04T13:01:24Z

tools/test/audio/std_utils/fr_test_measure.m

 				  test.fr_mask_mhi(:,j), f_log, 'linear');
                over_mask = test.m(:,j)-mask_hi';
-                idx = find(isnan(over_mask) == 0);
+                idx = ~isnan(over_mask);


this and next to cleanup commit

singalsu · 2022-11-04T13:02:25Z

tools/test/audio/std_utils/level_dbfs.m

 %
 % Output
-% dbfs - sigmal level in dBFS
+% dbfs - signal level in dBFS


to cleanup commit

singalsu · 2022-11-04T13:03:08Z

tools/test/audio/std_utils/level_dbfs.m

 %% Reference AES 17 3.12.3
-level_ms = mean(x.^2);
-dbfs = 10*log10(level_ms + 1e-20) + 20*log10(sqrt(2));
+dbfs = 20*log10(rms(x) * sqrt(2));


singalsu · 2022-11-04T13:03:52Z

tools/test/audio/std_utils/thdnf_test_input.m

 test.sm = 3; % Seek start marker from 3s from start
 test.em = 3; % Seek end marker from 3s from end
-test.mt = 0.1; % Error if marker positions delta is greater than 0.1s
+test.mt = 2; % Error if marker positions delta is greater than 2s


This is 2nd for "improve real device tests robustness"?

singalsu · 2022-11-04T13:04:37Z

tools/test/audio/test_utils/find_test_signal.m

 nt_use = [];
 nt_skip = [];
-
+trace_en = false; % Enable to trace algorithm -analysis


Find_test_signal() change to own commit and describe what the changes do.

singalsu

Comments for 3rd commit

singalsu · 2022-11-04T13:06:48Z

tools/test/audio/std_utils/dr_test_measure.m

 level_out = level_dbfs(y);
 level_final = level_dbfs(y_n);
-test.dr_db = level_out-level_final-test.a_db;
+test.dr_db = level_out - level_final - test.a_db;


This is not compatibility but a code cleanup, goes to first commit.

singalsu · 2022-11-04T13:08:13Z

tools/test/audio/std_utils/thdnf_test_measure.m

-	fidx = find(test.thdnf(idx, 1) > mask_hi);
-	if length(fidx) > 0
+	fidx = find(test.thdnf(idx, 1) > mask_hi, 1);
+	if ~isempty(fidx)


Is this for Matlab compatibility? Looks more like cleanup, though I'm not 100% sure. Please check.

isempty is faster than comparing length to 0. Since it is a general performance improvement, compatibility section does not apply to it.

singalsu · 2022-11-04T13:08:32Z

tools/test/audio/test_utils/mix_sweep.m

 % Author: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

-%% Adjust tone lengt to integer number of samples
+%% Adjust tone length to integer number of samples


singalsu · 2022-11-04T13:09:55Z

tools/tune/crossover/crossover_gen_coefs.m

 % Generate the a,b coefficients for a second order
 % low pass butterworth filter
-function lp = lp_iir(fs, fc, gain_db)
+function lp = lp_iir(fs, fc, ~)


Please explain why gain_db is removed? It does not look like Matlab compatibility change.

Yes, I agree that I don't see the compatibility between Octave and Matlab as a problem, more as a general coding error.
If you recommend more commits for general improvement, I'm happy to integrate them.

First option: Input argument gain_db is unused, so this is OK, to be replaced with ~ i.e. avoid addition text label. There is a typographical error in the declaration. The body of the function does not use specified input argument.
or
Second option: lp_iir() should be 2 input argument instead of 3. but this need change across the all files across the code base. In the current code, the calling routines are filled with 0 all over for 3rd input argument.

So I opted first option

singalsu · 2022-11-04T13:10:53Z

tools/tune/crossover/example_crossover.m

-blob_fn = "../../ctl/crossover_coef.blob" % Blob binary file
-alsa_fn = "../../ctl/crossover_coef.txt" % ALSA CSV format file
+blob_fn = "../../ctl/crossover_coef.blob"; % Blob binary file
+alsa_fn = "../../ctl/crossover_coef.txt"; % ALSA CSV format file


These are cleanup, not compatibility.

singalsu · 2022-11-04T13:17:10Z

tools/tune/eq/eq_compute.m

 %% Define target (e.g. speaker) response as parametric filter. This could also
 %  be numerical data interpolated to the grid.
-if length(eq.parametric_target_response) > 0
+if ~isempty(eq.parametric_target_response)


Are these length() vs. ~isempty() cleanup or Matlab compatibility?

The execution speed of isempty is faster then comparing length to 0, therefore the proposal is replace length with isempty

singalsu · 2022-11-04T13:20:20Z

tools/tune/eq/eq_compute.m

 myeps = 1e-3;
 hdzeros = roots(blin);
-ind1 = find( abs(hdzeros) < (1-myeps) );
+ind1 =  abs(hdzeros) < (1-myeps) ;


Extra blanks before ; in this and next ones.

singalsu · 2022-11-04T13:21:16Z

tools/tune/eq/eq_define_parametric_eq.m

+        b = [A * A, 0, 0];
+        a = [1, 0, 0];
+        return;
+    end


cleanup, not a compatibility change

singalsu · 2022-11-04T13:21:45Z

tools/tune/eq/eq_define_parametric_eq.m

+        b = [0, 0, 0];
+        a = [1, 0, 0];
+        return;
+    end


cleanup, not compatibility. Also no point to change indent by 8 to 4. Since FW code uses 8 with tab, we could default to it for Matlab. The indents are well visible with 8, and a huge accumulated indent indicates issue in code.

Sure, I will change my editor indent settings with 8. Thanks !!

singalsu · 2022-11-04T13:22:54Z

tools/tune/eq/example_spk_eq.m

+    eq.fir_length = 63;
+    eq.fir_autoband = 0;
+    eq.fmin_fir = 900;
+    eq.fmax_fir = 10700;


Please set up your Matlab to indent by 8, also prefer tab if possible. Unnecessary change.

singalsu · 2022-11-04T13:29:23Z

tools/test/audio/process_test.m

 	fprintf('Number of skipped tests = %d\n', r.n_skipped);
 	fprintf('============================================================\n');

-	n_fail = r.n_fail;


Please prefix all your commit titles with Tools: Test: So that reviewers and and other git log viewers know what these are about. These do not contain changes for audio components.

ShriramShastry

I have included my comments. For other's comments, I'll make the necessary changes for the code check-in.

ShriramShastry · 2022-11-06T06:38:53Z

tools/tune/eq/mls_freq_resp.m

-measfn = sprintf('mls-%s.wav', id);
-csvfn = sprintf('mls-%s.txt', id);
+measfn = sprintf('mls-%d.wav', id);
+csvfn = sprintf('mls-%d.txt', id);


%s should have returned id, but what we get instead is a strange character, which suggests that the id character has been lost.

sprintf('mls-%s.wav', 'id') and NOT sprintf('mls-%s.wav', id) should be used if you are expecting a character id.

The second option is to set id = 'id' and then use sprintf("mls-%s.wav", id) to produce mls-id.wav output

I don't think having a name is useful, hence I think a number is preferable.

ShriramShastry · 2022-11-06T07:09:52Z

tools/tune/eq/mls_freq_resp.m


 if selftest
-        idx = find(f < f_hi);
+        idx = f < f_hi;


Line Number 204, f(idx) is an array or `cell array' . The performance is improved using logical indexing instead of FIND. If we are not considering boolean or logical array indexing , then shape of the output vector f(idx) could change . To retain the same output shape, we should replace find(f(idx)) by f(:).

sys-pt1s · 2023-04-06T10:20:00Z

Can one of the admins verify this patch?

ShriramShastry force-pushed the shastry_audioquality_develop branch from 33e93b4 to e1525df Compare July 28, 2022 13:02

ShriramShastry added this to the v2.1 milestone Jul 28, 2022

ShriramShastry force-pushed the shastry_audioquality_develop branch 2 times, most recently from 249e58e to 40c6875 Compare July 28, 2022 13:55

ShriramShastry requested review from aiChaoSONG, andrula-song, juimonen and sebcarlucci July 28, 2022 14:30

lgirdwood reviewed Aug 1, 2022

View reviewed changes

tools/test/audio/sof_test_perf_config.m Outdated Show resolved Hide resolved

ShriramShastry force-pushed the shastry_audioquality_develop branch from 40c6875 to 8225cea Compare August 1, 2022 12:14

ShriramShastry requested a review from marc-hb August 2, 2022 03:05

singalsu requested changes Aug 8, 2022

View reviewed changes

singalsu reviewed Aug 8, 2022

View reviewed changes

ShriramShastry force-pushed the shastry_audioquality_develop branch 3 times, most recently from 106f443 to 4519c84 Compare September 1, 2022 11:39

ShriramShastry marked this pull request as ready for review September 1, 2022 11:51

ShriramShastry requested review from a team, dbaluta, lbetlej, mmaka1 and plbossart as code owners September 1, 2022 11:51

ShriramShastry requested review from lgirdwood and singalsu September 1, 2022 11:51

ShriramShastry force-pushed the shastry_audioquality_develop branch from 4519c84 to dc55e7c Compare September 1, 2022 12:01

ShriramShastry requested review from btian1 and removed request for sebcarlucci September 1, 2022 12:07

btian1 reviewed Sep 8, 2022

View reviewed changes

lgirdwood reviewed Sep 9, 2022

View reviewed changes

ShriramShastry force-pushed the shastry_audioquality_develop branch 4 times, most recently from 1a833ed to 2b2a57a Compare September 12, 2022 15:13

marc-hb reviewed Sep 12, 2022

View reviewed changes

tools/test/audio/std_utils/fr_test_measure.m Outdated Show resolved Hide resolved

ShriramShastry requested a review from lgirdwood September 19, 2022 10:51

lgirdwood reviewed Sep 19, 2022

View reviewed changes

ShriramShastry added 3 commits November 3, 2022 14:21

Audio: BugFix : Enhancing audio quality tests for device testing

0343001

For the purpose of remote device testing, this check-in adds a correction to the scripts that evaluate gain,frequency responsiveness, total harmonic distortion plus noise Signed-off-by: shastry <malladi.sastry@intel.com>

Audio:Matlab compatibility : Enhancing audio quality tests for device…

4060f44

… testing The check-in overcomes Octave and Matlab compatibility issues. The performance of matlab and octave is now comparable. Signed-off-by: shastry <malladi.sastry@intel.com>

ShriramShastry force-pushed the shastry_audioquality_develop branch from 2b2a57a to 4060f44 Compare November 3, 2022 14:06

ShriramShastry requested a review from lgirdwood November 3, 2022 15:33

lgirdwood reviewed Nov 3, 2022

View reviewed changes

singalsu reviewed Nov 4, 2022

View reviewed changes

ShriramShastry commented Nov 6, 2022

View reviewed changes

kv2019i modified the milestones: v2.1, v2.6 Feb 15, 2023

lgirdwood modified the milestones: v2.6, TBD May 10, 2023

marc-hb mentioned this pull request Jun 6, 2023

Tools: Test: Audio: Valgrind run related fixes #7719

Merged

Audio: Enhancing audio quality tests for device testing #6056

Are you sure you want to change the base?

Audio: Enhancing audio quality tests for device testing #6056

Uh oh!

Conversation

ShriramShastry commented Jul 28, 2022

Uh oh!

Uh oh!

lgirdwood commented Aug 2, 2022

Uh oh!

singalsu 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!

lgirdwood commented Sep 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btian1 left a comment

Choose a reason for hiding this comment

Uh oh!

ShriramShastry commented Sep 8, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

singalsu commented Sep 12, 2022

Uh oh!

ShriramShastry commented Sep 12, 2022

Uh oh!

Uh oh!

lgirdwood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu commented Oct 19, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

singalsu 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!

ShriramShastry Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ShriramShastry Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment •

edited

Loading

ShriramShastry Nov 7, 2022 •

edited

Loading

ShriramShastry Nov 6, 2022 •

edited

Loading

ShriramShastry Nov 5, 2022 •

edited

Loading