wwmm / easyeffects

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

Incorrect behaviour on effect re-enablement #2867

Open LebedevRI opened 9 months ago

LebedevRI commented 9 months ago

EasyEffects Version

7.1.3

What package are you using?

Other (specify below)

Distribution

debian unstable

Describe the bug

  1. Enable e.g. compressor effect
  2. Start some audio playback
  3. Let it play just for a bit (don't stop it yet)
  4. Disable compressor effect while the audio is still playing
  5. Stop audio playback. No sound is being played!
  6. Re-enable compressor effect. Suddenly, there is a short burst of audio.

Expected Behavior

What happens is that some effects may store internal state, and disabling-reenabling the effect does not account for that. It is not okay. This is dangerous.

It sounds like an optimization to avoid destroying and recreating effect nodes in PW graph, and if there is no native way to tell PW to flush such buffers, i would suggest to remove that optimization.

(i suspect this is a duplicate.)

Debug Log

No response

Additional Information

No response

wwmm commented 9 months ago

It sounds like an optimization to avoid destroying and recreating effect nodes in PW graph, and if there is no native way to tell PW to flush such buffers, i would suggest to remove that optimization.

It was not done for the sake of the PW graph. In the past I did exactly what you are suggesting but for different reasons. The problem is that destroying the plugins objects and recreating them was causing very noticeable crackling for many people that came here to report bugs. So if I do what you are suggesting I will only make that kind of bug report to come back.

This is dangerous.

There is a big amount of exaggeration in this statement. How noticeable are the side effects of both the current as well as the old behavior depend a lot on the user hardware. Both approaches never caused problems on my hardware for example.

As far as I am aware there isn't a good way to handle this problem at the moment. Third party plugins may have some data internally at any point. If I do not destroy them this data will be played at a later point. But If I do annoying crackling can be heard on some setups.

wwmm commented 9 months ago

Disable compressor effect while the audio is still playing

By "disable" do you mean using the bypass button or removing the plugin from the pipeline?

LebedevRI commented 9 months ago

image

This is dangerous.

There is a big amount of exaggeration in this statement. How noticeable are the side effects of both the current as well as the old behavior depend a lot on the user hardware.

Eh. It also strongly depends on the input and rest of the pipeline. For example, consider situation of loud input, that was getting handled by the limiter, but said limiter was also disabled before the compressor was reenabled.

wwmm commented 9 months ago

I see. Honestly I do not know how to solve this without causing another problems. For example we could check the bypass state and decide to free some plugin internal libraries objects like the ones from LV2 plugins. But depending on how the plugin is configured toggling the bypass could potentially also lead to unexpected jumps in audio levels.

For example consider the equalizer in FIR mode or any other plugin with large lookahead or that adds large latency because of buffered data or similar reasons. Destroying the internal state means starting from the beginning. Depending what what the plugin is doing to the sound a volume jump may happen.

wwmm commented 9 months ago

Destroying the internal state means starting from the beginning. Depending what what the plugin is doing to the sound a volume jump may happen.

It wouldn't be a problem in your example because the pipeline would have stopped for some time. So it would be similar to just open EasyEffects from a clean state. But the same bypass could be used on the fly with the pipeline running something. In this case it could be a problem to reset the state.

violetmage commented 9 months ago

Shouldn't it be possible to just feed the plug-in a silent audio stream and null sink its output, to flush any buffer(s) the plug-in might have?

wwmm commented 9 months ago

Shouldn't it be possible to just feed the plug-in a silent audio stream and null sink its output, to flush any buffer(s) the plug-in might have?

This could also cause jumps in level in a active pipeline like I talked about before. But let's forget about this for now. Differently from GStreamer there isn't a "pipeline object" in PipeWire. Just a bunch of totally independently filters that happens to be linked. PipeWire is in total control of the filter state. We define a callback for each plugin and at each iteration we handle what PipeWire gives us when it calls the callback. If this is not done right we can easily enter on collision course with the server.

LebedevRI commented 6 months ago

What are your general thoughts on "optional" complexity, in the sense, how about hiding this workaround for

The problem is that destroying the plugins objects and recreating them was causing very noticeable crackling for many people that came here to report bugs.

behind an option in settings, that would switch between the current mode (toggling pass-through for plugins) vs the remove-and-recreate mode? I'm looking at pw-top, and i'm pretty consistently seeing xruns for some of disabled effects, but nothing else.

wwmm commented 6 months ago

What are your general thoughts on "optional" complexity, in the sense, how about hiding this workaround for

Technically speaking it isn't a workaround but the expected behavior for LV2 plugins. By recreating their instance on every new latency value we were the ones violating the standard. But I guess it should be possible to put this behind a option that disables the recreation by default.

I'm looking at pw-top, and i'm pretty consistently seeing xruns for some of disabled effects, but nothing else.

Disabled effects are removed from the pipeline. So whateer pw-top is reporting about them do not matter. But if it is for bypassed plugins it is different because they are still in the pipeline in passthrough mode. But it is weird that a plugin in passthrough mode is causing xruns. They are only copying data from input directly to output like the spectrum and the output level meter do.

LebedevRI commented 6 months ago

I'm looking at pw-top, and i'm pretty consistently seeing xruns for some of disabled effects, but nothing else.

Disabled effects are removed from the pipeline.

qpwgraph seems to not agree with that statement:

image

wwmm commented 6 months ago

qpwgraph seems to not agree with that statement:

Oops. When I said disabled I was actually thinking about deleting them from the pipeline. The disable button at the side of the plugin name just puts them in bypass mode.

LebedevRI commented 6 months ago

Ah, i see.

What are your general thoughts on "optional" complexity, in the sense, how about hiding this workaround for

Technically speaking it isn't a workaround but the expected behavior for LV2 plugins. By recreating their instance on every new latency value we were the ones violating the standard. But I guess it should be possible to put this behind a option that disables the recreation by default.

To be noted, i was really only talking about recreating their instances upon toggling the bypass mode. I don't have a strong opinion about also recreating them upon every latency value.

LebedevRI commented 5 months ago

PipeWire-native Filter Chain explicitly mentions this problem and implies that there is a native solution: https://gitlab.freedesktop.org/pipewire/pipewire/-/wikis/Filter-Chain#draining

Draining

When a filter chain becomes unused, it will pause. This will stop playback of all audio abruptly. In some cases like with long reverberation, you would want to let the filter continue until it is drained.

You can stop the filter from pausing by setting the node.pause-on-idle = false property in the playback and capture props like so:

            ...
            capture.props = {
                ...
                node.pause-on-idle = false
                ...
            }
            playback.props = {
                ...
                node.pause-on-idle = false
                ...
            }
            ...

The session manager will eventually pause/suspend the filter chain after a timeout. You can also set this timeout with session.suspend-timeout-seconds (0 to disable suspend of the filter).

But of course if the disabled EE filters are simply in bypass mode and never actually enter IDLE state, that drainage won't trigger, so they'd need to at least be unlinked from the graph, but perhaps not outright destroyed (although by that point, is there even much point in not just destroying them?)

wwmm commented 5 months ago

But of course if the disabled EE filters are simply in bypass mode and never actually enter IDLE state, that drainage won't trigger, so they'd need to at least be unlinked from the graph, but perhaps not outright destroyed (although by that point, is there even much point in not just destroying them?)

Something else to have in mind is that we implement our filters using pw_filter objects. But for some reason PipeWire developers decided to implement filter chain plugins through a combination of input and output streams like the ones created by audio players/recorders. That node.pause-on-idle property probably does even exist for pw_filter.

As far as I remember a pw_filter can also enter an idle state. But we have no control over when it does it.