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

Equalizer plugin, refinement to analysis display #3530

Merged
merged 1 commit into from Jul 15, 2017

Conversation

Projects
None yet
6 participants
@curlymorphic
Contributor

curlymorphic commented May 2, 2017

The spectral analysis was using a rectangle window, leading to high spectral leakage.
This pull request uses the Blackman-Harris window to give a display more representative of the audio.

Fixes #2703

Show outdated Hide outdated plugins/Eq/EqSpectrumView.cpp Outdated
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf May 3, 2017

Member

@michaelgregorius do you have time to test and approve this pull request?

Member

tresf commented May 3, 2017

@michaelgregorius do you have time to test and approve this pull request?

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius May 3, 2017

Contributor

@tresf, @curlymorphic: I had a look at the changed code and also compiled the branch myself to test it with a simple sine wave at 110Hz again.

First something that I noticed while looking at the code. It might be worthwhile to put the code that generates the window into a separate class in case we decide later to also implement other windowing functions.

As for the practical test it seems that unfortunately the patch doesn't have a big effect in the tested case.

Here's the EQ from master:
3530-sine-from-master

And here's the same signal with the patch applied:
3530-sine-from-branch

For reference here's the same signal shown in the frequency analyzer from REAPER using a buffer size of 2048:
reaper-blackman-harris-2048

Please note that LMMS cuts off the display at around -18 dB FS and that that's the reason why the plots don't look almost identical.

Judging by the plots and looking at the changed code the implementation seems to be correct. However, it seems that the main culprit for the high leakage is the buffer size of 2048 that's used for the FFT. If I set the buffer size to 32768 in the REAPER frequency analyzer the peak for the test signal becomes very sharp and does not spill over.

I think the following features might be interesting for the analyzer:

  • The ability to switch the windowing function.
  • The ability to switch the buffer sizes.
  • The ability to switch the scales from -18 dB to smaller values.

The reason for the last request is that some people mix with signals peaking at around -18 dB to have enough headroom. If you do this in LMMS and send such a channel through the EQ you will likely see almost nothing in the plot window.

Sorry, for requesting all this here. I usually hate pull requests that request more and more features myself. 😉

If you want we can add separate issues for these requests.

Contributor

michaelgregorius commented May 3, 2017

@tresf, @curlymorphic: I had a look at the changed code and also compiled the branch myself to test it with a simple sine wave at 110Hz again.

First something that I noticed while looking at the code. It might be worthwhile to put the code that generates the window into a separate class in case we decide later to also implement other windowing functions.

As for the practical test it seems that unfortunately the patch doesn't have a big effect in the tested case.

Here's the EQ from master:
3530-sine-from-master

And here's the same signal with the patch applied:
3530-sine-from-branch

For reference here's the same signal shown in the frequency analyzer from REAPER using a buffer size of 2048:
reaper-blackman-harris-2048

Please note that LMMS cuts off the display at around -18 dB FS and that that's the reason why the plots don't look almost identical.

Judging by the plots and looking at the changed code the implementation seems to be correct. However, it seems that the main culprit for the high leakage is the buffer size of 2048 that's used for the FFT. If I set the buffer size to 32768 in the REAPER frequency analyzer the peak for the test signal becomes very sharp and does not spill over.

I think the following features might be interesting for the analyzer:

  • The ability to switch the windowing function.
  • The ability to switch the buffer sizes.
  • The ability to switch the scales from -18 dB to smaller values.

The reason for the last request is that some people mix with signals peaking at around -18 dB to have enough headroom. If you do this in LMMS and send such a channel through the EQ you will likely see almost nothing in the plot window.

Sorry, for requesting all this here. I usually hate pull requests that request more and more features myself. 😉

If you want we can add separate issues for these requests.

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic May 3, 2017

Contributor

@michaelgregorius

Thanks for the in-depth report.

I will have to check the range on the display when I'm at my pc, but its alot more than the -18dB, its just squashed vertically when compared to reapers output. The +-18dB is the gain 'for the eq controls. It could be an idea to add the analysis scale on the right hand side.
What I am going to do is write the content of the fft buffer to a csv file and plot the curves, as I want to chech the display when over a larger y axis.

