Add Remote Access interface to KWayland
ClosedPublic

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

Details

Summary

This commit adds an interface bridge from KWin to KRfb. The purpose of
this protocol is to pass a GBM fd of currently displayed buffer from
KWin. The buffer is expected to be fully drawn once it is passed.

Related to D1230

Test Plan
  • Start testing of RemoteAccessTest *****

Config: Using QtTest library 5.6.0, Qt 5.6.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 6.1.1 20160501)
PASS : RemoteAccessTest::initTestCase()
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-client: Connected to Wayland server at: "kwayland-test-remote-access-0"
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-client: Wayland Interface: wl_shm / 1 / 1
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-client: Wayland Interface: org_kde_kwin_remote_access_manager / 2 / 1
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-server: Server buffer sent: fd 15
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-client: Got buffer, server fd: 15
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-server: Remote buffer returned, client 4 , id 0 , fd 15
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-server: Buffer released, fd 15
QDEBUG : RemoteAccessTest::testSendReleaseSingle() kwayland-client: Buffer released
PASS : RemoteAccessTest::testSendReleaseSingle()
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Connected to Wayland server at: "kwayland-test-remote-access-0"
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Wayland Interface: wl_shm / 1 / 1
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Wayland Interface: org_kde_kwin_remote_access_manager / 2 / 1
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-server: Server buffer sent: fd 15
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Got buffer, server fd: 15
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Got buffer, server fd: 15
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-server: Remote buffer returned, client 4 , id 0 , fd 15
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-server: Remote buffer returned, client 5 , id 0 , fd 15
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-server: Buffer released, fd 15
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Buffer released
QDEBUG : RemoteAccessTest::testSendReleaseMultiple() kwayland-client: Buffer released
PASS : RemoteAccessTest::testSendReleaseMultiple()
QDEBUG : RemoteAccessTest::testSendClientGone() kwayland-client: Connected to Wayland server at: "kwayland-test-remote-access-0"
QDEBUG : RemoteAccessTest::testSendClientGone() kwayland-client: Wayland Interface: wl_shm / 1 / 1
QDEBUG : RemoteAccessTest::testSendClientGone() kwayland-client: Wayland Interface: org_kde_kwin_remote_access_manager / 2 / 1
QDEBUG : RemoteAccessTest::testSendClientGone() kwayland-server: Server buffer sent: fd 15
QDEBUG : RemoteAccessTest::testSendClientGone() kwayland-server: Buffer released, fd 15
PASS : RemoteAccessTest::testSendClientGone()
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-client: Connected to Wayland server at: "kwayland-test-remote-access-0"
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-client: Wayland Interface: wl_shm / 1 / 1
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-client: Wayland Interface: org_kde_kwin_remote_access_manager / 2 / 1
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-server: Server buffer sent: fd 15
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-client: Got buffer, server fd: 15
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-client: Buffer released
QDEBUG : RemoteAccessTest::testSendReceiveClientGone() kwayland-server: Buffer released, fd 15
PASS : RemoteAccessTest::testSendReceiveClientGone()
PASS : RemoteAccessTest::cleanupTestCase()
Totals: 6 passed, 0 failed, 0 skipped, 0 blacklisted

  • Finished testing of RemoteAccessTest *****

Diff Detail

Repository
R127 KWayland
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
Kanedias added inline comments.Jun 13 2016, 2:32 PM
autotests/client/test_remote_access.cpp
162

Got it

Kanedias updated this revision to Diff 4401.Jun 13 2016, 2:33 PM
Kanedias edited the test plan for this revision. (Show Details)
Kanedias marked 12 inline comments as done.

Review comments & cleanup

I just realized a possible problem: multi-screen. On multi-screen we have one buffer for each screen. But how does the client know for which screen the buffer is. I think we need to somehow pass the information for which wl_output the buffer is.

