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

Configure input capture channels for a layer #254

Closed
wants to merge 1 commit into from

Conversation

jypma
Copy link
Collaborator

@jypma jypma commented Jun 16, 2020

This commit adds the ability to configure which capture inputs route to
the top input on an effects layer.

Internally, this is stored in a "audio_in" dict that precisely specifies
which capture inputs are connected to which LV2 input(s).

To the user, a simplified list of mono / stereo choices are provided,
depending on the number of capture inputs and number of LV2 inputs.

Supersedes #253 and implements the "simple UI" idea from there.

TODO

  • Load and save audio_in in snapshot state, once agreed on the format
  • Remove extraneous debugging lines and exception catching

@@ -38,7 +38,7 @@
# Configure logging
#-------------------------------------------------------------------------------

log_level = logging.WARNING
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take this out before merging :-)


try:
Copy link
Collaborator Author

@jypma jypma Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the try: should also be removed here (was for debugging only).

However, I found that if audio_autoconnect() throws an exception, two problems occur:

  • The exception doesn't make its way into the systemd journal logs (perhaps it's eaten or delayed?)
  • The mutex is never released so the UI freezes

That's for a different PR of course; any ideas on how to approach this?

@jofemodo
Copy link
Member

There is only ONE input (root) plugin on every FX layer, so the only info we need to store is what capture channels are "enabled" for this layer: 1, 2, or both. That's all. The autoconnector will do its task and will connect everything accordingly to the number of input channels on the root input plugin.

@jypma
Copy link
Collaborator Author

jypma commented Jun 17, 2020

The decision to make the data model store routing decisions, rather than make them dynamically each time, was made purposefully. This enables us to evolve the UI separately from the data model, e.g. to implement more complex routing cases later. Routing logic need not be hidden from the user completely; a Zynthian user can be expected to know what "Mono" or "Stereo" means.

Please see the following screen shots of this branch:
mono
stereo
These are very close to your suggestion, yet more explicitly describe what is being routed.

If instead a multi-select is presented, and 4 capture inputs are available, it also becomes less obvious to the user that only 2 'X'es should be selected.

A second problem is that auto-detecting whether an LV2 is mono or stereo is hard to do reliably. Take for example the LSP Sidechain Mono Compressor (not currently in zynthian, but it's open source and popular). It has 2 audio inputs, but is not stereo.

This commit adds the ability to configure which capture inputs route to
the top input on an effects layer.

Internally, this is stored in a "audio_in" dict that precisely specifies
which capture inputs are connected to which LV2 input(s).

To the user, a simplified list of mono / stereo choices are provided,
depending on the number of capture inputs and number of LV2 inputs.
@jofemodo
Copy link
Member

Hi @jypma !

Your solution is good. I like it (the first one too!). The problem with this one is that it doesn't scale if we decide to add support for more than 2 audio channels. And then we would need to add code to migrate snapshots ...

I would prefer a more "raw" approach, with the minimal coding complexity, keeping the same approach it's been used on the "Audio Out" routing. Having different approaches will create confusion ...

Regarding the problem of detecting whether an LV2 is mono or stereo, it's a marginal problem and it could be solved by using a simple "gain" element as root. Anyway, i think this should be addressed by LV2 specification...

@jofemodo
Copy link
Member

OK! Please, don't work on this until i release a basic implementation with the same approach than the current output routing. After that, we can open the discussion and see if more functionality is wanted/desirable and, in such a case, improve both routing tools (output and capture) at once.

@jypma
Copy link
Collaborator Author

jypma commented Jun 19, 2020

Sure thing! Thanks a lot for the discussion so far.

I actually did realize there's a case that my current "simple" setup doesn't cover: combining (adding) two capture channels into one mono plugin. The "X-es" approach would more naturally allow for that.

Though, how would we handle the UX of having 4 capture channels listed, but limiting the selection to only 2 of them?

Tying in to the coding discussion we were trying to get started, would this be a case to start experimenting with unit tests? zynthian_autoconnect.py is now going to handle lots of different cases, with already up to 5 indentation layers of looping/switching. Capturing a few expected use cases as tests could both help against regression and serve as documentation.

@jofemodo
Copy link
Member

Sure thing! Thanks a lot for the discussion so far.

It was a pleasure ... and believe me, you make me think about future approaches on zynthian's routing. You put good ideas over the desk ... ;-)

I actually did realize there's a case that my current "simple" setup doesn't cover: combining (adding) two capture channels into one mono plugin. The "X-es" approach would more naturally allow for that.

BTW, the basic "raw" implementation is commited. Perhaps you find interesting the autoconnect fragment that manage the capture connections. I tried to add support for more than 2 capture channels. And also extended this support to system output connections. But it must be tested with multi-channel cards...

#Get System Capture ports => jack output ports!!

Though, how would we handle the UX of having 4 capture channels listed, but limiting the selection to only 2 of them?

I didn't. If you select more than 2 channels, they will be down-mixed. But i limited the number of input ports on root plugins that will be connected to 2 ... so ...

  • if you have 4 channels and select all of them:

    • If the root FX-plugin is stereo: capture_1 & capture_3 are connected to port_1, capture_2 & capture_4 are connected to port_2.
    • If the root FX-plugin is mono: capture_1, capture_2, capture_3 & capture_4 are connected to port_1.
  • if you have 4 channels and select 3 of them:

    • If the root FX-plugin is stereo: capture_1 & capture_3 are connected to port_1, capture_2 is connected to port_2. OK! This is a little bit bizarre ... same than the selection!! Bizarre selection, bizarre result. But creative people love this kind of features!! ;-D
    • If the root FX-plugin is mono: capture_1, capture_2 & capture_3 are connected to port_1.

Tying in to the coding discussion we were trying to get started, would this be a case to start experimenting with unit tests? zynthian_autoconnect.py is now going to handle lots of different cases, with already up to 5 indentation layers of looping/switching. Capturing a few expected use cases as tests could both help against regression and serve as documentation.

I really like the idea of unit tests, but it takes a lot of time to create and maintain!! Of course, you and everybody are very very welcome to add unit tests.

I've already added some unit tests to the MIDI-filter rule parser:

Perhaps this could be used as example. The "unittest" python module is really simple & clean to use ...

The best!

@jofemodo jofemodo closed this Jun 20, 2020
@jofemodo jofemodo deleted the jypma_input_config2 branch March 15, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants