WIP Implement backends as plugins
Needs ReviewPublic

Authored by Leon0402 on Sep 13 2019, 6:13 PM.

Details

Reviewers
davidre
Group Reviewers
Spectacle
VDG
Summary

A flexible approach to reduce overhead and allow a spectacle to be used on more platforms (cross platforms) and add new features like screen record capabilities without drawbacks.
Makes Code also more readable as for example the X11 backend can be splitted into two different backends (one for kwin, one for general)

Backends are now plugins, which can be linked statically or be loaded dynamically. The user has a new setting, in which he can see all available backends and can select a new one ... load/unload existing ones.

Currently this is work in progress, there are still some bugs and missing featurs

  • Bug fix: KWin X11 backend -> Fullscreen, rectangular mode doesn't work as the signal screenshot created is not emitted (presumably bug in KWin?)
  • Bug fix: Support for the case where no screenshot backend is available (due to that, currently all modes except the gui mode are disabled)
Test Plan

Can already been tested, but keep in mind that it still has certain issues (listed above), so not everything works perfectly find. But it should help understanding the idea.

See https://phabricator.kde.org/T11655

Diff Detail

Repository
R166 Spectacle
Branch
backendPlugins (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16921
Build 16939: arc lint + arc unit
Leon0402 created this revision.Sep 13 2019, 6:13 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptSep 13 2019, 6:13 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
Leon0402 requested review of this revision.Sep 13 2019, 6:13 PM
Leon0402 edited the test plan for this revision. (Show Details)Sep 13 2019, 6:14 PM
Leon0402 added reviewers: davidre, Spectacle, VDG.
Leon0402 edited the summary of this revision. (Show Details)
Leon0402 edited the test plan for this revision. (Show Details)

Cool beans. I'm not sure this is something that should be exposed to the user in the settings window, though--a tleast not as currently presented. I can see people disabling things and breaking Spectacle.

ndavis added a subscriber: ndavis.Sep 13 2019, 8:31 PM

Flexibility sounds good, but how am I supposed to know what I'm looking at or what options to pick? Perhaps it's better to handle the backends automatically, if possible?

I see your points at @ngraham and @ndavis

Maybe I should write a little bit more about how it currently works and what I would like to add to make this easy to use. Or maybe more appropriate: Easy to not do anything wrong :D Feel free to add some ideas and comment on mine

Problem 1: The user selects a backend, which is not working

Every plugin provides a json file describing the plugin's metadata. For example the look of the KWin Wayland json file:

{
    "Name" : "Wayland KWin Screenshot Backend",
    "Version" : "1.0",
    "Vendor" : "Leon De Andrade",

    "Platforms" : [
        "Wayland"
    ], 
    "Requirements" : [
        "KWin"
    ],

    "GrabModes" : [
        "AllScreens", 
        "WindowUnderCursor", 
        "CurrentScreen"
    ],
    "ShutterModes" : [
        "OnClick"
    ]
}

From the json file you can see what the difference between the backends is.
The most important thing is the platform on which the backend can run -> This could be Linux X11, Linux Wayland, Windows, Mac Os

As you can see from the screenshot above, this is displayed in the middle column currently. The program detects the current platform automatically and disabled the "activate Button" for the plugins, which cannot run on the current platform. In the screenshot I was using X11, therefore the Wayland plugin could not be selected.

The second important information is "Requirements" ... this is probably mostly the windows manager (but could also be other stuff ... that really depends on what is needed on Windows, Mac Os etc. to perform screenshots). For example this Wayland backend requires kwin to be available. We could again automatically detect if kwin is available ... the problem for this is that we probably don't want to hardcode all possible requirement as it's inflexible and blows up the code. Instead, it would be much better if each plugin could tell us if it's able to run by checking its own requirements.
One way to do this would be to add a method in the interface:

virtual bool isAvailable() const = 0;

With this approach we could do the same as with the platform, disable all options, which cannot run for some reason.

Problem 1 solved.

Disadvantage:
To run C++ code, we need to load plugins ... obviously it would be nicer to only use the json file, so we don't have to load plugins not needed. On the other hand ... it's very unlikely that there will be so many suitable plugins, which run on the current platform ... most likely there is only one option or 2. And static plugins are loaded anyways ... so it won't affect performance that much.

Problem 2: There are two suitable backends, which one should be choosen

First off, currently there is a default backend specified, which is the kwin backend. So it iterates over all plugins and always saves one suitable plugin ... then it looks up if this is the default plugin. If it is, it takes it, otherwise it continues the search. So in the end either the default plugin is used or the last suitable plugin found (if the default plugin is not available).
So there is an automatic picking.

Why should the user then have the option to choose at all? As you can see from the screenshot above for X11 Kwin there are currrently two backends suitable. The x11 kwin backend and the general X11 backend. The x11 kwin backend is most likely more efficient than the general X11 backend, but on the other hand the general X11 backend support currently more grab modes (so differences in functionalty are possible due to restrictions in certain modes, although not guaranteed).
So I think the user should be able to choose a backend depending on his requirements (he might need a specific feature). An optional feature would be to allow multiple backends selected e.g "Use this normally, but use the other plugins for features not provided by the first one"

So how does the user know, which backend is the best for his use case?

I believe there should be more information about the plugin. The settings should display the features, so the user know what backend supports which features. This would be one factor to help deciding.
Additionally I would add a property "Description", which we could show as a tooltip ... for x11 kwin it might be something like "This is a fast backend, which requires the window manager kwin. It offer less feature though than the general x11 backend. If those extra feature are not needed, it's recommended to use this backend".

And probably most importantly, I would add a "recommended" label. This will help unsure users.

But in general, we have to keep in mind, that all displayed backends work, so it's not super important to explain the common user the technical details of each backend. Most important is that he no matter what he does, everything works. The little extra things like a recommended label, will help enough to not confuse unexperienced users.

Problem 3: The user removes plugins, so there might be situations where no suitable backends can be used anymore

Imho this is the most critical problem. This case should be possible though as I plan to add screen record support (e.g. there will be a second table to select a screen record backend) and the user should have the option to use spectacly only as a screenshot tool or only as a screen recorder. I've planned that control for screenshot/screenrecord will not be displayed if there is no backend selected.

So what do we do about this?

Most importantly, the application will start no matter what. If there is no backend selected the main view (so all the options to do a screenshot / screen record will be removed). So the user has always the possibility to go back to the settings and enable the backends again. I believe if a user was able to go once to the settings and remove something against the recommended label, he will be able to revert his action then.

Additionally, I would like to display help text in the area where the screenshot picture is displayed / the screenshot options are together with a button, which brings the user directly to this setting in case there was not found a suitable backend. This should help users to know what happened and how to fix it.

And lastly, if the user is about to remove the last plugin or one specific platform, there should be an additional confirm dialog warning the user that there is no suitable backend anymore if he is on this platform.

Btw. when I say "remove" I mean unload ... so the plugin is still there and can be "installed" -> loaded again ... but it removes the overhead. The option "install/remove" is also only available for dynamic plugins, for static plugins the option is disabled anyway (see Screenshot).

In conclusion, I think my provided actions are enough to not confuse any user and prevent them from doing anything which makes spectacle unusable. On the other hand the settings would allow power users to select the best backend (based on what features they need) and allow all users to select whether they want to use Spectacle as a screenshot / screen record or screenshot + screen record tool, which I personally would find awsome :D

Anything requiring so technical an explanation of the options is certainly not something that should be in the UI. :)