I had been thinking about multi-screen issue and how we can get it working in the protocol. The biggest problem is that we cannot really map to the wl_output in a way that it's useful to the client. Also caused by QtWayland not exposing the wl_output in the native interface. From server side we could send a wl_output resource of that client, but our Qt based clients would not know what to do with them :-(

Given that we need to have the client tell the server for which wl_output it wants to have the buffer. A possibility would be to pass the wl_output as argument to the get_buffer request. But then how would the buffer_ready event indicate for which wl_output it is? Maybe we need to do it like the with org_kde_kwin_dpms_manager. It would require to add another level of indirection. Which is nothing I want as it just sounds too complicated and requires quite some changes to the otherwise finished review here. Le sigh. I wish I had noticed that problem earlier. It's something one only notices when using Wayland in day-to-day with multi-screen setup.

Can't we pass screen index along with all the invocations? Krfb (and other recording tools) will know the screen configuration as they reside in same wayland session. They'll get the buffer and the screen index and will know exactly what to map. Am I missing something here?

Besides, I didn't find any mentions of multi-screen capabilities in Krfb at all. It currently works like this:

d->framebufferImage = XGetImage(QX11Info::display(),
                                id,
                                0,
                                0,
                                QApplication::desktop()->width(),
                                QApplication::desktop()->height(),
                                AllPlanes,
                                ZPixmap);

If that's the requirement, there will be huge amount of work to implement it from ground up.
Patchset for KRfb is already enormous and rewrites half of the input system into plugins instead of built-in libraries (to integrate it with fake-input). I doubt it will endure another set of additions, the review will take forever.
I think we should implement screen indexing in protocol but start with passing screen №1 only for now.

Can't we pass screen index along with all the invocations?

what is a screen index? You mean the id of the global referencing the wl_output?

Krfb (and other recording tools) will know the screen configuration as they reside in same wayland session. They'll get the buffer and the screen index and will know exactly what to map. Am I missing something here?

That might be a race condition. What if the output got removed? On the other hand if the events are queued, it might work.

Besides, I didn't find any mentions of multi-screen capabilities in Krfb at all. It currently works like this:

d->framebufferImage = XGetImage(QX11Info::display(),
                                id,
                                0,
                                0,
                                QApplication::desktop()->width(),
                                QApplication::desktop()->height(),
                                AllPlanes,
                                ZPixmap);

If that's the requirement, there will be huge amount of work to implement it from ground up.

Right I see the problem. On X11 of course there is just one virtual screen for all outputs. Thus krfb just always get the whole screen. On Wayland we have the problem that the compositor is rendering to each output individually. So we end up with a buffer for each output. Argh that sucks. I'm not seeing a solution for it right now and would say just ignore it for the moment. Either we only support one output at the start or combine the image of all outputs to one.

Also caused by QtWayland not exposing the wl_output in the native interface.

This might have been true at the time of writing,. It's not the case now.

nativeResourceForScreen will return a wl_output, we can loop through them, and then match the wl_output ID to a buffer here.

This revision now requires changes to proceed.May 30 2017, 8:19 AM

Seaprate question,
In wl_surface when we attach a new buffer we also mark what areas are damaged.

Here we're passing an even bigger buffer.

Would it benefit from a series of damage events being sent in the org_kde_kwin_remote_buffer before the gbm_handle event?
Kwin should have all this information available. Or is it best for VNC do that sort of thing itself?

src/client/protocols/remote-access.xml
24

Assuming we do output merging in the client we want an extra arg with the wl_output here.

We don't need want it in the request, as each output will get a different buffer and therefore a different ID.

Kanedias updated this revision to Diff 15125.Jun 3 2017, 10:04 PM
Kanedias edited edge metadata.
Kanedias added a reviewer: davidedmundson.

Rebased the protocol against latest KWayland branch.
Will update on the comments this week.

Restricted Application added a project: Frameworks. · View Herald TranscriptJun 3 2017, 10:04 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript

