wwmm / easyeffects

Limiter, compressor, convolver, equalizer and auto volume and many other plugins for PipeWire applications
GNU General Public License v3.0
6.55k stars 270 forks source link

Poor sound quality when using the pitch shift effect. #723

Closed ghost closed 2 years ago

ghost commented 4 years ago

Good day to all! I use the application mainly only to lower the pitch of all audio streams on a PC with Linux (openSUSE with KDE) by 32 cents so that the music goes into the A 432 Hz pitch. And I noticed that the quality of sound conversion leaves much to be desired ... This is especially noticeable in music and speech programs when viewing content on YouTube, and this manifests itself as a slight tremor of sound during speech inserts and when one note lasts for a while ... I think that the pitch shifter effect algorithm in your program is rustic. So I want the quality of the conversion to be much higher. I compared the conversion quality in your program with the sound conversion quality in the Foobar2000 player using the appropriate DSP plug-in, and in Foobar the sound is very good. I really want your pitch shift algorithm to be better in your program, so that the quality improves. When I researched this question, I stumbled on this material on the Internet https://github.com/tomszilagyi/tap-plugins Perhaps something from there would be useful for your program? Thank you!

DoctorBertalis commented 4 years ago

I left this review. There were some troubles with the previous account, do not ask. Now I have a new account.

wwmm commented 4 years ago

The pitch plugin was not written by me. It is a third party plugin from the rubberband library https://github.com/breakfastquay/rubberband. As I know nothing about this kind of algorithm I am not in a position to improve it.

As we have other problems with this plugin(sometimes it crashes) it may be worth to take a look at the one provided by tap-plugins.

Digitalone1 commented 4 years ago

There's also the pitch plugin by calf, so it could be removed and extra dependency.

Digitalone1 commented 4 years ago

Can you provide a way to test the quality of the plugin?

Maybe a specific YouTube song and a preset to test? It's weird Rubber Band is low quality since it's used in mpv to do pitch shift, but it could be changed.

It's preferable with calf shift plugin so we don't have an extra dependency.

Digitalone1 commented 4 years ago

Unfortunately pitch shift from tap-plugin can't be used because it's not stereo, only mono.

There's the one from Calf, but has less options, so I don't think it's same quality as Rubber Band's.

@DoctorBertalis did you try to set different options in PulseEffects to achieve same quality as foobar?

DoctorBertalis commented 4 years ago

I compared the sound of an audio file of a sine wave in the first case played in foobar2000 with the pitch shift plugin at minus 32 cents (without running Pulseeffects) and the sound of the same file played in a VLC player with Pulseeffects enabled with a plugin with a decrease of 32 cents (WAV file), in the first the sound was smooth, without jerks and artifacts, and in the second case, small jerks were heard and the sound was uneven with twitching ... In the case of Pulseeffects, I tried different settings options, the Sharpness parameter was "3", the rest of the settings did not affect the quality, and the Faster button was not pressed ... And if you use something from here? - https://github.com/mudlord/foobar2000-plugins/tree/master/foo_dsp_effect

Digitalone1 commented 4 years ago

Well, it seems in that repository there's pitch shift from rubberband. Maybe foobar is using the same plugin but it's doing something else to the sound we can't implement.

Digitalone1 commented 4 years ago

I think foobar is using a combination of soundtouch and rubberband plugins, while we're using only rubberband. Don't know how to do this, and I'm not even aware if soundtouch pitch could be used in PE since it's not a ladspa/lv2 plugin.

Digitalone1 commented 4 years ago

https://hg.sr.ht/~breakfastquay/rubberband/browse/CHANGELOG?rev=tip

Fix incorrect numbering of pitch speed/quality flags in the auxiliary C wrapper header. The effect of this was that code using the C wrapper that intended to select the higher-quality pitch-shift mode was actually choosing the higher-speed mode, and vice versa. (The third mode - high-consistency, commonly used in real-time applications - was correct.) Thanks to Michael Bradshaw for reporting this.

@DoctorBertalis try latest PulseEffects with the new rubberband 1.9 to see if this fix helps in quality improvement.

DoctorBertalis commented 4 years ago

