Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tools: Tune: EQ: Normalize IIR and FIR separately #9086

Merged
merged 1 commit into from
May 3, 2024

Conversation

singalsu
Copy link
Collaborator

This allows more freedom to scale the equalizers. Common criteria resulted with safe scaling to unnecessarily silent result or risk for audible clipping in the equalizer.

This allows more freedom to scale the equalizers. Common criteria
resulted with safe scaling to unnecessarily silent result or
risk for audible clipping in the equalizer.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu marked this pull request as ready for review April 26, 2024 14:24
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShriramShastry pls review

Copy link
Contributor

@ShriramShastry ShriramShastry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the change. They seem to be good. I have identified a few improvements that can help improve the code.

Thank you

@@ -87,8 +87,8 @@ function cmocka_data_eq_iir()
eq = eq_defaults();
eq.fs = fs;
eq.enable_iir = 1;
eq.norm_type = 'peak';
eq.norm_offs_db = 1;
eq.iir_norm_type = 'peak';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion, a check like below would be helpful

function [x, yi] = get_chirp(fs, t_chirp)

% To ensure that the sampling frequency is positive and non-zero.
if fs <= 0
    error('Sampling frequency fs must be positive and non-zero.');
end
end

And redundant code inside
function [x, yi] = get_chirp(fs, t_chirp)

channels = 2;
imax = scale - 1;
imin = -scale;

error('Requested IIR normalization is not supported');
end
switch lower(eq.fir_norm_type)
case 'loudness'
Copy link
Contributor

@ShriramShastry ShriramShastry May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scaling factor calculations for FIR and IIR do not appear to account for possible zero values, which may result in division by zero (1/sens_iir or 1/m_lin_iir(i1k)). You might want to add a tick to avoid this.

Example:

>> eq.f = 48e3;
      i1k = find(eq.f > 1e3, 1, 'first') - 1;
      disp(i1k)
     0

then

        if sens_fir ~= 0
            g_fir = 1/sens_fir;
        else
            error('Sensation level is zero for FIR, cannot normalize by loudness.');
        end

For other's as well

@kv2019i kv2019i merged commit 03dbc7b into thesofproject:main May 3, 2024
43 of 46 checks passed
@singalsu singalsu deleted the tune_eq_fir_iir_separate_norm branch August 21, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants