zynthian / zynthian-issue-tracking

Centralized Issue Tracking for Zynthian Project
https://github.com/orgs/zynthian/projects/1
11 stars 3 forks source link

Implement a "Master FX chain" #641

Closed jofemodo closed 2 years ago

jofemodo commented 2 years ago

Is your feature request related to a problem? Please describe. When using several layers, a global/master audio FX chain is very convenient. It would increase usability, while avoiding duplication and wasting of CPU resources.

Describe the solution you'd like Initially we can create a "layer" on MIDI channel 16, associated to mixer's Main strip. It would work like a normal "FX-layer" except most of layer options would be disabled.

Describe alternatives you've considered When "chainification" is done, it would be implemented as a "chain" object and no MIDI channel association would be needed, etc.

jofemodo commented 2 years ago

An initial implementation can be tested in the masterfx development branch:

https://github.com/zynthian/zynthian-ui/compare/masterfx?expand=1

Adding FX send/receive ports to zynmixer will allow having Dry/Wet control from the mixer although this is not a big priority because many audio FX-plugins already have mix or dry/wet controls. In a perfect world, we should have per-strip FX-send, but before over-complicating things, let's play with the current implementation and get feedback from the community.

Enjoy!

riban-bw commented 2 years ago

I don't think we necessarily need a dry/wet mix. Most standard mixing desks don't offer that on their insert points which is what I see these as. The input chains each have an insert for audio effects between the sound source and the fader. The main mix bus will have an insert between the mix bus and the main fader. If there is a need to adjust the send gain then this can be done with other effects in the chain or the input gain control or the synth volume. If someone wants a dry/wet mix then they may be able to provide this within the effect or maybe with multiple chains.

I will add a main mix send output and return pair (left/right) of audio ports and normalise them together if nothing is routed to the return so that it will work out of the box... This may take a few days due to other commitments.

jofemodo commented 2 years ago

OK! I will be awaiting for your comments about the current implementation.

jofemodo commented 2 years ago

Merged! Snapshot workflow now feels much better. Thanks!

jofemodo commented 2 years ago

I just merged the latest commits from testing to masterfx (aka, the snapshot workflow improvements)

jofemodo commented 2 years ago

If no dry/wet control is desired, i don't think you need to add the send/receive ports to the zynmixer. Currently it works pretty nice without them. Please test.

riban-bw commented 2 years ago

We want a final level fader post-effects. I would like the mix bus to feed an output any the return to feed the main fader which in turn feeds the main output.

jofemodo commented 2 years ago

OK! If i understand well, all channels are mixed according to its fader levels, this mix is sent to Master FX and returned for the post-FX final fader level, then sent to system output. From the GUI view, no changes, but functionally, the last fader acts after master FX instead before. Am i right?

Regards,

riban-bw commented 2 years ago

Correct! I will add this feature soon...

riban-bw commented 2 years ago

UI does not work as expected:

I fixed a regression with CUIA control of chain selection today in the masterfx branch. I will leave the rest of these issues to @jogemodo and I will look at implementing the changes to the zynmixer library, probably tomorrow.

jofemodo commented 2 years ago

Hi @riban-bw !

Regarding this:

"After CLEAN ALL, bold SELECT does not show context menu so can't toggle mono (for example)"

I understand what you mean and you are right, but sorry, it's too complex to implement with the current state of things. Don't forget this is a dirty workaraound, not a fine implementation. It would be improved in the "Chainification".

Regards,

riban-bw commented 2 years ago

I have pushed a commit to the masterfx branch that adds the main bus send and return insert point. Send is normalised to return and broken away when either channel of return is routed. I think it works well. You will need to insert main mix effects between the send and return:

zynmixer:send_a -> effects -> zynmixer:return_a zynmixer:send_b -> effects -> zynmixer:return_b

I look forward to seeing it all working when I awake... good night!

jofemodo commented 2 years ago

Curiosity: At what point do you measure the output for the meter?

jofemodo commented 2 years ago

Could we have a "bypass" flag for avoiding extra processing when master FX-chain is empty?

riban-bw commented 2 years ago

Meters are post-fader. For each channel they are the (dampened) level after all mixer controls before contributing to the main mix which is now actually summed onto the main send. The main send or the main return (depending on routing to main return ports) is then copied to the main output with fader level adjustment. The main meter is effectively what is on the main output, post main fader.

I will check for optimisations but I don't think there is significant extra processing within the mixer. There is one extra copy of the audio data (from send or return to main fader) which is cheap. There is no extra processing.