Unfortunately, I am currently unable to test the quality of the pitch shift plug-in in PE 4.8.0 because one of my HDDs broke and took openSUSE Tumbleweed with it. On my current system, openSUSE Leap 15.2 PE 4.8.0 does not work from flatpack and is also missing from the repos. I have to wait for it to appear there, or think about installing Tumbleweed, but so far I can't do it...

Digitalone1 commented 4 years ago

You can still test with an old version of PulseEffects, but be sure to have new rubberband 1.9 version.

DoctorBertalis commented 3 years ago

Well, now the Rubberband 1.9.0 update has come to my Fedora 33 (not yet on openSUSE). I am writing about the plugin test results. I cannot say that the quality has improved significantly. The following checks have been performed and the quality has been compared with foobar2000 when applicable:

  1. Play a sinusoidal WAV file;
  2. Play classical music from WAV file and from YouTube;
  3. Reproduction of children's cartoon;
  4. Reproduction of pink noise from WAV file. What can I say, the sinusoidal is reproduced as before. In some cases, the classics go more or less well, but I can still hear artifacts in places. The cartoon is probably going about the same... The most interesting part of the test is pink noise, this is the most difficult test. Foobar plays it with an accuracy of 95-98%, and in this case the Rubberband gives a not very good result. It should be noted that the Rubberband loads the CPU by 15%, and foobar by 30%, and here it is clear that Fobar has a more complex algorithm...
Digitalone1 commented 3 years ago

When I was checking at the time, I noticed that foobar was also using something called "tempo". Maybe the file processed is modified in its speed reproduction and this could explain the difference.

But I'm not sure, try to measure the time in foobar. At least in mpv the rubberband library is used in display resample mode to modify the audio speed to sync the frames to monitor refresh rate.

If this is the reason, PulseEffects can't do nothing since it processes live sources and modifying the speed reproduction will lead to desync.

DoctorBertalis commented 3 years ago

Now I checked the sound in the foobar, set the pitch shift in its plug-in to "- 6 semitones" in order to accurately catch the tempo change, if any. But no, the tempo does not change and the timing of the tracks also does not change.

Digitalone1 commented 3 years ago

There should be something foobar does in addition, but I'm afraid we can't do nothing.

@wwmm what do you think?

wwmm commented 3 years ago

wwmm what do you think?

I don't have a good idea to solve this. I am not familiar with how pitch shifting algorithms work. Both from the user as from the developer point o view. Maybe something in the rubberband ladspa plugin is not write. Or maybe it has to be integrated in a different way than the one I am currently doing.

wwmm commented 3 years ago

One way to test the rubber band plugin outside of pulseeffects is this:

gst-launch-1.0 pulsesrc ! ladspa-ladspa-rubberband-so-rubberband-pitchshifter-stereo octaves=1 ! pulsesink

pulsesrc will record from the default source device. using Pavucontrol it is possible to make it record from somewhere else. @DoctorBertalis you could try the following after closing PulseEffects with the command pulseeffects -q:

pacmd load-module module-null-sink sink_name=test sink_properties=device.description=test

This will load a virtual output device. Using Pavucontrol make the audio player use it instead of the sound card. And make the gst-launch-1.0 command above record from this test sink monitor. If the sound is still bad the problem is inside rubberband and not in how its plugin is being used in PulseEffects.

wwmm commented 3 years ago

If the sound is still bad the problem is inside rubberband and not in how its plugin is being used in PulseEffects.

And in the case the only thing I could do is trying other plugin. I do not have the knowledge to write a pitch plugin myself or to know what other kind of plugin I would need to use in combination with rubberband to improve audio.

DoctorBertalis commented 3 years ago

Well, now everything has become clear - the reason is the Rubberband. I figured it out, having tested it according to your method, only the "octaves=1" parameter I changed to "cents=-32". Yes, the same artifacts...

wwmm commented 3 years ago

Ok. That method can be used to test another plugins too. To test the tap plugin you mentioned in your first post you can do:

gst-launch-1.0 pulsesrc ! ladspa-tap-pitch-so-tap-pitch semitone-shift=1 ! pulsesink

