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
graesslin | |
davidedmundson | |
romangg |
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
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
Lint Skipped |
Unit Tests Skipped |
autotests/client/test_remote_access.cpp | ||
---|---|---|
161 ↗ | (On Diff #4387) | Got it |
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.
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.
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 ↗ | (On Diff #4100) | 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. |
Rebased the protocol against latest KWayland branch.
Will update on the comments this week.
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 ↗ | (On Diff #4100) | 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?
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
I'll get rid of the fakeinput-related changes and test it with KRfb tomorrow.
@davidedmundson , can you test this with multiple outputs?
@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.
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.
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?
src/server/remote_access_interface.cpp | ||
---|---|---|
207 ↗ | (On Diff #4100) | Can a rogue client do it though? This would crash the server then? |
src/server/remote_access_interface.cpp | ||
---|---|---|
207 ↗ | (On Diff #4100) |
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. |
src/server/remote_access_interface.cpp | ||
---|---|---|
207 ↗ | (On Diff #4100) | Only first bound like you do it now. Just remove the Q_ASSERT (and make sure boundScreens.size() >= 1, otherwise continue).
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/ |
What's the status of this? Are we waiting for something other than @graesslin's review?
@graesslin, would you mind reviewing this so we can push forward with the feature? Thanks!
@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?
What port? This patch doesn't expose any port.
No, nobody can manage server sessions with this protocol.
Looks fine to me. Tested compilation as well as runtime with KWin's DRM+EGL backend.
src/client/remote_access.cpp | ||
---|---|---|
140 ↗ | (On Diff #4100) | rm whitespace |
src/server/remote_access_interface.cpp | ||
119 ↗ | (On Diff #4100) | rm whitespace |
src/server/remote_access_interface.h | ||
39 ↗ | (On Diff #4100) | rm whitespace (at the end) |
43 ↗ | (On Diff #4100) | rm whitespace |
56 ↗ | (On Diff #4100) | rm whitespace |
src/server/remote_access_interface_p.h | ||
29 ↗ | (On Diff #30273) | rm whitespace |
Thanks gentlemen, it's was 1 day more for the second birthday of this patch :)