nwhitehead / pyfluidsynth

Python bindings for FluidSynth
GNU Lesser General Public License v2.1
197 stars 56 forks source link

Some extensive changes and enhancements #21

Open SpotlightKid opened 5 years ago

SpotlightKid commented 5 years ago

This PR is more of a request for comment than an actual merge request.

I heavily extended and changed the module, but I tried to separate the commits into distinct units of functionality. But I also re-formatted and re-arranged the code and the docstrings and I wasn't able to separate these changes from the functional ones. So it might be difficult to cherry-pick from theses changes. But if you are interested in incorporating only some of these changes, I'll see what I can do. I'll also understand if these changes are too extensive to incorporate. I've started this fork mainly with the goal to have a special version of pyfluidsynth to incorporate into an application, but since this could be useful to others I wanted at least to open some kind of PR.

Enhancements & new features:

Changes:

Fixes:

EternityForest commented 4 years ago

Seems to need a fix to prevent an AttributeError with fluid_player_seek, otherwise pretty great work!:


#FS 1.0 doesn't have this.
try:
    fluid_player_seek = cfunc(
        'fluid_player_seek',
        c_int,
        ('player', c_void_p, 1),
        ('ticks', c_int, 1))
except AttributeError:
    pass
SpotlightKid commented 4 years ago

@EternityForest Thanks for the hint, I'll look into that.

I also have a couple of additions to my fork, which I need to sort out into separate commits.

EternityForest commented 4 years ago

Oh! One more thing!

In this section, I don't understand the "pointer" part. It seems to crash with some kind of "expected a cfunctype instance instead of a cfunctype"

# Fluid MIDI driver
new_fluid_midi_driver = cfunc(
    'new_fluid_midi_driver',
    c_void_p,
    ('settings', c_void_p, 1),
    ('handler', CFUNCTYPE(POINTER(c_int), c_void_p, c_void_p), 1),
    ('event_handler_data', c_void_p, 1))
delete_fluid_midi_driver = cfunc(
    'delete_fluid_midi_driver',
    None,
    ('driver', c_void_p, 1))

Changing to a plain int at least lets me call start() without a crash:

# Fluid MIDI driver
new_fluid_midi_driver = cfunc(
    'new_fluid_midi_driver',
    c_void_p,
    ('settings', c_void_p, 1),
    ('handler', CFUNCTYPE(c_int, c_void_p, c_void_p), 1),
    ('event_handler_data', c_void_p, 1))
delete_fluid_midi_driver = cfunc(
    'delete_fluid_midi_driver',
    None,
    ('driver', c_void_p, 1))

But I have not tested if it actually works.

cghawthorne commented 4 years ago