Would it benefit from a series of damage events being sent in the org_kde_kwin_remote_buffer before the gbm_handle event?
Kwin should have all this information available. Or is it best for VNC do that sort of thing itself?

I wanted to implement dumb buffer retrieval from KWin and passing as a first step and then to implement damage.
Or do you think we should stabilize protocol now? I see KRfb X11 plugin indeed waits for XDamage events and adds them to query in FrameBuffer::modifiedTiles later.

src/client/protocols/remote-access.xml
24

Gotcha, will do

@davidedmundson , do we really have nativeResourceForScreen call? AFAIK KWin uses its own QPA, which implements only bits of functionality. We need to have nativeResourceForScreen to be able to pass wl_output, should I patch QPA also, or is there better way?

Kanedias updated this revision to Diff 15553.Jun 18 2017, 9:17 PM

Fixed KWAYLAND -> Kwayland in CmakeLists.txt

We don't want or need to go via the QPA on the server side. The wayland specific output management is in the DRM plugin, your code is in the DRM plugin. It should be simple.

How do I get ID from wl_output interface? I kinda got how I can get the needed info from DrmOutput instance, but not sure how to compare them on server and client side

Kanedias updated this revision to Diff 16318.Jul 7 2017, 6:24 PM

Added wl_output reference for buffers as requested by @davidedmundson and @graesslin

I'll get rid of the fakeinput-related changes and test it with KRfb tomorrow.

@davidedmundson , can you test this with multiple outputs?

Kanedias updated this revision to Diff 16359.Jul 8 2017, 3:58 PM

some -> const modifiers and typos

@davidedmundson , @graesslin , I cleaned up fake-input handling, fixed autotests.

Tested this manually with patched KWin and KRfb version - all works fine (only one screen though).
I'm able to retrieve wl_output from native interface as David suggested.

Regarding damage regions - I don't quite see how this works along with GBM buffer passing (as far as I can see we have only whole screens as GBM BOs in KWin,
but I may be missing something.

Btw, should I bump patches to KWin/KRfb to match this version?

Gentle reminder

All good to me. But double check with Martin.

@graesslin this is a good week for merging bit stuff as the last frameworks was just released.

Martin, I explicitly asked you to look at this on Monday.

It's being really unfair to a new contributor to make them jump through all sorts of hoops to do things the way you want them, and leave them hanging for literally over a year.

...
Besides, I didn't find any mentions of multi-screen capabilities in Krfb at all. It currently works like this:

d->framebufferImage = XGetImage(QX11Info::display(),
                                id,
                                0,
                                0,
                                QApplication::desktop()->width(),
                                QApplication::desktop()->height(),
                                AllPlanes,
                                ZPixmap);

If that's the requirement, there will be huge amount of work to implement it from ground up.
Patchset for KRfb is already enormous and rewrites half of the input system into plugins instead of built-in libraries (to integrate it with fake-input). I doubt it will endure another set of additions, the review will take forever.
I think we should implement screen indexing in protocol but start with passing screen №1 only for now.

Speaking about krfb, after D5211 X11 plugin (and XGetImage code) does not exist anymore. And krfb is aware of having multiple screens, but it shares only primary screen area:

I cannot imagine how VNC server application can properly serve multiple monitors at once, especially if they have different resolutions (merge them into one big image covering all monitors at once, with black border around the smaller one?). So I think if krfb will ever support multiple monitors explicitly, there will be a combo box to select which screen to share.

Do you think other screen recording applications will need to capture several monitors at once?

Is this mergeable? @graesslin?

romangg added inline comments.
src/server/remote_access_interface.cpp
207

Can a rogue client do it though? This would crash the server then?

Kanedias added inline comments.Oct 23 2017, 12:53 PM
src/server/remote_access_interface.cpp
207

Can a rogue client do it though? This would crash the server then?

