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

obs-filters: Factor out NVIDIA Audio effects #9457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pkviet
Copy link
Member

@pkviet pkviet commented Aug 17, 2023

Description

Updated, 02/25/24
This PR does the following:

  1. Factor out NVIDIA Audio Effects from Noise Suppression filter.
  2. Move NVIDIA Audio Effects to a new filter in a new nv-filters
    project.
  3. Migrate Noise Suppression filter settings to the new filter when
    NVIDIA Audio effects were used.
  4. 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.
obs64_2024-02-25_18-53-49

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.

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:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@pkviet pkviet requested a review from notr1ch August 17, 2023 10:17
@pkviet pkviet added Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable labels Aug 17, 2023
@notr1ch
Copy link
Member

notr1ch commented Aug 17, 2023

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.

@pkviet
Copy link
Member Author

pkviet commented Aug 17, 2023

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).
obs64_2023-08-17_13-26-17

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.

plugins/obs-filters/data/locale/en-US.ini Outdated Show resolved Hide resolved
plugins/obs-filters/data/locale/en-US.ini Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
plugins/obs-filters/nvidia-audiofx-filter.c Outdated Show resolved Hide resolved
@pkviet pkviet force-pushed the nvidiadefactor branch 2 times, most recently from 70de258 to 84e4a18 Compare August 17, 2023 15:38
@pkviet
Copy link
Member Author

pkviet commented Aug 18, 2023

@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...

@pkviet pkviet force-pushed the nvidiadefactor branch 2 times, most recently from 46ea36a to 50e2a2e Compare August 20, 2023 11:53
@pkviet
Copy link
Member Author

pkviet commented Aug 20, 2023

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.

@pkviet pkviet removed the request for review from notr1ch August 21, 2023 17:21
@norihiro
Copy link
Contributor

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..

@pkviet
Copy link
Member Author

pkviet commented Aug 26, 2023

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 ...
notepad++_2023-08-26_21-43-48

Also, what's the problem with the formatting ? the commit stays below 72 ...

@norihiro
Copy link
Contributor

norihiro commented Aug 27, 2023

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..

@pkviet
Copy link
Member Author

pkviet commented Aug 28, 2023

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..

Notepad ++ is counting 71 chars ...
This is how the commit message shows on github:
chrome_2023-08-29_00-00-27
I don't see any issue.
I'm sorry but I don't understand.

@norihiro
Copy link
Contributor

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.)

@pkviet
Copy link
Member Author

pkviet commented Aug 29, 2023

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.

@PatTheMav
Copy link
Member

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.

@pkviet pkviet force-pushed the nvidiadefactor branch 3 times, most recently from b90db69 to 17dde44 Compare February 25, 2024 18:08
@pkviet
Copy link
Member Author

pkviet commented Feb 25, 2024

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.

@pkviet pkviet added this to the OBS Studio (Next Version) milestone Feb 25, 2024
@pkviet pkviet added the Seeking Testers Build artifacts on CI label Feb 25, 2024
This commit does the following:
1. Factor out NVIDIA Audio Effects from Noise Suppression filter.
2. Move NVIDIA Audio Effects to a new filter in a new nv-filters
project.
3. Migrate Noise Suppression filter settings to the new filter when
NVIDIA Audio effects were used.
4. 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.
I also added a mutex in the process_fx function to avoid a crash when
swapping effects.

Signed-off-by: pkv <pkv@obsproject.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants