GBM remote access support for KWin
ClosedPublic

Authored by Kanedias on Mar 26 2016, 8:02 PM.

Details

Summary

Implements a KWayland protocol to pass GBM fd from KWin to KRfb and
addictions to relevant projects from both sides.

Note that this patch does not affect default behaviour of mentioned projects. It can be used
only with KWIN_REMOTE=1 in env from KWin side and with preferredFrameBufferPlugin=gbm in krfbrc from
KRfb side. In all other aspects app behaviour remains unchanged.

Test Plan

Launched KWin in Wayland mode, launched KRfb in it, launched KRDC on a laptop, connected in read-only mode, observed a correctly retrieved desktop with Krfb window

Diff Detail

Repository
R108 KWin
Branch
gbm-vnc
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
aacid added a subscriber: aacid.Jun 25 2017, 4:08 PM

If this indeed needs fixing, can someone please mark it as "request changes" so it doesn't show up in the list of "patches that have been accepted but have not been commited"?

Kanedias updated this revision to Diff 18859.Aug 27 2017, 6:29 PM

Fixes for review comments, rebased to latest version

With D6186 merged, is this now ready to go in?

With D6186 merged, is this now ready to go in?

Yes, just waits for @graesslin's final review :)

Hi Oleg,

sorry that I haven't given you feedback to your patch series until now. First some smaller points:

  • Please merge current master.
  • What about the software cursor dynamically switching? I don't see where it does that.
  • The RemoteAccessManager is active all the time. It would be nice if it would be only active if there are clients who ask for buffers.

Then in general did you look at GNOME's way forward of screen recording/sharing on Wayland? They use PipeWire (http://pipewire.org/) for that. I'm not very knowledgeable on the whole desktop sharing / video stream side, so just a small info on what you think about it and how this relates to your code would be nice. I'm asking that, because I find joint work on single solutions very important.

plugins/platforms/drm/remoteaccess_manager.cpp
78

Local var for gbmbuf->getBo()

plugins/platforms/drm/remoteaccess_manager.h
24

Do you need all these includes in the header file, or could you forward declare for example the KWayland::Server::RemoteAccessManagerInterface and then include in the cpp file?

The RemoteAccessManager is active all the time. It would be nice if it would be only active if there are clients who ask for buffers.

How is a client going to ask for a buffer if there is no RemoteAccessManagerInterface? (which this owns)

Then in general did you look at GNOME's way forward of screen recording/sharing on Wayland?

They still glReadPixels out the framebuffer, which MG excplicitly didn't want and proposed this.

Us using this iface in a helper and shoving it into a pipebuffer should still please everyone.

In D1230#158795, @subdiff wrote:

Hi Oleg

Hi. Haven't been here in a long time.

  • Please merge current master.

Will do this and fix review comments on weekend (or earlier if I can).

  • The RemoteAccessManager is active all the time. It would be nice if it would be only active if there are clients who ask for buffers.

Well, it has active property in KWayland that is true only if at least one client is connected (if I remember my code from 6 months back correctly), so I can expose it and use here. Will this suffice?

Then in general did you look at GNOME's way forward of screen recording/sharing on Wayland?

No, sorry. This patchset was ready long before the GNOME news. I use different idea here that was proposed by Martin Graesslin, with GBM and Wayland protocol we actually can pass file descriptors to video buffer objects from one app to another
so KWin is passing BOs to KRfb which then can do all heavy-lifting without hurting KWin performance or stability.

The RemoteAccessManager is active all the time. It would be nice if it would be only active if there are clients who ask for buffers.

How is a client going to ask for a buffer if there is no RemoteAccessManagerInterface? (which this owns)

Yea, I meant that passBuffer is allways called. But I just realized on the isBound call it returns if there are no clients interested.

Well, it has active property in KWayland that is true only if at least one client is connected (if I remember my code from 6 months back correctly), so I can expose it and use here. Will this suffice?

I think you do already what I meant with the isBound() call. Sorry for the confusion. :)

Then in general did you look at GNOME's way forward of screen recording/sharing on Wayland?

No, sorry. This patchset was ready long before the GNOME news. I use different idea here that was proposed by Martin Graesslin, with GBM and Wayland protocol we actually can pass file descriptors to video buffer objects from one app to another
so KWin is passing BOs to KRfb which then can do all heavy-lifting without hurting KWin performance or stability.

The thing is I would like to see your solution also be applicable to other apps that need access to the buffer contents, like screen recording tools (SimpleScreenRecorder for example) and so on. If you say KRfb does the heavy-lifting, I assume such a screen recording tool would need to do this as well in a nontrivial additional backend.

