FEAT(client): Restore RNNoise for version 0.2 #6607
Conversation
guillermofbriceno
force-pushed
the
rnn2
branch
3 times, most recently
from
d5e9670 to
36e3c59
Compare
|
Just noticed I forgot to enable rnnoise so it builds by default, since it was originally turned off before Mumble switched to ReNameNoise. I had it turned on manually. Sorry in advance for prompting another build check. |
guillermofbriceno
force-pushed
the
rnn2
branch
from
36e3c59 to
7689a5d
Compare
guillermofbriceno
force-pushed
the
rnn2
branch
2 times, most recently
from
db9d1d9 to
b4024b0
Compare
|
@guillermofbriceno Is there a good reason that this PR is still in draft mode? |
It does not compile. See the discussion in the original issue. (It seems to be abandoned at this point) |
guillermofbriceno
force-pushed
the
rnn2
branch
from
b4024b0 to
b936b9f
Compare
|
I had a possible solution to the platform problem, hesitated whether it would work, and never got around to cleaning it up. I'll take another crack at it. |
guillermofbriceno
force-pushed
the
rnn2
branch
from
b936b9f to
7cf4f1c
Compare
|
Nice seeing the Windows build pass. FreeBSD failed from what looks like a Qt module version deprecation, rebased my commit to the latest master. |
guillermofbriceno
force-pushed
the
rnn2
branch
from
7cf4f1c to
bbeb71d
Compare
guillermofbriceno
force-pushed
the
rnn2
branch
2 times, most recently
from
68d53c9 to
74ad84e
Compare
|
@guillermofbriceno Could you rebase against |
Reverted the merge which added ReNameNoise as an RNNoise replacement. Added the git submodule from xiph/rnnoise after removing ReNameNoise. Chose to go with rnnoise's `main` here. Resolves mumble-voip#6395 Reverts mumble-voip#6364
guillermofbriceno
force-pushed
the
rnn2
branch
from
74ad84e to
e646e8a
Compare
There was a problem hiding this comment.
Overall:
-
I guess this compiles on all platforms, which I find slightly surprising so hats off. I don't have a Windows system to test it nor do I want one. So I guess we will see, if any runtime issues arise.
-
This reintroduces our CMake glue, which is not ideal, but I guess it technically works with CMake. Fair enough.
-
But, most importantly and for the record, I personally think upstream has not sufficiently proven that the noise reduction quality is as good or better than v0.1.
There seem to be a few issues floating around that show v0.2 has some quality regressions: e.g. it does not seem to filter keyboard clicks as good as v0.1 which may or may not be addressed by now.
It would have been nice of upstream to provide samples and or show us downstream projects which already use v0.2 so we can experience it ourselves. Nothing of that happened. It seems to be more of a "trust me, bro".
But OK, I realize I might be biased because I spent so many hours on creating the ReNameNoise fork, so I don't plan on blocking this PR. If it turns out that any issues arise from this in the Mumble 1.6 RCs, I will not have any hesitation reverting this, though.
|
|
||
| if(MSVC) | ||
| # Use malloc() and free() instead of variable length arrays (unsupported) | ||
| target_compile_definitions(rnnoise PRIVATE "USE_MALLOC") |
There was a problem hiding this comment.
USE_MALLOC is not present in upstream RNNoise. Davide introduced it in our first fork, because RNNoise was doing VLAs which it now apparently does not anymore. We can remove this then.
The _USE_MATH_DEFINES I'm not sure. It might be a MSVC specific. If it was instead also introduced for our first fork, please also remove it.
|
|
||
| file(READ "${RNNOISE_SRC_DIR}/model_version" rnnoise_model_hash) | ||
| string(STRIP ${rnnoise_model_hash} rnnoise_model_hash) | ||
| string(CONCAT rnnoise_model_url "${RNNOISE_MODEL_URL}" "${rnnoise_model_hash}" ".tar.gz") |
There was a problem hiding this comment.
I dislike the fact that we download the model from a random webserver at compile-time. My build process will not be affected but I strongly suspect this will most likely break the build process for some package maintainers on certain distros.
However, I have no immediate idea how to solve this. If this is not addressed before the merge, I advice downstream package maintainers (who have a problem with this) to just ship Mumble with ReNameNoise.
| @@ -831,11 +839,11 @@ void AudioInput::selectNoiseCancel() { | |||
| iArg = 1; | |||
| break; | |||
| case Settings::NoiseCancelRNN: | |||
| qWarning("AudioInput: Using ReNameNoise as noise canceller"); | |||
| qWarning("AudioInput: Using RNNoise as noise canceller"); | |||
There was a problem hiding this comment.
Please explicitly change this message to say RNNoise v0.2. That way we will know what version users are using when they file bug reports.
Also, please change this to use a Log::info() instead of a qWarning
Reverts #6364 and adds the xiph/rnnoise repository at
main. This addresses #6395.There were a couple of requirements for this PR set by @Hartmnt in #6395 (comment). I do not have multiple platforms to test the build, but it works on my Linux host. Regarding examples, there are some good examples posted on the regression item. A friend and I did some light testing on Mumble and had similar results. I do not have the hardware to test on multiple audio devices.
Other than that, I think this fulfills the other requirements, but please let me know. Since this reverts an old merge, forgive me if there are style issues.
Checks