feat: implement generic interface for plugins

[frenchwr]

This PR is motivated by the request in #447 for the upstream GIMP packaging team to take over ownership of this snap.

What has changed

• Qt G'MIC part removed from snap and migrated to a standalone snap
• OpenVINO plug removed from snap
• Generic plug introduced that supports connecting to multiple slots and mounts their content at $SNAP/gimp-plugins
• OpenVINO command chain scripts removed from snap
• Generic command chain script introduced that supports loading wrapper scripts from content snaps, which is helpful for running setup commands (e.g. exporting environment variables)
• OpenVINO plugin docs removed from README and snapcraft.yaml
• Generic plug docs added to README

What has not changed

• Multiple plug definitions needed for supporting OpenVINO plugins. Unfortunately these cannot be delegated to the openvino-ai-plugins-gimp snap as GIMP itself needs access to the host resources or libs provided by these plugs.

Testing

I have tested on my laptop with the following commands.

sudo snap install --dangerous ./gimp_3.0.4_amd64.snap 
sudo snap install openvino-toolkit-2404 --channel latest/edge
sudo snap install gimp-plugins-gmic --channel latest/edge
sudo snap connect gimp:gimp-plugins gimp-plugins-gmic:gimp-plugins
sudo snap install openvino-ai-plugins-gimp --channel latest/edge
sudo snap connect gimp:gimp-plugins openvino-ai-plugins-gimp:gimp-plugins
sudo snap connect gimp:dot-local-share-openvino-ai-plugins-gimp

Note the last command is only needed because the GIMP snap was built and installed locally. Normally this would auto-connect when installed from the Snap Store. We could also pursue auto-connections for the gimp-plugins-gmic and openvino-ai-plugins-gimp snaps.

• Screenshot showing OpenVINO's stable diffusion plugin in action:

• Screenshot showing Qt G'MIC in action, editing the image resulting from the stable diffusion plugin

Related work

• Qt G'MIC plugins snap
• Build command-chain script into snap canonical/openvino-toolkit-snap#24
• Refactor for generic GIMP plug canonical/openvino-ai-plugins-gimp-snap#18

[Jehan]

Hi! I am not sure to understand. Did you not manage to have the OpenVino into its own plug-in snap? Only G'MIC? So that means the OpenVino plug-in is dropped?

[brunvonlope]

@Jehan Yes. From my understanding of the patches OpenVino, GMIC (and any other 3P plug-in) will "mount" on /PATH_TO_SNAP_FILES/gimp-plugins like we mount on /PATH_TO_FLATPAK_FILES/extensions

Actually, it was mostly separated but the initialization of some OpenVino things like GPU etc was being done hardcoded in GIMP Snap files. Now, it is done by command-chain/enable-plugins.

And I request your security opinion about such enable-plugins Shell file because it allows to run anything the 3P plugin want before GIMP command is run?

[brunvonlope]

@Jehan I ported this MR to our Nightly snap so you can understand the diff easier: https://gitlab.gnome.org/GNOME/gimp/-/merge_requests/2452/diffs

[frenchwr]

Please note I just added a small commit to add back a few plugs to the GIMP app section. I initially removed these when I began the work and forgot to re-add them. The plug definition was in the original PR but they were not actually being used.

[frenchwr]

@brunvonlope Yes, I think that captures it well.

Did you not manage to have the OpenVino into its own plug-in snap? Only G'MIC? So that means the OpenVino plug-in is dropped?

OpenVINO is already in its own plug-in snap called openvino-ai-plugins-gimp. With this PR we no longer have direct references to this snap (except maybe the dot-local-share plug described below). It's been replaced by a generic interface that can be used by OpenVINO, G'MIC, and others in the future.

The only parts I was unable to remove were the following plugs:

• intel-npu: provides access to the Intel NPU device node(s) in /dev/accel on the host. The OpenVINO plugins need access to these nodes but since the plugins are run by the GIMP process you cannot delegate access to the openvino-ai-plugins-gimp snap.
• npu-libs: These are the .so's for the Intel NPU user mode driver and compiler. It's possible to build these into the openvino-ai-plugins-gimp snap but then you lose the advantages of running and testing different versions easily.
• openvino-libs: similar to npu-libs. These provide .so's and Python code used by the openvino-ai-plugins-gimp snap.
• dot-local-share-openvino-ai-plugins-gimp: grants r/w access to $HOME/.local/share/openvino-ai-plugins-gimp on the host for installing AI models used by the OpenVINO plugins. The openvino-ai-plugins-gimp snap distributes a command that can be used to install models to this same path, or you can install them directly through GIMP (see the model management window in my screenshot above).