"First something that I noticed while looking at the code. It might be worthwhile to put the code that generates the window into a separate class in case we decide later to also implement other windowing functions"

I would agree. The fft-helper class, but that would cause confusion, as there is already a function there, that's suposed to do the windowing, that iirc is used in the spectrum analyzer plugin. However there are some problems with it. It calculates the window incorrectly, it also calculates the window for each sample played, 2 trig functions and 2 divides per sample is just silly.
I don't want to change the code for another plugin this close to a release.

"I think the following features might be interesting for the analyzer:

The ability to switch the windowing function.
The ability to switch the buffer sizes.
The ability to switch the scales from -18 dB to smaller values."

These are all good ideas, I was tempted to add these to this pull request but i decided againt adding new features at this stage. I do think it would be a good idea to add these suggested features, but in a seperate pr aimed at master. I can do a mock up of the additional features, in the not to distant future, but i would prefer to fix current bugs first.

"If you want we can add separate issues for these requests." Yes please.

Contributor

curlymorphic commented May 3, 2017

@michaelgregorius

Thanks for the in-depth report.

I will have to check the range on the display when I'm at my pc, but its alot more than the -18dB, its just squashed vertically when compared to reapers output. The +-18dB is the gain 'for the eq controls. It could be an idea to add the analysis scale on the right hand side.
What I am going to do is write the content of the fft buffer to a csv file and plot the curves, as I want to chech the display when over a larger y axis.

"First something that I noticed while looking at the code. It might be worthwhile to put the code that generates the window into a separate class in case we decide later to also implement other windowing functions"

I would agree. The fft-helper class, but that would cause confusion, as there is already a function there, that's suposed to do the windowing, that iirc is used in the spectrum analyzer plugin. However there are some problems with it. It calculates the window incorrectly, it also calculates the window for each sample played, 2 trig functions and 2 divides per sample is just silly.
I don't want to change the code for another plugin this close to a release.

"I think the following features might be interesting for the analyzer:

The ability to switch the windowing function.
The ability to switch the buffer sizes.
The ability to switch the scales from -18 dB to smaller values."

These are all good ideas, I was tempted to add these to this pull request but i decided againt adding new features at this stage. I do think it would be a good idea to add these suggested features, but in a seperate pr aimed at master. I can do a mock up of the additional features, in the not to distant future, but i would prefer to fix current bugs first.

"If you want we can add separate issues for these requests." Yes please.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 3, 2017

Member

I think you should try out the von Hann window, it's the default in calf analyzer and works pretty good. When using the blackman-harris windowing in the calf analyzer it added a lot of weird harmonics to the sine waves, while the von Hann window really seemed like the best choice.

Member

Umcaruje commented May 3, 2017

I think you should try out the von Hann window, it's the default in calf analyzer and works pretty good. When using the blackman-harris windowing in the calf analyzer it added a lot of weird harmonics to the sine waves, while the von Hann window really seemed like the best choice.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje May 3, 2017

Member

If I set the buffer size to 32768

Wouldn't that make the latency on the EQ go through the roof?

Member

Umcaruje commented May 3, 2017

If I set the buffer size to 32768

Wouldn't that make the latency on the EQ go through the roof?

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic May 3, 2017

Contributor

Wouldn't that make the latency on the EQ go through the roof?

Its the trade off between frequency and time resolution.

I think you should try out the von Hann window,

Thats a great idea, maybe for the update aimed after 1.2 release, we should collate a list of what windowing functions users would like, and any additional functionally required then plan and evaluate the expansion. I am keen to do this, could be a nice team project.

Contributor

curlymorphic commented May 3, 2017

Wouldn't that make the latency on the EQ go through the roof?

Its the trade off between frequency and time resolution.

I think you should try out the von Hann window,

Thats a great idea, maybe for the update aimed after 1.2 release, we should collate a list of what windowing functions users would like, and any additional functionally required then plan and evaluate the expansion. I am keen to do this, could be a nice team project.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius May 4, 2017