Yes, I guess so... What would you propose? Should we send it only to first bound? Or last one?

P.S. Even more: this interface has no authentication/authorization at all, so any client can connect and steal our video buffers.
Martin said that first version of protocol can go without it and we can readd it later (as with fakeinput protocol).

romangg added inline comments.Oct 23 2017, 1:06 PM
src/server/remote_access_interface.cpp
207

Only first bound like you do it now. Just remove the Q_ASSERT (and make sure boundScreens.size() >= 1, otherwise continue).

no authentication/authorization at all

That's a generic problem yet to be solved on Wayland / the Linux desktop. This also correlates with the push to containerized apps. I would just want something like the permission system in Android, but there might be better solutions. It's a bigger project for sure.

Also see here for some early thoughts on it, which to my knowledge until now did not lead to anything more: http://www.mupuf.org/blog/2014/02/19/wayland-compositors-why-and-how-to-handle/

Kanedias updated this revision to Diff 21212.Oct 24 2017, 6:43 AM

Updated against latest master,
fixed review comments

Kanedias marked 5 inline comments as done.Oct 24 2017, 6:45 AM

What's the status of this? Are we waiting for something other than @graesslin's review?

@ngraham, yes, he didn't review this after changes were made

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

Kanedias updated this revision to Diff 25976.Jan 26 2018, 6:12 AM
Kanedias edited the test plan for this revision. (Show Details)
  • Add RemoteAccess interface to KWayland
  • Merge branch
  • Fix compilation warnings uint -> int

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

It will soon be this patch 2nd birthday. Can we speed up things a bit? I've seen this is scheduled for Plasma 5.13, would be good if we have time to test it.

Please rebase onto master (or if this leads to problems with your remote merge master).

As I don't see anything related to security in this patch, I have two questions.

Could anyone with access to server:port manage the server wayland sessions or just create a new session?
The access control should be done in the firewall?

As I don't see anything related to security in this patch, I have two questions.

Could anyone with access to server:port manage the server wayland sessions or just create a new session?
The access control should be done in the firewall?

What port? This patch doesn't expose any port.
No, nobody can manage server sessions with this protocol.

romangg added inline comments.Mar 22 2018, 5:50 PM
autotests/client/CMakeLists.txt
430

add_test(NAME kwayland-testRemoteAccess COMMAND testRemoteAccess)

otherwise ctest doesn't find the test.

src/server/remote_access_interface.cpp
207
Kanedias updated this revision to Diff 30271.Mar 23 2018, 6:40 AM
  • Implement releasing of client-freed output
  • Review fixes
Kanedias updated this revision to Diff 30272.Mar 23 2018, 6:41 AM

Remove already merged changes

Kanedias updated this revision to Diff 30273.Mar 23 2018, 6:43 AM
Kanedias marked an inline comment as done.
  • Merge branch 'master' into gbm-vnc
Kanedias marked an inline comment as done.Mar 23 2018, 6:44 AM
romangg accepted this revision.Mar 25 2018, 5:04 PM

Looks fine to me. Tested compilation as well as runtime with KWin's DRM+EGL backend.

  • Please change in comments the frameworks version number these changes are landing in.
  • I would like to have @davidedmundson look over this patch a last time. If he doesn't have time until mid-week to do this, you can push.
  • Probably you know this, but please push as one commit only.
src/client/remote_access.cpp
140

rm whitespace

src/server/remote_access_interface.cpp
119

rm whitespace

src/server/remote_access_interface.h
39

rm whitespace (at the end)

43

rm whitespace

56

rm whitespace

src/server/remote_access_interface_p.h
30

rm whitespace

davidedmundson accepted this revision.Mar 25 2018, 5:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 25 2018, 5:18 PM
This revision was automatically updated to reflect the committed changes.
  • squashed before pushing
  • fixed all versions to 5.45

Thanks gentlemen, it's was 1 day more for the second birthday of this patch :)