Splitting out the backends is still a good idea for technical reasons though. However I don't think allowing users to select backends makes any sense. Why would a user ever want to turn off or remove a backend? They would lose functionality but gain nothing. What's the advantage from a user perspective?

Anything requiring so technical an explanation of the options is certainly not something that should be in the UI.

That's unfair :D it's not a finished feature, it's wip , it was a technical analysis of scenarios, which should be taken into account and possible solutions (with the option to add some solutions from you or another). That doesn't make the setting complicated or anything for the user. I could explain every single revision I've done in such great detail ;)

They would lose functionality but gain nothing. What's the advantage from a user perspective?

The idea behind this was performance reasons ... for example a few people said, when I asked them in vdg if they would like screen record capabilities, that they don't because they don't need it and it's a significant overhead. So it makes sense to unload plugins. But I agree there is actually no advantage of making this optional ... this should just be the default ... plugins which are not in use, should be not loaded :)
The "Activate" button should therefore load the plugin (if it's dynamic).

... for example a few people said, when I asked them in vdg if they would like screen record capabilities, that they don't because they don't need it and it's a significant overhead.

...But is this true? the feature doesn't exist yet. We have no idea whether or not it would impose overhead on Spectacle when not in use. Maybe we should implement it before designing a feature to turn it off if it has bad performance. Who knows, maybe the performance turns out to be just fine! :) I honestly have no idea how the feature could possibly cause bad performance when it's not in use. "I want to turn it off because it has bad performance" smacks of a fixable design error that should be fixed, not worked around.

Again I think the basic idea to make the backends plugins makes sense. I just don't see an advantage to exposing that fact to the user at this stage in the game.

...But is this true? the feature doesn't exist yet. We have no idea whether or not it would impose overhead on Spectacle when not in use. Maybe we should implement it before designing a feature to turn it off if it has bad performance. Who knows, maybe the performance turns out to be just fine! :) I honestly have no idea how the feature could possibly cause bad performance when it's not in use. "I want to turn it off because it has bad performance" smacks of a fixable design error that should be fixed, not worked around.

I should learn to not make unclear statements :D I think, the problem was less runtime performance, but in general overhead. This includes: Compile time, start up time, cluttered/more complictaed/more full interface.
I think in general it's a very good idea to allow users to disable the screen record feature (if there will be one in the future), but this could be done over a checkbox as well.

I understand your point and will reevaluate it. For now I will leave it as it's a useful development feature to test, if my backends are actually working :) When it's more ready to land, we can take it out or discuss again what features are actually needed by the user. I feel like there has to be some way how advanced users can switch the backend, because of needed features, but I'm thinking about other possibilities as well (maybe set it over the command line or something like that?)

Leon0402 updated this revision to Diff 66466.EditedThu, Sep 19, 1:50 PM
  • Use dynamic plugins instead of static plugins
  • Changed CMakeLists, so plugins will be compiled to shared libs (dymanic plugins)
  • Use json metadata
  • Improved auto selection algorithm (added "recommended" field in .json, so the algo knows, which plugin should be preferred)
  • Added runtime checks, so the plugins can check whether all requirements are fullfilled (for example if kwin is available)
  • plugins are unloaded automatically if not needed (instead of install/remove button)
  • Fixed some Bugs
Leon0402 edited the summary of this revision. (Show Details)Thu, Sep 19, 1:53 PM
Leon0402 edited the test plan for this revision. (Show Details)
Leon0402 updated this revision to Diff 66685.Mon, Sep 23, 4:34 PM
  • Added CaptureMode Rectangular Region
  • Moved code to determine wether the "Current Screen" CaptureMode should be shown to the comboBox
  • ComboxBox and shutter options update, when the backend changes
  • Fixed a few bugs
  • Only uses one enum now called CaptureMode, instead of two very similar enums
Leon0402 updated this revision to Diff 66687.Mon, Sep 23, 4:53 PM
  • Fixs to CMakeLists
Leon0402 added a comment.EditedMon, Sep 23, 5:03 PM

Hi there,

I did a few changes. Most important ones:

  • Support for dynamic plugins, removed support for static plugins (there is not really a point in adding them, if a plugin is required it should be built by the CMakeLists directly as done now)
  • Use json metadata for performance
  • Added runtime checks (for example to check whether kwin is available)
  • Improved selection algorithm (including a "preferred" property ... so plugins can be ranked)
  • Changes involving Capture Mode (modifying the enum, add RectangularRegionMode, update captureComboBox etc., when backend changes)

Known bugs:

  • If you switch to KWin Backend and then back to the general Xcb backend and do a shot in "rectangular region", only a fullscreen capture is performed ... haven't yet figured out the problem ... it's definitely related to the problem that there is no proper implementation of the rectangular region mode ... the backend performs a normal fullscreen shot and Spectacle seems to crop the image then
  • When the testing part is included in the CMakeLists, there is a build error ... I really don't know how to solve this (or what I have changed to cause this issue)

I think this is ready to be tested :)
Looking forward to some feedback!

