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

Increase maximum number of LADSPA nodes #793

Closed
mawe42 opened this issue Mar 5, 2021 · 6 comments · Fixed by #795
Closed

Increase maximum number of LADSPA nodes #793

mawe42 opened this issue Mar 5, 2021 · 6 comments · Fixed by #795
Milestone

Comments

@mawe42
Copy link
Member

mawe42 commented Mar 5, 2021

Following up on https://lists.nongnu.org/archive/html/fluid-dev/2021-03/msg00000.html

The maximum number of nodes in the LADSPA system is currently hard-wired to 100. When using multi-channel outputs and/or many effects or buffers, this limit can be easily reached.

A quick solution would be to simply increase FLUID_LADSPA_MAX_NODES to a higher number, like 500 or 1000. That would increase static memory consumption of FluidSynth by 400 * sizeof(void*) or 900 * sizeof(void*).

Alternatively, we could change the way the nodes are being allocated and move to dynamic allocation and linked lists instead. I would be fine either way, leaning a bit towards simply increasing the limit. Any other thoughts?

@mawe42 mawe42 added bug and removed bug labels Mar 5, 2021
@jjceresa
Copy link
Collaborator

jjceresa commented Mar 5, 2021

I would be fine either way, leaning a bit towards simply increasing the limit.

Perhaps the maximum number of nodes could be simply set at LADSPA system creation ?. Then if the limit is reached, the user should destroy/recreate the LADSPA system with an adequate number of nodes ?. The shell command interface (or a setting) could provide this also ?.

@mawe42
Copy link
Member Author

mawe42 commented Mar 5, 2021

I would rather not burden the user with that choice. It would mean that users would need to understand or at least know about "nodes" and why there is a an upper limit. But that knowledge does not actually help them in reaching their goal, it's simply in their way. Nodes (and the limit placed on them) are an implementation detail, only people trying to understand or change the LADSPA code should need to know about them, in my opinion.

But thanks for that thought JJC, because I think it made up my mind about which way to go! As we can't realistically choose a limit that will not waste lots of memory and still ensure that people will never run in to it, going for dynamic allocation seems to be the way to go. I will draft a PR that handles those parts with linked lists.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 5, 2021

going for dynamic allocation seems to be the way to go.

I think you are right. As the user have to deal with other LADSPA intricacy , "nodes limit" should not boring them anymore.

@jjceresa
Copy link
Collaborator

jjceresa commented Mar 5, 2021

I will draft a PR that handles those parts with linked lists.

Even if linked lists is wasting 50% of memory it is still a good choice because it is far more easier to implement that using memory allocation/reallocation.

@mawe42
Copy link
Member Author

mawe42 commented Mar 5, 2021

Even if linked lists is wasting 50% of memory it is still a good choice because it is far more easier to implement that using memory allocation/reallocation.

I agree, linked lists are much easier. And I think they will be beneficial in other ways as well, as they allow easy ordering of nodes and therefore remove the need to keep three separate node lists for host, audio and other nodes.

derselbst added a commit that referenced this issue Mar 6, 2021
Quick solution for 2.1.8 only.
Addresses #793.
@derselbst
Copy link
Member

I've committed the quick solution on 2.1.x. For master I agree we should go with linked lists instead.

mawe42 added a commit to mawe42/fluidsynth that referenced this issue Mar 6, 2021
Removes the need for static allocation and prevents users from
running into our arbitrary node and effect limits.

Fixes FluidSynth#793
@derselbst derselbst added this to the 2.2 milestone Mar 8, 2021
jet2jet pushed a commit to jet2jet/fluidsynth-emscripten that referenced this issue May 27, 2021
Quick solution for 2.1.8 only.
Addresses FluidSynth#793.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants