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

[plugin API] Make plugin API extendable #5347

Open
Spacechild1 opened this issue Feb 10, 2021 · 19 comments
Open

[plugin API] Make plugin API extendable #5347

Spacechild1 opened this issue Feb 10, 2021 · 19 comments

Comments

@Spacechild1
Copy link
Contributor

Spacechild1 commented Feb 10, 2021

Currently, every change to the plugin API leads to an ABI breakage, so all existing plugins have to be recompiled.

Of course, ABI breakage is unavoidable when a public struct changes (members added, removed or changed in size).

However, there should be no need to bump the plugin API version everytime we add a new method.

One simple solution is to add a dummy void * array to the end of the interface table. The array is zeroed, so a plugin can simply check for NULL to see if the method is supported. This is necessary so that plugins compiled against a recent sclang version can still work (or at least not crash :-) with previous versions.

struct InterfaceTable {
    ... // existing methods
    void *reserved[256]; // set to zero
};

When we add an API method, we add it before the padding array and reduce the size of the array by one:

struct InterfaceTable {
    ... // existing methods
    void (someNewCoolMethod*)(void);
    void *reserved[255];
};

There are, of course, more elaborate techniques. For example, the plugin could get the interface table through a function that takes a version argument, so the host can return different interface tables. However, I have the feeling that it only makes things unnecessarily complicated. Most of time we should only need to add functions. *) If we have to change things, we can still bump the plugin API version and force a recompilation.

*) plugin API methods can also be used to extend the scsynth host context! Instead of adding new members to World and breaking ABI, one could bundle the members in a new struct and return a pointer through a (new) API method.

@Spacechild1
Copy link
Contributor Author

Spacechild1 commented Feb 10, 2021

There was already a discussion on the SC forum: https://scsynth.org/t/scsynth-plugincmd-and-sending-responses/2638/5?u=spacechild1

@scztt feel free to join in!

@scztt
Copy link
Contributor

scztt commented Feb 18, 2021

There are, of course, more elaborate techniques. For example, the plugin could get the interface table through a function that takes a version argument, so the host can return different interface tables. However, I have the feeling that it only makes things unnecessarily complicated. Most of time we should only need to add functions. *) If we have to change things, we can still bump the plugin API version and force a recompilation.

I think this is the right approach. I think the better architecture MIGHT be a pull model, where the plugin requests the InterfaceTable with a version number - but this diverges from the mechanism we have now, and we'd need to maintain both indefinitely, so probably not worth it.


The one problem I see with this - I'd love to brainstorm how we can make it better: Suppose we make this change and our api version is basically locked where ever it is now (since we have backwards compatibility, we don't ever need to bump it).... Now imagine it's far in the future and we've added many new API's: The only way the plugin will have of detecting whether those API's are available (e.g. if they are running on an older server) is to null-check the entry in the table. It's reasonable to think that the plugin WILL have hard dependencies on some of those newer table entries. This means a situation where the plugin needs to, in their load function, do a null check on every single IT entry that they are going to use, and error of any of them are not present. This is... a lot of pain to push down to plugin developers. It also basically means we can only know if a plugin is compatible by trying to load it, and waiting for an error.

Here's one solution: we CONTINUE to bump the API version when we add entries to the InterfaceTable, as we are doing now. Our API has a contract that, for the api_version() you specify, the interface table you are passed will have non-null entries for every field defined AT THAT VERSION. Then you have these scenarios:

  1. server_version == plugin_api_version
    Obviously, the plugin gets a fully valid IT.
  2. server_version < plugin_api_version
    The plugin requires entries that the server cannot provide, so it is never loaded in the first place.
  3. server_version > plugin_api_version
    Server passes it's current IT - in normal circumstances, the plugin has all the entries it needs populated. If the plugin can optionally support newer features, it can up-cast the IT and then check for null entries to determine whether they are available. Optionally, we have the ability to patch up the IT to e.g. workaround version specific bugs etc.

@Spacechild1
Copy link
Contributor Author

Generally, I think that's a good idea. Two comments:

we CONTINUE to bump the API version when we add entries to the InterfaceTable, as we are doing now.

There might be still situations where the plugin API breaks. Maybe we could add a minor version number. For breaking changes, the major API version number is increased. For non-breaking changes (i.e. additions), the minor version number is increased. The major API version would have to match exactly (= current behavior), but the minor API version would follow the logic described above.

server_version < plugin_api_version

There's a minor problem: Currently the API version is hardcoded in the header file. Now, I might write a plugin which requires API version 4.3+, but I'm developing against recent header files where the API version might be something like 4.11. This means that my plugin will only run on the same recent SuperCollider version (and above), although older versions would actually work. This could be solved by providing a way to set the minimum required plugin API version at compile time.