[frenchwr]

And I request your security opinion about such enable-plugins Shell file because it allows to run anything the 3P plugin want before GIMP command is run?

I am definitely open to enhancing the script if security is a concern. I purposefully kept it simple to reduce maintenance and make it easy to understand for future plugin packagers.

[brunvonlope]

@frenchwr intel-npu and npu-libs plugs are provided by which Snap? They seem to be generic so that wouldn't be a problem to maintain

Regarding openvino-libs and dot-local-share-openvino-ai-plugins-gimp, I tell you in advance that, while I understand the explanation, they can't go on in the official snap (if it comes to GIMP umbrella). As upstream, we follow a maintenance principle of never specificying plugin-specific (non-vanilla) things in packages. I mean, it is a good first step but still a lot of OpenVINO-specific things are in the .yml

[frenchwr]

@frenchwr intel-npu and npu-libs plugs are provided by which Snap? They seem to be generic so that wouldn't be a problem to maintain

They are both provided by the intel-npu-driver snap.

[brunvonlope]

Yeah, I'm fine with these two from intel-npu-driver👍. While they are hardware-specific is not like we have a lot of CPU vendors around there to care of

[frenchwr]

Regarding openvino-libs and dot-local-share-openvino-ai-plugins-gimp, I tell you in advance that, while I understand the explanation, they can't go on in the official snap (if it comes to GIMP umbrella). As upstream, we follow a maintenance principle of never specificying plugin-specific (non-vanilla) things in packages. I mean, it is a good first step but still a lot of OpenVINO-specific things are in the .yml

I see. For openvino-libs I could build those directly into the openvino-ai-plugins-gimp snap. It'd be a bit of a bummer but it's something we can do.

For dot-local-share-openvino-ai-plugins-gimp is it just the name of the plug that is the issue? If so perhaps we could rename it and make it something more general purpose for the GIMP snap.

The other option is to only allow users to install models from GIMP, and not from this command that is distributed with the openvino-ai-plugins-gimp snap. Then we would not need a shared location and could simply install the models in SNAP_USER_DATA or SNAP_USER_COMMON.

[brunvonlope]

If so perhaps we could rename it and make it something more general purpose for the GIMP snap.

Would be nice. On GIMP we normally point user data to $HOME/.config/GIMP/MAJOR.MINOR

[Jehan]

And I request your security opinion about such enable-plugins Shell file because it allows to run anything the 3P plugin want before GIMP command is run?

Well sure, from a security standpoint, it does mean that these third-party snaps can run anything they want. Now I guess it's still within the main sandbox, and yeah when you add code from various sources, you do weaken security. You have to somehow trust the sources for all the code you run. It's not that different from any other system.

That's all the more reasons why we want this code separated. We don't want to be responsible for it. We will provide the "entry point" for third-party plug-ins to plug into the GIMP snap, but in the end, it's the people's job (the ones who install GIMP and these third-party plug-ins) to decide if they trust all the software source (us, but also the third-party plug-in developers).

So I don't see more of a problem than that.

Ideally though if the only use of this all is just to add some environment variables (at least, that's what I see in the .env in the G'MIC snap, though I could not find a .env in the OpenVino snap… or is that supposed to be this file?), we could make up some more basic format only accepting var=value code.

But if it really needs to be random shell commands, then I guess… 🤷

[jnsgruk]

Overall I like the approach. It still seems like we have more than we might need like openvino-libs as a plug - if we can avoid that then we should, but I also don't think it'll cause the upstream any issues if we can't find a workaround.

As for dot-local-share-openvino-ai-plugins-gimp - if we can make sure that the plugins just use a directory that GIMP already has access to, I think that would be preferable.

[brunvonlope]

but I also don't think it'll cause the upstream any issues if we can't find a workaround.