Just curious what the status is for this PR and related ones (e.g., #17) that fix FluidSynth 2 compatibility. I'm especially wondering because there haven't been any new commits to this repo since December, 2018. @nwhitehead do you have plans to update the project? Or is there a different fork I should switch to using? Thanks!

EternityForest commented 4 years ago

For now, I've been using a slightly tweaked fork and a custom wrapper, the standard API was just a lot lower level than I wanted to work with.

Although it does need a few dependancies like sf2utils to work correctly, it seems to be reliable with JACK on Linux, but I haven't really had time to get it working with anything else, or fully document.

https://github.com/EternityForest/scullery/tree/master/scullery

On Fri, Mar 27, 2020, 10:49 AM cghawthorne notifications@github.com wrote:

Just curious what the status is for this PR and related ones (e.g., #17 https://github.com/nwhitehead/pyfluidsynth/pull/17) that fix FluidSynth 2 compatibility. I'm especially wondering because there haven't been any new commits to this repo since December, 2018. @nwhitehead https://github.com/nwhitehead do you have plans to update the project? Or is there a different fork I should switch to using? Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nwhitehead/pyfluidsynth/pull/21#issuecomment-605151941, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFZCH67QA6HSS635ZWHEJ3RJTRJTANCNFSM4HYXK7NA .

SpotlightKid commented 4 years ago

I totally understand that @nwhitehead would be reluctant to merge these extensive changes without further discussion and probably some amendments.

The thing is, I have done some even more extensive changes to my fork, which I - unfortunately - haven't even comitted and pushed yet, because I wanted to break them up into smaller pieces. But I never got around to doing that and the project I needed pyfluidsynth for didn't go as planned, so my work on pyfluidsynth hasn't progressed since quite some time (June 2019).

Anyway, I just commited and pushed these changes into a new branch of my fork, in case anyone wants to have a look or build on it:

https://github.com/SpotlightKid/pyfluidsynth/tree/feature/more-refactoring

jmaibaum commented 4 years ago

@SpotlightKid , I have tried to use this PR on a Raspberry Pi (Raspbian Buster) in a Python 3 project. fluidsynth is still on version 1.1.11 in Raspbian Buster, which seems to be incompatible with your changes (with the code in #17 import fluidsynth was still possible):

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/pi/[snip]/env/lib/python3.7/site-packages/fluidsynth.py", line 669, in <module>
    ('ticks', c_int, 1))
  File "/home/pi/[snip]/env/lib/python3.7/site-packages/fluidsynth.py", line 89, in cfunc
    return CFUNCTYPE(result, *atypes)((name, _fl), tuple(aflags))
AttributeError: /usr/lib/arm-linux-gnueabihf/libfluidsynth.so.1: undefined symbol: fluid_player_seek

Just out of curiosity: Is compatibility to Fluidsynth 1.x one of your goals with your fork or not?

SpotlightKid commented 4 years ago

Is compatibility to Fluidsynth 1.x one of your goals with your fork or not?

Only a low priority one. I have made provision in the code to maintain it, but it is not extensively tested.

fluid_player_seek was added in fluidsynth 2.0.0. It would be easy to make the function import on line 578 and the Player method conditional.

In the newer branch linked above, unsupported functions when using fluidsynth 1.x should never lead to an ImportError, but can still lead to runtime errors, when trying to use function that does not exist in the used fluidsynth library yet/anymore.

jmaibaum commented 4 years ago

In the newer branch linked above, unsupported functions when using fluidsynth 1.x should never lead to an ImportError, but can still lead to runtime errors, when trying to use function that does not exist in the used fluidsynth library yet/anymore.

Thanks, it seems to work fine with the fork linked in https://github.com/nwhitehead/pyfluidsynth/pull/21#issuecomment-605272797 on Raspbian Buster/fluidsynth 1.1.11. Since I'm only using 1.x API for now, I should be fine I guess...

alextz commented 4 years ago

Quick note: iteritems can be un-imported if line 1157 or 1164, depending on version, is:

for opt, val in kwargs.items(): instead of: for opt, val in iteritems(kwargs):

SpotlightKid commented 4 years ago

@alextz: the whole point of using six.iteritems instead of dict.items is to have the same semantics on Python 2 and 3. Anyway, this PR is going nowhere, it seems, so it's a moot point.

alextz commented 4 years ago

dict.items() goes back to Python V1. LIke {}.keys() and {}.values() These aren't going away, even when Python and JavaScript are merged in 2040. :)

Yep, it looks like the thing to do is to snatch one of the two updated versions of fluidsynth.py referred to in this PR's comments. Lotta nice work done on 'em!

SpotlightKid commented 4 years ago

dict.items() goes back to Python V1.

@alextz: Yes, but they behave differently. The Python 2 version builds a list, the Python 3 a type of iterator. Look it up in the Python 3.0 "What's new" docs. Things like these are exactly the reason why six exists.

albedozero commented 3 years ago

At lot of the features in this fork and in https://github.com/SpotlightKid/pyfluidsynth/tree/feature/more-refactoring could be useful, if they can be incorporated into the current master. I'll spend some time looking at how to go about that, but if anyone wants to offer thoughts or contribute time/effort that would be appreciated :)

cclauss commented 4 months ago

@SpotlightKid Please rebase to resolve git conflicts.

SpotlightKid commented 4 months ago

See my last comment above.