Btw. the backend setting is just for development/testing ... it'll be removed (or only shown when a specific flag is used?) when this is ready to land as discussed with @ngraham

My initial thoughts (I was on vacation): Is there really an advantage in using shared libs? You claim it reduces overhead but I don't see in which way. Instead there is a performance penalty in using dynamic libraries compared to having all the code in one binary. Also we are not in a situation where we have that many backends where there would be a significant impact on executable size. Neither do we support a a public plugin framework where one could install third party plugins.
I see no point in this, sorry.

Also we are not in a situation where we have that many backends where there would be a significant impact on executable size

There are actually two main reasons, why I would like to see this happen:

Support for different backends: You could add as many as you like, one extra only for xcb kwin (I already did this, although it's not finished), one for wayland without kwin , one for Windows, one for mac os ... even one for android/iOS would be possible ... you could add a new backend supporting screen record (again for all the different platforms). It's a highly requested feature ... but on the other hand many people are absolutely against it, as they don't want the overhead (the overhead on the executable size or the visual overhead etc.). With this approach it's possible to add it without having all the disadvantages. You could even deploy Spectacle with only screen record capabilities.
I know that some of the ideas might not happen, are not the intention/ the way to go. I don't want a discussion here, if we should really add screensrecord capabilties (or deploy a screen record app only) etc. . I just want to make clear that the approach gives us a lot more flexibility and allows us to do things, which weren't possible before.
I also know that you are in general not a friend of implementing stuff, which could allow things, which are not implemented yet. Well, there is already a change with the new kwin backend, which has quite a lot of advantages, but obviously that would have been possible without this revision (Even though, it might be not as clean). But I mean, there is currently no windows backend or mac Os backend etc. ... But I think this revision made it more attractive to implement these. Because it would not be possible to make spectacle cross platform before (by not possible, I mean not possible without clear disadvantages, messy code etc.). And there might be a developer, who actually knows how to do a windows backend and want to do it, but it was just to complicated up to now due to the fact that the current architecture was not made for it.
And if absolutely required, I will work on a backend for a different platform, which builds up on this revision, before accepting this revision ;)