It will open a precedent to list plugin specific plugs etc in an upstream manifest. I see that for a downstreamer this is easy and fun but for us (upstreamers) that maintain official packages used by many people this is not acceptable. We can't maintain a manifest with plugin developers reporting to add plugin-specific plugs: etc to privilege specific plugins. We need to stay neutral.

[frenchwr]

Overall I like the approach. It still seems like we have more than we might need like openvino-libs as a plug - if we can avoid that then we should, but I also don't think it'll cause the upstream any issues if we can't find a workaround.

As for dot-local-share-openvino-ai-plugins-gimp - if we can make sure that the plugins just use a directory that GIMP already has access to, I think that would be preferable.

Ok - let me work on removing these later (hopefully today). We'll lose some convenient features but nothing essential.

[frenchwr]

Ideally though if the only use of this all is just to add some environment variables (at least, that's what I see in the .env in the G'MIC snap, though I could not find a .env in the OpenVino snap… or is that supposed to be this file?), we could make up some more basic format only accepting var=value code.

That's the one (well, a slight re-named variation of it is in a PR). It's mostly just setting environment vars but there is a python script that gets run and is needed to do device detection (i.e. CPU, GPU, NPU) at runtime.

[frenchwr]

The openvino-libs and dot-local-share-openvino-ai-plugins-gimp plugs are now gone, and I've tested that the plugins still function correctly.

I managed this by (i) installing the openvino-toolkit-2404 snap directly into the openvino-ai-plugins-gimp snap via stage-snaps and (ii) switching to a model installation path that is private to the GIMP snap at ~/snap/gimp/common (also known as SNAP_USER_COMMON).

[jnsgruk]

Nice! Thanks Will! @Jehan & @brunvonlope this must be getting pretty close for you? :)

[brunvonlope]

@jnsgruk Seems fine 👍. I will just wait for Jehan final word. I didn't totally understand what he said in this comment #460 (comment) about the wrapper concept:

gimp/command-chain/enable-plugins

Line 9 in 4157064

source "${wrapper}"

[Jehan]

@brunvonlope I mostly said that it's not ideal, but well… I was a bit fatalistic, but maybe we should strive for the best Snap interface. If we settle for "meh", we'll regret it later and it will be hard to change once all plug-ins will start using this.

That's the one (well, a slight re-named variation of it is in a PR). It's mostly just setting environment vars but there is a python script that gets run and is needed to do device detection (i.e. CPU, GPU, NPU) at runtime.

Why isn't this device detection code in the plug-in itself? In fact, could someone tell me exactly when these .env files are run? Is it when you start snapped GIMP, the snap runs all the .env files before it starts GIMP?

If it's to generate some data which needs to be in the Snap, couldn't it be run during the Snap creation?

And if it needs absolutely to be run at the start of GIMP, each time the plug-in is updated, this code can be run inside the query_procedures() implemention of the GimpPlugIn. Or if it really needs to be run at each and every startup, the plug-in developers can implement init_procedures() instead.

[frenchwr]

Why isn't this device detection code in the plug-in itself?

The issue is that the plugins rely on device detection at install-time (you can see the logic here, if you're interested), which is not ideal as generally the snap build environment and snap runtime environment can be very different. I've raised this with the plugin developers but at the moment the snap overcomes this by re-running their logic for device detection each time GIMP is run.

In fact, could someone tell me exactly when these .env files are run? Is it when you start snapped GIMP, the snap runs all the .env files before it starts GIMP?

Yes, exactly. Btw, I realize that file naming is a bit unusual. I was looking for a unique naming pattern and I thought .env at least conveyed (in part) their intention. However, I'm happy to change the pattern or even the design if there are concerns. I'm open to ideas. :)

[frenchwr]

@Jehan @brunvonlope What do you think about the state of this work? Any other feedback or suggestions?

[brunvonlope]

@Jehan Ping to see #460 (comment)

[Jehan]

I was planning on suggesting a MR to https://github.com/intel/openvino-ai-plugins-gimp/

I just haven't had the time to look at it yet. But anyway moving their hardware-detection code to query_procedures() (which will be run by GIMP every time the plug-in is updated) should be extra simple and would get rid of the whole issue of having to run it by the Snap.

My idea is that if we are lax now, then later it's too late because we have created the interface allowing plug-ins to just run random code out of GIMP control. And once the interface is there, it will be hard to remove.