Contributor

Wouldn't that make the latency on the EQ go through the roof?

Yes, bigger windows definitively introduce some latency. However, I assume that this would only affect the graphical display and not the processing of the signal itself.

I am keen to do this, could be a nice team project.

I agree and I think that LMMS needs more of such team efforts/projects. Right now LMMS seems to be mainly developed by individual developers who contribute with pull requests. Obviously there is a limit to what a single developer can do and this keeps the project from implementing the bigger changes that are needed/wanted like a better separation between the core and GUI classes, realtime safe operations or the single window GUI.

Contributor

michaelgregorius commented May 4, 2017

Wouldn't that make the latency on the EQ go through the roof?

Yes, bigger windows definitively introduce some latency. However, I assume that this would only affect the graphical display and not the processing of the signal itself.

I am keen to do this, could be a nice team project.

I agree and I think that LMMS needs more of such team efforts/projects. Right now LMMS seems to be mainly developed by individual developers who contribute with pull requests. Obviously there is a limit to what a single developer can do and this keeps the project from implementing the bigger changes that are needed/wanted like a better separation between the core and GUI classes, realtime safe operations or the single window GUI.

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic May 6, 2017

Contributor

@Umcaruje
I have changed the fft window to use a Von Hann function, to see what you prefer?

I will remove the unused windowing function code, when a decision has been made as to what is preferred.

Contributor

curlymorphic commented May 6, 2017

@Umcaruje
I have changed the fft window to use a Von Hann function, to see what you prefer?

I will remove the unused windowing function code, when a decision has been made as to what is preferred.

@Umcaruje Umcaruje added this to the 1.2.0 milestone May 14, 2017

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jun 7, 2017

Member

Maybe it will be better to separate window initializing code to some new functions and implement switching between window functions.
If it is not needed for now, just providing windowing toggle button will be better.

Member

PhysSong commented Jun 7, 2017

Maybe it will be better to separate window initializing code to some new functions and implement switching between window functions.
If it is not needed for now, just providing windowing toggle button will be better.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Jun 10, 2017

Contributor

@curlymorphic I have added #3624, #3625 and #3626 to deal with the aforementioned features separately.

@PhysSong I agree that it would be a good idea to have a class or a collection of functions that can generate different types of windowing functions that can be reused in different places.

Contributor

michaelgregorius commented Jun 10, 2017

@curlymorphic I have added #3624, #3625 and #3626 to deal with the aforementioned features separately.

@PhysSong I agree that it would be a good idea to have a class or a collection of functions that can generate different types of windowing functions that can be reused in different places.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jun 17, 2017

Member

@Umcaruje
I have changed the fft window to use a Von Hann function, to see what you prefer?

I will remove the unused windowing function code, when a decision has been made as to what is preferred.

I have no preferred windowing function myself. I've tested this and it works fine. I think we can merge it as it is with a von Hann window and the commented out Blackman-Harris function and then get back to it after 1.2 is released.

Member

zonkmachine commented Jun 17, 2017

@Umcaruje
I have changed the fft window to use a Von Hann function, to see what you prefer?

I will remove the unused windowing function code, when a decision has been made as to what is preferred.

I have no preferred windowing function myself. I've tested this and it works fine. I think we can merge it as it is with a von Hann window and the commented out Blackman-Harris function and then get back to it after 1.2 is released.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jun 20, 2017

Member

Some windowing functions(Hamming and von Hann) are already implemented in fft_helpers.h, and I think it is possible to reuse them. If someone needs other window functions, simply adds it into fft_helpers.h and use it in plugins.

Member

PhysSong commented Jun 20, 2017

Some windowing functions(Hamming and von Hann) are already implemented in fft_helpers.h, and I think it is possible to reuse them. If someone needs other window functions, simply adds it into fft_helpers.h and use it in plugins.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 6, 2017

Member

Some windowing functions(Hamming and von Hann) are already implemented in fft_helpers.h, and I think it is possible to reuse them. If someone needs other window functions, simply adds it into fft_helpers.h and use it in plugins.