The thing is I would like to see your solution also be applicable to other apps that need access to the buffer contents, like screen recording tools (SimpleScreenRecorder for example) and so on. If you say KRfb does the heavy-lifting, I assume such a screen recording tool would need to do this as well in a nontrivial additional backend.

But it is. By heavy-lifting I mean calling glReadPixels on that BO.

romangg added a comment.EditedOct 23 2017, 6:49 PM

The thing is I would like to see your solution also be applicable to other apps that need access to the buffer contents, like screen recording tools (SimpleScreenRecorder for example) and so on. If you say KRfb does the heavy-lifting, I assume such a screen recording tool would need to do this as well in a nontrivial additional backend.

But it is. By heavy-lifting I mean calling glReadPixels on that BO.

I'm not sure that I understood you correctly but the problem I mean is that every other app (SSR, OBS, ...) also needs to:

  • Speak to the proposed Wayland protocol extension
  • Have logic to handle GBM buffers and copy these buffers into Gl textures or something similar

While this is manageable, I would like to have such apps only need to write one backend for all Wayland compositors. PipeWire maybe could provide this.

I could imagine the following (broad strokes):

  • Use your KWin code
  • Use your KWayland code as a Plasma only protocol
  • Move your KRfb code into plasma-workspace in the following way:
    • Let a KDE Daemon process talk with KWin via your protocol
    • Let this Daemon handle client requests (we should align its interface with GNOME)
    • If clients want screen capture do a copy once per frame to Gl texture (if needed by the next step)
    • Pipe the Gl textures (or if PipeWire can handle GBM buffers these directly) into a PipeWire stream
  • Let KRfb talk to the Daemon and get video stream via PipeWire from it as other apps

On the other side PipeWire is pretty new and its documentation is nearly non-existent. I don't know if with PipeWire it really could work in the way I described above. Your current solution works right now. It might still be an idea if you are interested to check out if you could integrate your approach with PipeWire in the way I described above or something similar and if you could do this now or only after the current version version of your patches has landed.

On the other side PipeWire is pretty new and its documentation is nearly non-existent. I don't know if with PipeWire it really could work in the way I described above. Your current solution works right now. It might still be an idea if you are interested to check out if you could integrate your approach with PipeWire in the way I described above or something similar and if you could do this now or only after the current version version of your patches has landed.

Why is PipeWire preferred solution for this? GNOME has to support EGLStreams which have no such thing as buffer management like GBM, they can't just do buffer fd passing.
Martin said he's not interested in EGLStreams and this whole patchset was prepared in agreement with him. Yes, adding additional copying would solve many things, but I guess it's out of scope for this patch.

I'll take a look at PipeWire in my spare time. Is there any rush? I'm under strong impression something is happening.

romangg added a comment.EditedOct 23 2017, 7:51 PM

Why is PipeWire preferred solution for this?

The prefered solution from my side is one that interacts with clients the same way on every Wayland compositor, so that clients only have to write one new backend for Wayland and not n for n Wayland compositors. Another important Wayland compositor in real world at the moment besides KWin is GNOME's mutter. Since they already have a working solution using PipeWire we should try to align our one with their one if it is feasible.

GNOME has to support EGLStreams which have no such thing as buffer management like GBM, they can't just do buffer fd passing.
Martin said he's not interested in EGLStreams and this whole patchset was prepared in agreement with him.

Neither am I interested in EGLStreams. This is also not about not using GBM. My sketched concept with the KDE Daemon would still use GBM buffers for passing data between KWin and the Daemon. Only afterwards the Daemon would stream the Gl texture to interested clients via PipeWire instead of using GBM.

Yes, adding additional copying would solve many things, but I guess it's out of scope for this patch.

Not sure what you mean with additional copying. If you mean the Gl copy in the Daemon from my concept: As far as I've understood it with your current approach you need to do a Gl texture copy anyway in the KRfb backend (and probably in every other client using the protocol at the same time).

I'll take a look at PipeWire in my spare time. Is there any rush? I'm under strong impression something is happening.

Thank you. There is no rush and nothing is happening out of the orderly.

Ok @subdiff, I've got your idea. I'll align my patches with newest sources ASAP, check that they work, then we'll talk about what to do with KDE Daemon and Screen Recorder.
I just don't want to spread my efforts too much, maintaining more than one patchset in everchanging environment is quite troublesome.

Hope this finally gets merged :)

Kanedias updated this revision to Diff 21211.Oct 24 2017, 6:32 AM
Kanedias marked 2 inline comments as done.

Updated against latest master

This comment was removed by Kanedias.

What's the status of this?

@ngraham I'm still waiting for a review to land this

@graesslin, would you mind reviewing this so we can push forward with the feature? Thanks!