P.S.: if anyone gets what I suggest — it should really not be hard to implement at all! —, feel free to make the MR to OpenVino plug-in developers. I don't need to be the one doing it and I've already zillions of things to do.

[brunvonlope]

@frenchwr Now I think things are clearer. What is blocking this MR is the implementation of all the initialization in the plug-in itself. There is the Python reference of GIMP API: https://lazka.github.io/pgi-docs/#Gimp-3.0.

Then, without the wrapping concept, we can merge this MR.

[Jehan]

If anyone wants to do the MRs, it's pretty easy. Search for query_procedures in the plug-ins: https://github.com/search?q=repo%3Aintel%2Fopenvino-ai-plugins-gimp%20query_procedures&type=code

On the OpenVino side, I see there are 3 such plug-ins. Then move the install_utils code (currently in .env file) in there.

Well it's probably not needed in all 3 plug-ins. According to the comments I see in the .env, it looks like this is only for the super resolution and semseg plug-ins. But even running it twice seems too much. So either this can be split into 2 calls (in the semseg plug-in, you only generate the semseg models and in super-res plug-in only the relevant models). Or else maybe add some flag file with the date or something like this, so the first plug-in to run generates all the data and the second doesn't, after having verified the flag time (or some other way to make sure only one plug-in does the work if several are installed).

I was mentioning possibly switching to init_procedures() in a previous comment, but that's probably not needed. If the code is in init_procedures(), it will be run unconditionally at every startup. In query_procedures(), it will only be run every time you update the plug-in. From what I gathered, it should be enough. No need to overdo it.

The whole patch should take just a few minutes to make. That's the testing which is currently blocking me as it means I'd have to install the plug-in, test it, make it sure everything works fine how it should (while I never used it until now) with the patch, etc. It would not take hours, yet still more than someone who already has it all set up. So really anyone, feel free to make this patch before I do. 🤗

[frenchwr]

Thanks for the suggestions, @Jehan . I'm open to proposing something to the maintainers, I expect they will be open to it but it may take some weeks for them to test, merge, release, etc. In the meantime we can maintain a patch in the snap package of the plugins. I will take a closer look later this week.

You hinted before that you are ok with plugin snaps exporting environment variables within a wrapper that gets called before running GIMP - just making sure that is still the case? If so, I can make changes in this PR that enforces this, where we accept only shell variables expressed as key-value pairs from the .env wrappers. Let me know if this sounds reasonable.

[Jehan]

@frenchwr That would be a lesser evil, but is it absolutely needed even? I mean, this is not needed in system packages. This is not needed on Flatpak. Why would this be needed in Snaps?

GIMP plug-ins should just have their environment straight when GIMP calls them. It's not very complicated (and it works with the same plug-ins on any other system I know).

[frenchwr]

@brunvonlope I have pushed another fix (see the commit message for details): f777258

I was seeing a crash on one of the NPU systems before deploying this fix.

I have now tested this on two different generations of NPU and everything looks good, but just with my local build. If you could generate a new gitlab build with this fix I will do one last (hopefully!) round of testing.

[brunvonlope]

@frenchwr @Jehan I wonder if f777258 will not make GIMP load the libraries from the plug-in snap first. From my understanding RUNPATH don't have precedente over LD_LIBRARY_PATH.

So, this would conflict with the way flatpak behaves? #460 (comment)

If the $LD_LIBRARY_PATH environment variable exists, the path here is appended to it, otherwise it is put in a ld config file in /run/flatpak/ld.so.conf.d.

[frenchwr]

@frenchwr @Jehan I wonder if f777258 will not make GIMP load the libraries from the plug-in snap first.

Yes, that was my intention, but as I've thought more about it I suppose this is too dangerous, as it means a plug-in could break GIMP if it introduces a conflicting lib that GIMP depends on.

The reason I need the change is that the OpenVINO plugin snap ships some bleeding edge graphics libs for supporting newer Intel hardware. The plugins do not run at all on Intel Lunar Lake CPUs (which have a GPU and NPU built-int) when using the stock graphical libs that ship with GIMP (actually these are delivered via a different content producer snap).

I will re-visit tomorrow. Most likely I will revert f777258 and manage LD_LIBRARY_PATH from the plugin code so that it picks up the newer graphics libs only from its process space without impacting GIMP itself.