Cleaner code: Somehow I feel like you see that differently, but I feel like the approach is a lot more cleaner than the current approach. This is especially true for the xcb backend. Compare the old wayland code vs the xcb code ... the wayland code is really clean and straight forward. It's easy to understand it, modify it, find bugs if there are some (actually the risk of having bugs at all is less likely). Because the wayland code is kwin only and uses the method provided by kwin (which is ihmo a very good idea). The xcb backend in contrast uses the kwin methods if possible (at least it claims to do it, it actually doesn't always) and if not available falls back to its own very complicated method.
Just a few things I noticed through the new appraoch, which were difficult to spot in the old code ( I don't know if you were aware of these, but I wasn't):

  • Kwin has methods for performing a fullscreen shot, but they are not used by the old xcb backend
  • The fullscreen shot method in kwin doesn't work (as far as I found out). Well this could be the reason, why the old xcb backend didn't use it, but tbh I suspect it wasn't and actually nobody was aware that the method doesn't work
  • The rectangular mode is kinda fake :D There wasn't even a mode in the backend enum ... actually Spectacle just performs a fullscreenshot and crops it in the main source code (not in the backend). And how it exactly is implemented is quite hard to read in my opinion.
  • Kwin has again already methods for performing a shot of a rectangular region. It's not used, neither by xcb nor by wayland. (I haven't actually test the method yet though).

So it's quite a collection of things, which you can find out quite easily with the separation, but is hard to spot in the old long and unreadable code.

And also quite important, you were never able to add new modes, new options provided by kwin to the old xcb backend, if you were not able to provide code for the new mode/new options, which works without kwin. Because the backend does promises about its functionality and the xcb backend was for xcb systems with or without kwin, so new things have to work at least without kwin. I believe that could be the reason why you need to click on wayland to do a screenshot, this is a new method provided by kwin called "interactive", which is done for security reasons as far as I know (This should not be a discussion about whether this was a good idea or not). I see no reason why this only applies to wayland and not also to xcb? In fact, I believe we would use the same interactive method on xcb as well, if possible. But it isn't possible due to the fact that there is no fallback interactive method without kwin. Of course this is just an assumption, but I think it should make my point clear that there are situations, where we cannot add new things to the backend due to that limitation.

Hopefully I made some good points, why this idea is useful and should be accepted (if finished). Feel free to comment on the points, these are just things I had in my mind, maybe I'm wrong about these.