I did a small tweak just now to allow breaking away each main return individually, e.g. if you insert an effect in the left channel then the right channel remains normalised from its corresponding send.

I am still seeing main effects being inserted between system:capture and system:playback. This needs to change to route beween mixer:send and mixer:return.

image

I am also seeing audio channels not routing. See screenshot above for example where output_a is not routed.

riban-bw commented 2 years ago

Log error: ERROR:zynthian_autoconnect.audio_autoconnect: Error connecting 'zynmixer:output_a' -> 'GxEcho-Stereo-00:out' (-1) suggests that autoconnect is trying to connect an output to an output.

I see the problem. master_fxchain_ports is not getting only audio inputs. The following patch to audio_autoconnect will work but you have to restore the old behaviour of routing mixer main output to system playback (and headphones) and route master fxchain to mixer return insteada of system playback. You also need to stop system capture from getting routed to the master fxchain.

    if master_fxchain_layer:
        master_fxchain_ports = jclient.get_ports(master_fxchain_layer.get_audio_jackname(), is_audio=True, is_input=True)
        if len(master_fxchain_ports):
            try:
                connect_only("zynmixer:send_a", master_fxchain_ports[0])
                connect_only("zynmixer:send_b", master_fxchain_ports[1])
            except Exception as e:
                logging.error(e)

[Edit] Note that I don't route both ports of the master effect chain to allow for individual routing of channels but you may want / need to reinstate your code to route both if a mono effect is inserted. (We may need to think about more configurable audio routing options.)

jofemodo commented 2 years ago

I think it's working quite nicely now. You probably have to empty and re-create the Master FX chain to see that now it's not connected to capture by default, although it still allow to do it explicitly.

BTW, i also modified the audio recorder so MPlayer get connected to zynmixer's return by default. There is a problem when "looping" a recording. It seems that MPlayer reset the connection and reconnect to system output. I will try to fix it, although i can't imagine an easy solution except replacing MPlayer by something lighter and best suited for this ;-)

Regards,

riban-bw commented 2 years ago

I will take a look soon. I don't think MPlayer should connect to mixer return. This will disconnect all other audio which is proabably not desired behaviour. I can add another mixer channel for audio file player which was the plan. I was working on a replacement for MPlayer but that got shelved when I got busy with other stuff. I can pull that out again... one day!

riban-bw commented 2 years ago

There was a bug when headphones are enabled. I have pushed a fix.

You don't need to maintain a route between send and return. This is normalised internally within the mixer so if nothing is routed to return then the send is automatically connected internally.

There is another bug: Add two effects in parallel to the main bus and the second's inputs are not routed.

MPlayer is still routed to system out (not return) and does not connect to headphones. (It actually connects briefly then clears which is similar but different to the bug I reported early about loop play.) I see these two errors, first when audio starts and second when audio ends:

Apr 27 03:08:03 zynthian startx[4410]: ERROR:zynthian_autoconnect.audio_autoconnect_mplayer: Connecting Mplayer ports to ['system:playback_1', 'system:playback_2']
Apr 27 03:08:13 zynthian startx[4410]: Terminal type `unknown' is not defined.

I really don't like this inconsistency:

I know it is there and what the logic is but it is screwing with my head every time. I really don't like that I need to think about what just happened when I clicked a button rather than know what will happen when I click it. It could be releated to the lack of indication that the behaviour will be different but consistency where possible is desirable. I know that you said this is a temporary stage and that initial lack of layer means no layer option screen but we could catch that and show a reduced options list with just:

jofemodo commented 2 years ago

It's not an inconsistency, dear @riban-bw !

Anyway, the solution you propose is nicer, so feel free to implement it ;-)

Regards!

riban-bw commented 2 years ago

Hey hey! This is looking really good now. I added a context menu (Main Chain Options) with just mono and add audio effect accessed via bold SELECT. It feels far more consistent to me now. I added and fixed some issues today like muting mixer before removing a root chain and resetting mixer after removing root chain.

We are starting to think alike. I was going to change references to "Layer" to "Chain" and you have already done it. I did change "Master" to "Main" (not for Master MIDI).

I think this ticket is done! I shall leave it to @jofemodo to merge into testing.

jofemodo commented 2 years ago

It's merged now! Yessssssssssssssssssssssssssss!

jofemodo commented 2 years ago

I will generate a testing image in the next hours and after that we could invite people to help us with testing. And by the end of this week we could think of releasing the staging-2205. Do you agree?

riban-bw commented 2 years ago

I probably won't grab an image until after the weekend. There may be a few more fixes to do. We definitely need to fix the javl.gtk issue but can now concentrate on testing branch.