I think that can be fixed later here: #3625
We're pressed to release 1.2.0 and this is a pretty good place to merge this in my opinion.

@LMMS/developers Merge?

Member

zonkmachine commented Jul 6, 2017

Some windowing functions(Hamming and von Hann) are already implemented in fft_helpers.h, and I think it is possible to reuse them. If someone needs other window functions, simply adds it into fft_helpers.h and use it in plugins.

I think that can be fixed later here: #3625
We're pressed to release 1.2.0 and this is a pretty good place to merge this in my opinion.

@LMMS/developers Merge?

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 6, 2017

Member

Someone might want to see not windowed spectrum as before. I think windowing on/off button should exist.

Member

PhysSong commented Jul 6, 2017

Someone might want to see not windowed spectrum as before. I think windowing on/off button should exist.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 7, 2017

Member

@zonkmachine I really want to hear from @michaelgregorius on which windowing function is better, so we can clean up the code before merging.

Member

Umcaruje commented Jul 7, 2017

@zonkmachine I really want to hear from @michaelgregorius on which windowing function is better, so we can clean up the code before merging.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Jul 7, 2017

Contributor

@Umcaruje I don't think there is something like a "better" function. As can be seen here they all have certain tradeoffs: https://en.wikipedia.org/wiki/Window_function#A_list_of_window_functions

It seems that the main lobe of the FFT is more pronounced with Blackman-Harris. So I'd use Blackman-Harris for now and then implement the switching of windowing functions as part of #3625. One option for the switching should be something like "Off (Rectangular)" so that it can also be turned off.

Contributor

michaelgregorius commented Jul 7, 2017

@Umcaruje I don't think there is something like a "better" function. As can be seen here they all have certain tradeoffs: https://en.wikipedia.org/wiki/Window_function#A_list_of_window_functions

It seems that the main lobe of the FFT is more pronounced with Blackman-Harris. So I'd use Blackman-Harris for now and then implement the switching of windowing functions as part of #3625. One option for the switching should be something like "Off (Rectangular)" so that it can also be turned off.

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Jul 7, 2017

Contributor

@Umcaruje @michaelgregorius

If we can make a decision I will amend the pull request to suit. My vote would be Blackman-Harris,

I would like to stick with the decision of not changing the fft_helpers class this close to a release. I do plan on implementing the windowing functions in the fft_helper class, but I shall aim that pull request at master due to changes affecting more than just this plugin. Changing the Equalizer plugin to use this code can then be handled in #3625.

Contributor

curlymorphic commented Jul 7, 2017

@Umcaruje @michaelgregorius

If we can make a decision I will amend the pull request to suit. My vote would be Blackman-Harris,

I would like to stick with the decision of not changing the fft_helpers class this close to a release. I do plan on implementing the windowing functions in the fft_helper class, but I shall aim that pull request at master due to changes affecting more than just this plugin. Changing the Equalizer plugin to use this code can then be handled in #3625.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Jul 7, 2017

Contributor

@curlymorphic Sound reasonable and good to me!

Contributor

michaelgregorius commented Jul 7, 2017

@curlymorphic Sound reasonable and good to me!

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Jul 7, 2017

Contributor

@PhysSong is already working on updating the fft_helper class for the master branch in #3625
Sorry, I only just noticed

Contributor

curlymorphic commented Jul 7, 2017

@PhysSong is already working on updating the fft_helper class for the master branch in #3625
Sorry, I only just noticed

Equalizer plugin, refinement to analysis display
The spectural analysis was using a rectangle window, leading to high spectural leakage.
This pull request uses the Blackman-Harris window to give a display more representative of the audio.
@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 7, 2017

Member

Yeah, Blackman-Harris gets a 👍 from me as well then, this should be merged

Member

Umcaruje commented Jul 7, 2017

Yeah, Blackman-Harris gets a 👍 from me as well then, this should be merged

@Umcaruje Umcaruje merged commit b3054fd into LMMS:stable-1.2 Jul 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment