obs-filters: Factor out NVIDIA Audio effects
Description
Updated, 02/25/24 This PR does the following:
- Factor out NVIDIA Audio Effects from Noise Suppression filter.
- Move NVIDIA Audio Effects to a new filter in a new nv-filters project.
- Migrate Noise Suppression filter settings to the new filter when NVIDIA Audio effects were used.
- Migrate NVIDIA AI Greenscreen to the new nv-filters project for easier maintainance of all NVIDIA MAxine effects.
Context: Currently, the three NVIDIA Audio Effects (noise suppression, room echo removal, noise suppression + room echo removal combined) are part of the noise suppression filter. Historically, it's because a lot of code was shared between speex, rnnoise & NVIDIA noise suppression. But the NVIDIA code has become bulkier & cumbersome due to:
- addition of other effects;
- addition of a deferred loading thread. The factorisation makes the code very difficult to maintain for (un)readability reasons. This will make it easier to add other audio effects, should we wish to. Developers life will be easier too when debugging. The code has been reorganized and comments added.
Note:
The migration to NVIDIA Audio Effects filter is done at the first .filter_audio call because the parent needs
to be known.
If the parent source is not enabled, the call is not made but the following message is displayed.
Bugfix: I also added a mutex in the process_fx function to avoid a crash when swapping effects.
Motivation and Context
Code cleanup is good. Readable code is good. This will make it easier to add other audio effects, should we wish to. Developers life will be easier too when debugging. The code has been reorganized and comments added.
How Has This Been Tested?
The new filter works fine. Tested with noisy demo files provided by NVIDIA sdk.
- noise suppression works fine.
- room echo removal works fine too.
- the combined effect works fine. The auto-migration from Noise Suppression to NVIDIA Audio filter works fine: https://github.com/obsproject/obs-studio/assets/9283407/11f12f2b-434f-4384-af17-428e5e4caa55
The Greenscreen filter still works.
Types of changes
- Bug fix (non-breaking change which fixes an issue)
- Code cleanup (non-breaking change which makes code smaller or more readable)
Checklist:
- [x] My code has been run through clang-format.
- [x] I have read the contributing document.
- [x] My code is not on the master branch.
- [x] The code has been tested.
- [x] All commit messages are properly formatted and commits squashed where appropriate.
- [x] I have included updates to all appropriate documentation.
Have you tested both this filter and the legacy one loaded together? I am concerned that NVAFX might not be too happy with multiple instances from a single application.
Have you tested both this filter and the legacy one loaded together? I am concerned that NVAFX might not be too happy with multiple instances from a single application.
That was one of my concerns too. So Yeah I did test with both filters (old and new).
The top filter is the new one, with denoiser. The bottom one is the current noise suppression filter, set on dereverb.
I'm testing against a wav file which has both reverb + background noise.
@RytoEX @notr1ch Instead of having a transition period with both old and new filters, what do you think of graying out nvidia from noise suppression? And removing nvidia code from noise suppression already. There's no automated way to transfer from old to new filter anyway. So setups will be broken at some point eventually. Not sure there's a point in delaying the pain which also delays the benefit. There'll be pain anyway...
For the sake of discussion I've added a commit completely removing NVIDIA Audio Effects from noise suppression. It can be removed if it's preferred to have a transition period where both old and new filter are working.
PRO:
- no collision between the two filters loading the SDK (cf R1ch comment).
CON:
- this will break setups.
- the latter drawback is unfortunately unavoidable; it will happen sooner or later, so it might as well be sooner.
I'ld like to suggest folding commit message like below.
obs-filters: Factor out NVIDIA Audio effects
Currently, the three NVIDIA Audio Effects (noise suppression, room echo
removal, noise suppression + room echo removal combined) are part of the
noise suppression filter. Historically, it's because a lot of code was
shared between speex, rnnoise & NVIDIA noise suppression. But the NVIDIA
code has become bulkier & cumbersome due to:
- addition of other effects;
- addition of a deferred loading thread.
The factorisation makes the code very difficult to maintain for
(un)readability reasons. This will make it easier to add other audio
effects, should we wish to. Developers life will be easier too when
debugging. The code has been reorganized and comments added.
I also added a mutex in the process_fx function to avoid a crash when
swapping effects. A deprecation warning has been added to the noise
suppression filter to direct users to the new filter.
Please note that there is a CR instead of LF at the end of the line swapping effects..
I'ld like to suggest folding commit message like below.
obs-filters: Factor out NVIDIA Audio effects Currently, the three NVIDIA Audio Effects (noise suppression, room echo removal, noise suppression + room echo removal combined) are part of the noise suppression filter. Historically, it's because a lot of code was shared between speex, rnnoise & NVIDIA noise suppression. But the NVIDIA code has become bulkier & cumbersome due to: - addition of other effects; - addition of a deferred loading thread. The factorisation makes the code very difficult to maintain for (un)readability reasons. This will make it easier to add other audio effects, should we wish to. Developers life will be easier too when debugging. The code has been reorganized and comments added. I also added a mutex in the process_fx function to avoid a crash when swapping effects. A deprecation warning has been added to the noise suppression filter to direct users to the new filter.Please note that there is a CR instead of LF at the end of the line
swapping effects..
Not sure if it's your git client or mine; but for me on windows, all lines are CRLF ...
Also, what's the problem with the formatting ? the commit stays below 72 ...
The problem was the line below in the commit has 89 characters.
A deprecation warning has been added to the noise suppression filter to
For the CR LF, I saw only CR instead of CR LF at the end of swapping effects..
The problem was the line below in the commit has 89 characters.
A deprecation warning has been added to the noise suppression filter toFor the CR LF, I saw only CR instead of CR LF at the end of
swapping effects..
Notepad ++ is counting 71 chars ...
This is how the commit message shows on github:
I don't see any issue.
I'm sorry but I don't understand.
These two lines are not separated by LF but separated only by CR. So it becomes one line and is counted as 89 characters.
swapping effects.
A deprecation warning has been added to the noise suppression filter to
(When I first checked the commit message by script, the text before CR was cleared on my terminal and showed the text only after the CR.)
These two lines are not separated by LF but separated only by CR. So it becomes one line and is counted as 89 characters.
swapping effects. A deprecation warning has been added to the noise suppression filter to(When I first checked the commit message by script, the text before CR was cleared on my terminal and showed the text only after the CR.)
ok i spotted what you say. It was not displayed with fork (my git client) but with git on CLI I see it; it's corrected now.
The changes look fine but I agree that the missing migration of the the old filter to the new one could be a problem as it might break existing setups and we don't have good way to notify users about this yet.
Is there maybe a way for the new filter variant to always be loaded before the old noise suppression filter and have it load and apply the old settings? This way we could "convert" the old filter into the new one automatically, remove the old settings, and have the old filter disabled entirely.
PR Updated (Feb. 25, 2024) I've overhauled this PR. @PatTheMav I've written an auto-migration from the old "Noise Suppression" filter to the new "NVIDIA Audio Effects" filter. See video: https://github.com/obsproject/obs-studio/assets/9283407/7c69949e-9f6b-453f-9044-c63bbecddc69
@notr1ch Thanks to this migration, I've removed completely nvafx from 'Noise Suppression'; the NVIDIA afx dll is therefore loaded once.
Furthermore, I've migrated all NVIDIA filters (both audio and video) to their own projects so that if there's an issue, obs-filter.dll will not be affected.
PR updated to reflect comments by @RytoEX I checked that the porting still worked fine. I checked the porting works whether the audio source is active or inactive. The old noise suppress filter is destroyed and a new one created with the same name, except for the suffix _ported
Apologies. Merging #11043 caused a merge conflict here.
Apologies. Merging #11043 caused a merge conflict here.
rebased and fixed the conflict