you can read the plugins otions with the command gst-inspect-1.0 ladspa-tap-pitch-so-tap-pitch. And to find out which tap plugins were detected by GStreamer with the command gst-inspect-1.0 | grep -i tap. In case you test other libraries replace tap by what is appropriate.

If possible I would like not having to use the tap pitch plugin because it is a 1 channel plugin. Using 1 channel plugins in a stereo pipeline require much more work and make the reordering of plugins on the fly a lot more tricky.

DoctorBertalis commented 3 years ago

Today I was convinced that the Rubber Band itself does not introduce noticeable distortions when testing it from the command line when processing a single file, but when I tested its work both with PulseEffects and outside it, using a virtual sound device, then there were artifacts. Apparently, there is some nuance of integrating the Rubber Band into the Linux system in the case of streaming signal conversion...

wwmm commented 3 years ago

Rubber Band itself does not introduce noticeable distortions when testing it from the command line when processing a single file

Interesting. Did you process the file through gst-launch-1.0 or using something else?

DoctorBertalis commented 3 years ago

I ran this commands after renaming the original file to "input.wav" and placing it in the root of my home directory.

rubberband -p -0.32 -c 0 /home/bertalis/input.wav output.wav

rubberband -p -0.32 /home/bertalis/input.wav output.wav

In both cases, the result was the same, the quality of the output files was good!

wwmm commented 3 years ago

Ok. So either the rubberband executable is doing some kind of two pass process or its ladspa plugin has a bug. I hope it is the last option because two pass processing can not be done on live streams.

wwmm commented 3 years ago

I am busy this week but When I have time I will take a look at its ladspa plugin source code. If it is just a wrapper around the rubberband library calls it shouldn't be too hard to write a native GStreamer plugin for it.

DoctorBertalis commented 3 years ago

Yes, it seems there were two passes in the console output...

wwmm commented 3 years ago

Yes, it seems there were two passes in the console output...

That is concerning... I hope that the increased quality is not a result of the two pass analysis...

DoctorBertalis commented 3 years ago

Good day! Today I will write about how I tested RubberBand in the Reaper program ... I did the test on different WAV tracks, such as classical music and a sine wave of 350 Hz. The procedure is as follows:

  1. I import the wav-file in the Reaper program.
  2. To the left of the track I press the FX button and select in the category Pitch Correction VSТ: ReaPitch (Cockos) and press Add.
  3. Next, the settings dialog opens, and I do this:
  4. I set the Shift (Cents) parameter to -32.
  5. I choose the Algorithm Rubber Band Library.
  6. I set the Parameter in the drop-down list as follows (choose two of them) - Pitch Mode -> HighQ, (not Fast or Consistent!) And another parameter Transients -> Mixed (or may be Transients -> Smooth).
  7. Then I turn on the file playback (without rendering) and get a very good result! But if you select Parameter by default in step 6, or select Pitch Mode -> Fast or Consistent, leaving the parameter Transients -> Crisp, then in this case the very artifacts that I wrote about earlier appear! I think that maybe something similar is happening in PulseEffects, and maybe something else needs to be specified in the parameters of the Rubber Band Library when using it in PulseEffects ...
wwmm commented 3 years ago

maybe something else needs to be specified in the parameters of the Rubber Band Library when using it in PulseEffects

In EasyEffects I am using the rubberband library directly instead of calling its LADSPA plugins throught a GStreamer wrapper. So now I have more freedom to play with more settings.

Digitalone1 commented 3 years ago

Rubberbands has three modes:

https://github.com/breakfastquay/rubberband/blob/2472213d731527a2fadbc56c83a8490ed7e3d78d/rubberband/RubberBandStretcher.h#L261

  1. HighSpeed
  2. HighQuality
  3. HighConsistency