romangg added a comment.EditedJan 17 2018, 12:57 PM

I talked to @graesslin some time ago and he said, that @davidedmundson and I should do the review.

My plan is to get this review going after 5.12 has landed. For Plasma 5.13 then.

I would like to see at least some progress regarding PipeWire before that though. This has one reason mainly: I don't want to send out the signal to the public, that we would be only interested in our own solutions and would not be willing to cooperate with other DEs to get to unified ones on Wayland. On the other hand I can understand @Kanedias, that maintaining this patch set for such a long time was annoying and he wants to get done with it. So maybe we can find a compromise in setting up a separate task with a plan on what needs to be done after the patch has landed to integrate this solution with PipeWire. Could you do this @Kanedias? Just with some bullets points on what would probably be your next steps to integrate PipeWire with your solution here after the patches have landed?

@romangg , let's go with following approach: I'll upstream all changes I have left hanging (e.g. D9708) by the end of this week and will try to come up with some PoC work with PipeWire solution this/next weekend.
After it is working we'll have an implicit confirmation that these patches indeed have not regressed and can be merged.
And after they're merged I'll continue the work on PipeWire integration and discuss it in detail.

How does it sound?

jgrulich added a comment.EditedJan 18 2018, 8:11 AM

@romangg , let's go with following approach: I'll upstream all changes I have left hanging (e.g. D9708) by the end of this week and will try to come up with some PoC work with PipeWire solution this/next weekend.
After it is working we'll have an implicit confirmation that these patches indeed have not regressed and can be merged.
And after they're merged I'll continue the work on PipeWire integration and discuss it in detail.

How does it sound?

I see you have made some progress here, I had similar intentions to bring support for PipeWire to KWin as it's something we develop in Red Hat and trying to push it forward, but I'm still a newbie in this area and trying to understand the stuff around still. Anyway, I just wanted to tell you that I have an opportunity to talk to Wim Taymans (author of PipeWire) and attend a small PipeWire hackfest we will have in Brno's Red Hat office in two weeks so if there is anything I can help with, what should I discuss, suggest, whatever, just let me know, I would really like to help here.

After it is working we'll have an implicit confirmation that these patches indeed have not regressed and can be merged.

I don't fully understand what this means, but in general I think it sounds like a good plan. I just talked to David as well, and we agreed on making it our top priority after 5.12 release to finish this review.

I see you have made some progress here, I had similar intentions to bring support for PipeWire to KWin as it's something we develop in Red Hat and trying to push it forward, but I'm still a newbie in this area and trying to understand the stuff around still. Anyway, I just wanted to tell you that I have an opportunity to talk to Wim Taymans (author of PipeWire) and attend a small PipeWire hackfest we will have in Brno's Red Hat office in two weeks so if there is anything I can help with, what should I discuss, suggest, whatever, just let me know, I would really like to help here.

Great! Can you or @Kanedias then open a new KWin task here on Phabricator for everything related to the additional PipeWire support? We could then use this task to store questions for Wim and later the answers.

Kanedias added a comment.EditedJan 18 2018, 10:29 PM

Hi @jgrulich!

There are actually two sides of this dime.