@dyfer
Copy link
Member

dyfer commented Feb 4, 2022

We now have a PR with proposed API change: #5713. Also #5610, while not strictly breaking API, might also be related. I think it would be good to re-start discussion on this...

@Spacechild1
Copy link
Contributor Author

We now have a PR with proposed API change: #5713. Also #5610, while not strictly breaking API, might also be related. I think it would be good to re-start discussion on this...

I don't think that #5713 has any breaking changes. #5610 should be fine because it doesn't change the struct layout and the new member is for private use only.

Note that this issue is only about extending the plugin interface in a backwards compatible way (i.e. without having to recompile all existing plugins). Any changes to the plugin interface that necessarily break the API/ABI still require incrementing the plugin API version.

Actually, I still like the idea of introducing a minor version (#5347 (comment)).

@dyfer
Copy link
Member

dyfer commented Feb 7, 2022

Thanks for the explanation @Spacechild1 !

@dyfer
Copy link
Member

dyfer commented Feb 9, 2022

Not sure if this is the right thread to ask, but I was also wondering if it would be possible to have a single plugin file for scsynth and supernova. IIUC there would be some methods that would need to be chosen at plugin load, instead at compile time (e.g. these?), but with my incomplete understanding of plugin loading I wanted to ask if this is at all feasible.

@Spacechild1
Copy link
Contributor Author

Generally, scsynth and supernova plugins have to use different code because the latter uses spinlocks while the former doesn't.

In theory, it would be possible to have code for both combined in a single binary, but I think it would make things more complicated.

I think a better long-term solution would be to unify the plugin interface so that it wouldn't be necessary to have two different versions in the first place. One simple solution could be to add spinlocks to scsynth; they would be redundant and incure some (tiny) CPU overhead, but it would guarantee that the same plugin will run on both Servers.

Personally, I don't really have a problem with having two different binaries. The build system handles this transparently and the user usually doesn't notice.

@dyfer
Copy link
Member

dyfer commented Feb 10, 2022

Thanks for the explanation.

Personally, I don't really have a problem with having two different binaries. The build system handles this transparently and the user usually doesn't notice.

I agree it's not an issue for built-in plugins, but IMO it would help 3rd-party plugins (as would extensible API). As we are considering the future of sc3-plugins and feasibility of distributing plugins as independent packages, it might be nice to have a single binary. OTOH I know you have some experience with distributing 3rd-party plugins ;), do you think the need to build sn plugin separately is not an issue? (I have recently looked at FluCoMa which does not include supernova plugins in their release which renewed my interested in considering a unified plugin architecture)

@Spacechild1
Copy link
Contributor Author

Spacechild1 commented Feb 10, 2022

do you think the need to build sn plugin separately is not an issue?

Not at all. When using the coockie-cutter plugin or starting with the example CMakeLists.txt, this is pretty much transparent.

I guess the FluCoMa authors wrote their CMakeLists.txt from scratch and didn't know (or care) about Supernova.

I think in the long run (SC4?) it would make sense to merge scsynth and supernova anyway. That's maybe something to discuss in the dev meetings.

@weefuzzy
Copy link
Contributor

I guess the FluCoMa authors wrote their CMakeLists.txt from scratch and didn't know (or care) about Supernova.

Lurking FluCoMa author here, following this thread due to the original discussion back at the forum

We didn't implement Supernova support because it really wasn't clear who was actually using it and life was full enough at the outset trying to cater for scsynth, Max and PD. I'm pretty hazy on what the differences are for plugin authors, but if the threading model is significantly different, I'd personally be nervous about catering for both from the same code.

My 2c on the issue topic is that I'd still very much like it if the Plugin API were more extensible 😄 with the ulterior motive that we'd like the API to provide for copying ReplyCmd instances. I can contribute some labour as, at the moment, I'm employed to work on FluCoMa full time.

@Spacechild1
Copy link
Contributor Author

I'm pretty hazy on what the differences are for plugin authors, but if the threading model is significantly different, I'd personally be nervous about catering for both from the same code.

The only difference is that accessing Busses and Buffers requires locking. These macros are all you need:

# define ACQUIRE_BUS_AUDIO(index) unit->mWorld->mAudioBusLocks[index].lock()
# define ACQUIRE_BUS_AUDIO_SHARED(index) unit->mWorld->mAudioBusLocks[index].lock_shared()
# define RELEASE_BUS_AUDIO(index) unit->mWorld->mAudioBusLocks[index].unlock()
# define RELEASE_BUS_AUDIO_SHARED(index) unit->mWorld->mAudioBusLocks[index].unlock_shared()
# define LOCK_SNDBUF(buf) buffer_lock<false> lock_##buf(buf)
# define LOCK_SNDBUF_SHARED(buf) buffer_lock<true> lock_##buf(buf);
# define LOCK_SNDBUF2(buf1, buf2) buffer_lock2<false, false> lock_##buf1##_##buf2(buf1, buf2);
# define LOCK_SNDBUF2_SHARED(buf1, buf2) buffer_lock2<true, true> lock_##buf1##_##buf2(buf1, buf2);
# define LOCK_SNDBUF2_EXCLUSIVE_SHARED(buf1, buf2) buffer_lock2<false, true> lock_##buf1##_##buf2(buf1, buf2);
# define LOCK_SNDBUF2_SHARED_EXCLUSIVE(buf1, buf2) buffer_lock2<true, false> lock_##buf1##_##buf2(buf1, buf2);
# define ACQUIRE_SNDBUF(buf) \
do { \
if (!buf->isLocal) \
buf->lock.lock(); \
} while (false)
# define ACQUIRE_SNDBUF_SHARED(buf) \
do { \
if (!buf->isLocal) \
buf->lock.lock_shared(); \
} while (false)
# define RELEASE_SNDBUF(buf) \
do { \
if (!buf->isLocal) \
buf->lock.unlock(); \
} while (false)
# define RELEASE_SNDBUF_SHARED(buf) \
do { \
if (!buf->isLocal) \
buf->lock.unlock_shared(); \
} while (false)
# define ACQUIRE_BUS_CONTROL(index) unit->mWorld->mControlBusLock->lock()
# define RELEASE_BUS_CONTROL(index) unit->mWorld->mControlBusLock->unlock()

You can have a look at the core plugins for examples.

My 2c on the issue topic is that I'd still very much like it if the Plugin API were more extensible smile

+1. I already have a clear idea how to do this and I want to make a PR. I think I can manage in the next few weeks. This would need to wait for the next minor version (3.13) anyway.

@joshpar
Copy link
Member

joshpar commented Feb 10, 2022 via email

@Spacechild1
Copy link
Contributor Author

@joshpar Thanks for the info! I'm planning to attend the upcoming dev meeting, so we can talk about it.

@dyfer
Copy link
Member

dyfer commented Dec 18, 2022

I'd like to loop back regarding this.

The problem at hand is that upgrading bundled boost in SC will break FluCoMa plugin compatibility.

Since some will be necessary soon anyway, I'd like to proposed that we upgrade/break SC plugin interface to accommodate FluCoMa's needs (including compatibility with supernova), as well as make the interface extensible.

@Spacechild1 and @scztt , would there be a way to hash out the details of handling the extensible interface? How should we go about the proposed interface version numbering?

cc @tremblap @weefuzzy

@Spacechild1
Copy link
Contributor Author

The problem at hand is that upgrading bundled boost in SC will break FluCoMa plugin compatibility.

Why is that? I don't see any boost structures exposed in the public API... (That would be horrible.) Maybe FluCoMa uses some private APIs?

Anyway, if another plugin API break becomes necessary, it would be great to finally fix the extensibility issue, so thanks for pinging!

@dyfer
Copy link
Member

dyfer commented Dec 19, 2022

The problem at hand is that upgrading bundled boost in SC will break FluCoMa plugin compatibility.

Why is that? I don't see any boost structures exposed in the public API... (That would be horrible.) Maybe FluCoMa uses some private APIs?

I don't understand all the details, but quoting from @weefuzzy in flucoma/flucoma-sc#90:

As it stands, the brittle workaround for these lacunae in scsynth is to commit a code crime and just link to a bunch of internals that I have no business knowing or caring about. This then breaks if our plugins are built against a different SC (or Boost) to what's running. Not ideal but (I stress), not a decision made lightly.

Since there will be some breakage at least between FluCoMa and SC (due to boost upgrade atm), I'd like to push for plugin interface change that would 1) address the changes needed in FluCoMa (including support for supernova), which would prevent future breakage and 2) make the API extendable, if we are in agreement on how to do it.

@Spacechild1
Copy link
Contributor Author

I see!

Ad 1)
I think it would be great if @weefuzzy could make a feature request that describes what they want to do. (I suspect it is about sending OSC reply messages to clients.) Actually, I am also missing a few API functions, and there is probably some overlap :-)

Ad 2)
I would love to improve the plugin API! The good news is that all my big projects are finally over and I have much more time for open source stuff again. I will try to come up with a proof-of-concept in the next few days that can serve as a basis for further discussion. (The solution I imagine is quite simple, really.)

@weefuzzy
Copy link
Contributor

Of course – you suspect correctly! Feature request made at #5939

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Server plugin API
Requires breaking change
Development

No branches or pull requests

6 participants