[frenchwr]

Ok, I indeed reverted the last commit, so I'm back to appending to LD_LIBRARY_PATH instead of prepending. This took some refactoring work on the OpenVINO plugins (it's amazing how such a seemingly simple change can complicate things...) but I now have it working on the Lunar Lake system that gave me some problems before. I expect the other two systems should also work fine but let me test those (first thing Monday) just to be sure then we can merge.

Hope everyone has a nice weekend. :)

[brunvonlope]

@frenchwr Did the last upstream .snap I linked (which is the same as yours after the revert) worked with OpenVINO?

[frenchwr]

@frenchwr Did the last upstream .snap I linked (which is the same as yours after the revert) worked with OpenVINO?

Yes, I just need to test on a few more machines on Monday.

[frenchwr]

Success! I've now tested the GIMP 3.1.5 snap from the most recent Gitlab build on three different machines spanning different generations of hardware and Ubuntu releases: both the G'MIC and OpenVINO plugins work across the board.

I also rebased against the tip of candidate so d3169ee is also now included in this PR.

[jnsgruk]

Okay, great! I'll take one final look and merge this once the CI passes! Thank you, Will!

One thing we probably need to figure out is where to put the docs for adding the OpenVINO and GMIX plugins when using the Snap - perhaps there is somewhere in the upstream docs we can do that @Jehan?

[brunvonlope]

One thing we probably need to figure out is where to put the docs for adding the OpenVINO and GMIX plugins when using the Snap - perhaps there is somewhere in the upstream docs we can do that @Jehan?

The binaries will be stored in /snap. Are you talking about the gimp-plugins interface?

[jnsgruk]

Are you talking about the gimp-plugins interface?

Yeh, I'm talking about a brief docs entry we can send existing users of the snap to, so they know how to get GMIC back, or get OpenVINO back - where previously it was installed as part of the snap, they now need to add extra snaps that place content in the new plugins interface :)

[brunvonlope]

@jnsgruk I think that, for now, we need to mention in the Snap Store description.

Ideally, Snap Store itself should do that. See: canonical/snapcraft.io#5240 and ubuntu/app-center#1954

[jnsgruk]

Oh that's easy enough - just needs adding to the description field in your snapcraft.yaml and it'll get populated on the store :)

[Jehan]

One thing we probably need to figure out is where to put the docs for adding the OpenVINO and GMIX plugins when using the Snap - perhaps there is somewhere in the upstream docs we can do that @Jehan?

I don't think we want to favor specific plug-ins in the documentation. But we can put down command lines to install these in the news which we'll probably publish to announce our new Snap.

Ideally though, Snap should have an internal logic to "discover" extensions for another package. That's what they did in Flatpak so that on GNOME Software or other installer GUI, the "GIMP" page has a list of all extensions which can optionally be installed.

[brunvonlope]

Ideally though, Snap should have an internal logic to "discover" extensions for another package. That's what they did in Flatpak so that on GNOME Software or other installer GUI, the "GIMP" page has a list of all extensions which can optionally be installed.

@Jehan Yeah, I filled two issues for this. For the short term, we need to mention these two plugins to not disrupt greatly users expectations.

@jnsgruk But in the future I should set a ETA to remove the mentions. Then, we will need collaboration of Cannonical guys with the issues I linked.

[Jehan]

Just to make it clearer, the reason why this should not be maintained manually is:

• On our side, we don't want to favor specific extensions, which also means that all the cool ones we never heard about get shadowed by the most famous which most already know anyway.
• An automatic system would allow people to write their own extensions without going through us! We don't even have to like or dislike an extension. We provide a public API and people are welcome to use it without asking us. Then they could upload on Snap Store and it should be discoverable by anyone without us having to "validate" the extension.

[jnsgruk]

Then, we will need collaboration of Cannonical guys with the issues I linked.

Yes, I can work on that - not immediately - but I can start some discussions on how we solve that problem going forward!

[brunvonlope]

@Jehan Just to clarify too: I don't pretend to maintain manually new plugins in a list. What I mean is that we need to mention just these two historic plugins that were being shipped before the generic interface. Otherwise, will be mostly impossible to find them until it is not implemented on Snap Store frontend 😅