The one that I wanted to take is implementing pipewire server in kded (that's being discussed in T7785). It will be responsible for converting GBM fds to PW video stream fds and will announce this interface via DBus like Mutter does.
The second one is implementing actual screen recorder based on KF5 with pipewire sink. It will consume the stream from kded and save it.

You can take the second subtask as it doesn't actually depend on KWin/KWayland.

Kanedias updated this revision to Diff 25979.Jan 26 2018, 6:30 AM
  • Merge branch 'master' into gbm-vnc
  • Fix compilation on Clang

@graesslin , @davidedmundson , please approve this once again, this was updated numerous times after initial review

Pls remove the whitespace and rebase your branch on current master (it merges without conflict).

plugins/platforms/drm/drm_backend.cpp
793 ↗(On Diff #25979)

rm whitespace

Kanedias updated this revision to Diff 28973.Mar 7 2018, 6:53 PM

Review comments

Kanedias marked 2 inline comments as done.Mar 7 2018, 6:58 PM

@romangg , I had to recreate it after rebase, cause I don't have force-push.

Please re-review this and D1231 as I'm starting to work on KRfb support for this and it will be the 3rd patch depending on this.
Or suggest reviewers with more spare time so they can look this through.

romangg added inline comments.Mar 12 2018, 10:21 PM
main_wayland.cpp
780

Put this after the if clause (such that it shows the selected backend also on manual setting). But it's unrelated to GBM remote accesss, so better remove it and commit it as separate patch.

plugins/platforms/drm/drm_backend.cpp
103

Unrelated to GBM remote access. Remove.

plugins/platforms/drm/drm_output.h
139

Is it only a friend class to access m_waylandOutput.data()? In this case better create a getter for it in DrmOutput.

Or better do the passBuffer call in DrmBackend::present and give instead of the DrmOutput the KWayland::Server::OutputInterface from there to passBuffer.

plugins/platforms/drm/egl_gbm_backend.cpp
160

Should be the default not directly activated remote funcitonality? And if one wants to deactivate remote set KWIN_NO_REMOTE or something.

plugins/platforms/drm/remoteaccess_manager.cpp
85

This will spam the debug because it is called on every present.

romangg added inline comments.Mar 13 2018, 3:24 PM
plugins/platforms/drm/drm_output.h
139

Regarding the "better" part: you would need the m_remoteaccessManager in DrmBackend. Only with EGL/GBM this would then be set. I.e. in QPainter DrmBackend would ignore it.

But that's maybe a bit too much (also you would need to change the buffer cast in passBuffer). So better just add a getter in DrmOutput.

Kanedias updated this revision to Diff 29433.Mar 13 2018, 8:40 PM
Kanedias marked 6 inline comments as done.

Review fixes

plugins/platforms/drm/egl_gbm_backend.cpp
160

There's no authentication protocol, so any malicious application can get whole screencast for free.
But that's another story as we're getting full input control already in fakeinput-protocol.
Changed.

plugins/platforms/drm/remoteaccess_manager.cpp
85

Removed both this and the one at line 77

romangg added inline comments.Mar 20 2018, 1:55 PM
plugins/platforms/drm/drm_output.h
135 ↗(On Diff #25979)

This gives me an compile error: invalid static_cast from type 'QObject*' to type 'KWayland::Server::OutputInterface*'

I assume you can just pass m_waylandOutput as QPointer and then access data() in passBuffer. This compiled for me.

Kanedias updated this revision to Diff 30092.Mar 21 2018, 7:01 AM
  • Fix clang compilation
  • Fix QPointer
Kanedias requested review of this revision.Mar 21 2018, 7:02 AM
Kanedias marked an inline comment as done.
Kanedias added inline comments.
plugins/platforms/drm/drm_output.h
135 ↗(On Diff #25979)

Fixed. Also removed override in places where it didn't compile because of it.

Why did you remove the override specifier? It compiles for me also with them. And they are certainly in no connection to this patch.

Kanedias marked an inline comment as done.Mar 21 2018, 9:30 AM

They don't override anything and compile fails for me if they are present. GCC 7.3.1.

They don't override anything and compile fails for me if they are present. GCC 7.3.1.

I believe this is an unrelated regression you just ran into because of D11209. I hadn't yet updated my KDecoration clone, that's why it still compiled for me. In any case please remove the unrelated override keyword removal from the patch and let's hope the regression will be quickly fixed in KDecoration.

I don't have KDecoration checked out. The problem is much more simple. These functions don't override anything but there's override keyword present where it shouldn't be. The fix is still required for them, regardless of KDecoration status.

Any other issues apart from this one? I'll remove these lines once I'm home.

They don't override anything and compile fails for me if they are present. GCC 7.3.1.

I believe this is an unrelated regression you just ran into because of D11209. I hadn't yet updated my KDecoration clone, that's why it still compiled for me. In any case please remove the unrelated override keyword removal from the patch and let's hope the regression will be quickly fixed in KDecoration.

No, D11209 should be innocent here. Not exporting the symbols of the pimpl classes has no effect on the virtualness of the methods of the normal classes.

The issue is rather that those tooltip-related methods only got added recently to master, see D7246. So KWin master also expects KDecoration master.

This makes sense. Thanks, will try with KDecoration master once I'm home

Kanedias updated this revision to Diff 30189.Mar 22 2018, 6:40 AM

Remove override fix

romangg accepted this revision.Mar 25 2018, 5:04 PM

Looks fine to me. Tested runtime together with your KWayland patch.

Probably you know this, but please push as one commit only to master.

plugins/platforms/drm/egl_gbm_backend.cpp
156 ↗(On Diff #25979)
This revision is now accepted and ready to land.Mar 25 2018, 5:04 PM
davidedmundson accepted this revision.Mar 25 2018, 5:13 PM
Kanedias closed this revision.Mar 25 2018, 5:18 PM

Done! Thanks for looking this through.

@bcooksley this file was merged in D1231

I'll take a look once I'm home

bshah added a subscriber: bshah.Mar 26 2018, 7:29 AM

I'll take a look once I'm home

I've fixed the issue on git master so don't worry.