We only switch between 1 and 3, that's why the processed sound can't reach the highest quality (but just at HighConsistency my CPU can't get octaves above +1).

There is also the Transient option that we do not expose in our UI.

https://github.com/breakfastquay/rubberband/blob/2472213d731527a2fadbc56c83a8490ed7e3d78d/rubberband/RubberBandStretcher.h#L140

  1. Crisp
  2. Mixed
  3. Smooth
DoctorBertalis commented 3 years ago

Yes, I understand ... I would really like, if possible, that the UI has more customization options for rubberband so that the person who knows these subtleties can customize this plugin the way he needs ... I am currently working on openSUSE Tumbleweed and have EasyEffects 6.1.3 so far, but that is already good! So I'll wait for updates!..

Digitalone1 commented 3 years ago

It could be done. Maybe in the next 6.1.5 release.

wwmm commented 3 years ago

I see no problems in offering more configuration options. I did not do that before for lack of time. But at this moment I am more concerned about the crashes that we are having with the Rubberband library that only seem to happen in specific cases and systems. I am not sure about how to debug this yet...

Digitalone1 commented 3 years ago

@wwmm Did you read this? Could it be related to crashes somehow?

Multiple instances of RubberBandStretcher may be created and used in separate threads concurrently. However, for any single instance of RubberBandStretcher, you may not call process() more than once concurrently, and you may not change the time or pitch ratio while a process() call is being executed (if the stretcher was created in "real-time mode"; in "offline mode" you can't change the ratios during use anyway).

So you can run process() in its own thread if you like, but if you want to change ratios dynamically from a different thread, you will need some form of mutex in your code. Changing the time or pitch ratio is real-time safe except in extreme circumstances, so for most applications that may change these dynamically it probably makes most sense to do so from the same thread as calls process(), even if that is a real-time thread.

wwmm commented 2 years ago

you may not call process() more than once concurrently

It is called only once for each iteration of PipeWire's realtime thread. So that is not the problem.

and you may not change the time or pitch ratio while a process() call is being executed

As we already have a mutex there I do not think this could be happening.

Whatever it is the problem its cause is very subtle I have not been able to reproduce it yet on my computer. And the users that had it did not even try to change one of he parameters. EasyEffects crashed on startup just because the pitch plugin was in the pipeline.

DoctorBertalis commented 2 years ago

I found the rubberband plugin with a version number of only 1.8.2 on my system, while its developer has currently released version 2.0.0. It depends on openSUSE in my case ... On other systems, the version number may be different. Perhaps this affects the stability of EasyEffects in general ...

vchernin commented 2 years ago

Flatpak currently has rubberband-1.9.2, perhaps 2.0.0 is better and I should update it. But I haven't really tested the new version much.

Digitalone1 commented 2 years ago

I see Rubberband is made in C++ and it raises some exceptions, especially in resampler, allocator and FFT code. Should we catch one of those?

wwmm commented 2 years ago

I see Rubberband is made in C++ and it raises some exceptions, especially in resampler, allocator and FFT code. Should we catch one of those?

Ideally rubberband should not be throwing exceptions when running in realtime mode... But if there is a chance they do such a thing it is better to catch them.

Digitalone1 commented 2 years ago

Ideally rubberband should not be throwing exceptions when running in realtime mode... But if there is a chance they do such a thing it is better to catch them.

I searched the throw keyword in their code and they are disabled only at compile time.

https://github.com/breakfastquay/rubberband/blob/2472213d731527a2fadbc56c83a8490ed7e3d78d/src/dsp/FFT.cpp#L2413

But not everywhere:

https://github.com/breakfastquay/rubberband/blob/2472213d731527a2fadbc56c83a8490ed7e3d78d/src/dsp/Resampler.cpp#L829

wwmm commented 2 years ago

I see. As the source of the crash only some users are having is unknown at this moment I think it is worth trying to catch an eventual exception. And probably using util::warning so we can know such a thing is possible even when not running in debug mode.

Digitalone1 commented 2 years ago

I should have the time to work on this in the current week, so finally we will have the opportunity to close this issue.

I make a recap of what will be changed.

  1. Add Transient combo box to choose between Crisp, Mixed and Smooth modes.
  2. Add Detector combo box to choose between Compound, Percussive and Soft modes.
  3. Add Phase combo box to choose between Laminar and Independent modes.
  4. Remove Preserve Formant toggle button and add a Formant combo box to choose between Shifted and Preserved modes.
  5. Remove Faster toggle button and add an Operating Mode combo box to choose between High Speed, High Quality and High Consistency . This is relative to the topic of the present ticket and High Quality will fix it. Besides I think that High Speed should be the default because this plugin is very CPU intensive (I can't use smoothly some modes in High Consistency on my system, High Quality could be even worse). So the default mode should provide usability to almost all users, then if they want to use more resources and if their system can handle it, a better mode can be set.
wwmm commented 2 years ago

Ok. Sounds good.

Digitalone1 commented 2 years ago

Schermata del 2022-03-12 12-39-20

Digitalone1 commented 2 years ago

It's working. Unfortunately I can almost always reproduce a crash at octave -3. Not always, but changing parameters at octave -3 is often crashing. No big hints from command line and gdb bt. Only sigkill and the program is closed. I'll try to investigate.

Digitalone1 commented 2 years ago

@wwmm Any suggestion where I should add a try statement to catch the following exceptions?

https://github.com/breakfastquay/rubberband/blob/ce3f17acb1305f1454d6a4f4266c1bc12ab5ded6/src/dsp/Resampler.cpp#L829 https://github.com/breakfastquay/rubberband/blob/2472213d731527a2fadbc56c83a8490ed7e3d78d/src/dsp/Resampler.cpp#L902 https://github.com/breakfastquay/rubberband/blob/2472213d731527a2fadbc56c83a8490ed7e3d78d/src/dsp/Resampler.cpp#L942

Digitalone1 commented 2 years ago

So I made different tests. I can reproduce the crash setting semitones from 0 to -2, then octaves from 0 to -3 and finally mode from HighSpeed to HighQuality.

EasyEffects crashes instantly after changing the quality, but it's related to octaves at -3 because it does not happen with different values. Anyway I don't exclude that the crash could happen with another combination of scale parameters.

I can clearly see that my system is suffering on processing at octave -3 because it's not too powerful, it may be related to CPU insufficiency, but there's no way to prevent the crash. I tried to test adding debug strings and it crashes right before entering in set setPitchOption, so the crash should happen inside that function, but it does not raise any exception.

I even tried to wrap every strecther call in a try catch statement, but nothing came out from console. Neither gdb is of any help.

The strange thing is that the whole program is killed. I don't see any error, neither a segmentation fault.

Anyway I will make the merge request, please @wwmm try to test on your system, it may also not happen to you because your CPU is more powerful.

wwmm commented 2 years ago

So I made different tests. I can reproduce the crash setting semitones from 0 to -2, then octaves from 0 to -3 and finally mode from HighSpeed to HighQuality.

@Digitalone1 there is no crash on my system when applying these settings. But I do notice a ridiculously high increase in CPU usage at one of my CPU cores. Nothing it can't handle but it is definitely a lot higher than any of the other plugins. Looking at the output of sudo perf top -p pid while EasyEffects was running I've noticed that calls related to rubberband were at the top Screenshot from 2022-03-12 12-40-18

The only explanation that comes to my mind now is that with these settings Rubberband becomes so heavy on CPU that there is a chance that the maximum time allowed for a realtime thread is exceeded and the system kills EasyEffects in self defense. I do not remember the value now but there is a maximum time PipeWire sets in its realtime thread. If the CPU does not finish the work in time the process is killed.

Digitalone1 commented 2 years ago

The reason could be Rubberband using only one thread in realtime mode. Isn't there a way to catch this and avoid the crash?

@wwmm Did you check the other options? It seems that Phase, Transient and Detector are not producing substantial differences while switching their parameters. I hear much differences only switching Formant and Mode. Anyway it's better to have them all in the UI.

wwmm commented 2 years ago

Isn't there a way to catch this and avoid the crash?

If the crash is happening because the system is killing us for giving too much work for a realtime thread no amount of try/catch will help.

Did you check the other options? It seems that Phase, Transient and Detector are not producing substantial differences while switching their parameters. I hear much differences only switching Formant and Mode. Anyway it's better to have them all in the UI.

I did a quick test but I did not notice much difference. In any case it is not going to harm to keep them there.