D1231: Add Remote Access interface to KWayland

Kanedias added a comment. - squashed before pushing - fixed all versions to 5.45 Thanks gentlemen, it's was 1 day more for the second birthday of this patch :) REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson,

D1231: Add Remote Access interface to KWayland

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R127:5116fe0c6345: Add Remote Access interface to KWayland (authored by Kanedias). CHANGED PRIOR TO COMMIT

D1231: Add Remote Access interface to KWayland

romangg accepted this revision. romangg added a comment. 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

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 30273. Kanedias marked an inline comment as done. Kanedias added a comment. - Merge branch 'master' into gbm-vnc REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=30272=30273 BRANCH gbm-vnc REVISION DETAIL

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 30272. Kanedias added a comment. Remove already merged changes REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=30271=30272 BRANCH gbm-vnc REVISION DETAIL https://phabricator.kde.org/D1231 AFFECTED FILES

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 30271. Kanedias added a comment. - Implement releasing of client-freed output - Review fixes REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=25976=30271 BRANCH gbm-vnc REVISION DETAIL

D1231: Add Remote Access interface to KWayland

romangg added inline comments. INLINE COMMENTS > CMakeLists.txt:430 > +target_link_libraries( testRemoteAccess Qt5::Test Qt5::Gui > KF5::WaylandClient KF5::WaylandServer) > +add_test(kwayland-testRemoteAccess testRemoteAccess) > +ecm_mark_as_test(testRemoteAccess) `add_test(NAME

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. In D1231#224441 , @jtamate wrote: > 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

D1231: Add Remote Access interface to KWayland

jtamate added a comment. 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? REPOSITORY R127

D1231: Add Remote Access interface to KWayland

romangg added a comment. Please rebase onto master (or if this leads to problems with your remote merge master). REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson, romangg Cc: jgrulich, romangg, ngraham, alexeymin,

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. 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. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin,

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. @graesslin , @davidedmundson , please approve this once again, this was updated numerous times after initial review REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson, romangg Cc: jgrulich,

D1231: Add Remote Access interface to KWayland

jgrulich added a dependent revision: D10965: Add screen cast portal. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson, romangg Cc: jgrulich, romangg, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, schernikov,

D1231: Add Remote Access interface to KWayland

Kanedias added a task: T7785: PipeWire support in remote access to KWin. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson, romangg Cc: jgrulich, romangg, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel,

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 25976. Kanedias edited the test plan for this revision. Kanedias added a comment. - Add RemoteAccess interface to KWayland - Merge branch - Fix compilation warnings uint -> int REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE

D1231: Add Remote Access interface to KWayland

romangg added a reviewer: romangg. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson, romangg Cc: jgrulich, romangg, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai,

D1231: Add Remote Access interface to KWayland

ngraham added a comment. @graesslin, would you mind reviewing this so we can push forward with the feature? Thanks! REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: romangg, ngraham, alexeymin, #frameworks,

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. @ngraham, yes, he didn't review this after changes were made REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: romangg, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, leezu,

D1231: Add Remote Access interface to KWayland

ngraham added a comment. What's the status of this? Are we waiting for something other than @graesslin's review? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: romangg, ngraham, alexeymin, #frameworks,

D1231: Add Remote Access interface to KWayland

Kanedias marked 5 inline comments as done. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed,

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 21212. Kanedias added a comment. Updated against latest master, fixed review comments REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=16359=21212 REVISION DETAIL https://phabricator.kde.org/D1231

D1231: Add Remote Access interface to KWayland

subdiff added a task: T5653: [kwin] Screen recording in Wayland session. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, leezu, ZrenBot, progwolff,

D1231: Add Remote Access interface to KWayland

subdiff added inline comments. INLINE COMMENTS > Kanedias wrote in remote_access_interface.cpp:206 > > 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:

D1231: Add Remote Access interface to KWayland

Kanedias added inline comments. INLINE COMMENTS > subdiff wrote in remote_access_interface.cpp:206 > Can a rogue client do it though? This would crash the server then? > Can a rogue client do it though? This would crash the server then? Yes, I guess so... What would you propose? Should we send

D1231: Add Remote Access interface to KWayland

subdiff added inline comments. INLINE COMMENTS > remote_access_interface.cpp:206 > +// no reason for client to bind wl_output multiple times > +Q_ASSERT(boundScreens.size() == 1); > +org_kde_kwin_remote_access_manager_send_buffer_ready(res, buf->fd(), > boundScreens[0]);

D1231: Add Remote Access interface to KWayland

ngraham added a comment. Is this mergeable? @graesslin? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai,

D1231: Add Remote Access interface to KWayland

alexeymin added a comment. In https://phabricator.kde.org/D1231#39191, @Kanedias wrote: > ... > 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(), >

D1231: Add Remote Access interface to KWayland

davidedmundson added a comment. 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. REPOSITORY R127

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. Gentle reminder REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: #frameworks, davidedmundson, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts,

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. Btw, should I bump patches to KWin/KRfb to match this version? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai,

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. @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.

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 16359. Kanedias added a comment. some -> const modifiers and typos REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=16318=16359 REVISION DETAIL https://phabricator.kde.org/D1231 AFFECTED FILES

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. I'll get rid of the fakeinput-related changes and test it with KRfb tomorrow. @davidedmundson , can you test this with multiple outputs? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc:

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 16318. Kanedias added a comment. Added wl_output reference for buffers as requested by @davidedmundson and @graesslin REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=15553=16318 REVISION DETAIL

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. 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 REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias,

D1231: Add Remote Access interface to KWayland

davidedmundson added a comment. 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. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To:

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 15553. Kanedias added a comment. Fixed KWAYLAND -> Kwayland in CmakeLists.txt REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D1231?vs=15125=15553 REVISION DETAIL https://phabricator.kde.org/D1231 AFFECTED FILES

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. @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?

D1231: Add Remote Access interface to KWayland

Kanedias added a dependent revision: D6186: Implement software cursor in OpenGL backend . REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr,

D1231: Add Remote Access interface to KWayland

Kanedias added a dependent revision: D6096: Add Wayland RemoteAccess capabilities to KRfb. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D1231 To: Kanedias, graesslin, davidedmundson Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff,

D1231: Add Remote Access interface to KWayland

Kanedias added a comment. In https://phabricator.kde.org/D1231#112772, @davidedmundson wrote: > > 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.

D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 15125. Kanedias added a reviewer: davidedmundson. Kanedias added a comment. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. Rebased the protocol against latest KWayland branch. Will update on

D1231: Add Remote Access interface to KWayland

davidedmundson added a comment. 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

D1231: Add Remote Access interface to KWayland

davidedmundson added a comment. This revision now requires changes to proceed. > 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

[Differential] [Commented On] D1231: Add Remote Access interface to KWayland

graesslin added a comment. In https://phabricator.kde.org/D1231#39191, @Kanedias wrote: > 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

[Differential] [Commented On] D1231: Add Remote Access interface to KWayland

Kanedias added a comment. 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

[Differential] [Commented On] D1231: Add Remote Access interface to KWayland

graesslin added a comment. 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

[Differential] [Commented On] D1231: Add Remote Access interface to KWayland

graesslin added a comment. 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. REPOSITORY

[Differential] [Updated, 1,458 lines] D1231: Add Remote Access interface to KWayland

Kanedias updated the test plan for this revision. Kanedias updated this revision to Diff 4401. Kanedias marked 12 inline comments as done. Kanedias added a comment. Review comments & cleanup REPOSITORY rKWAYLAND KWayland CHANGES SINCE LAST UPDATE

[Differential] [Updated] D1231: Add Remote Access interface to KWayland

Kanedias marked 12 inline comments as done. Kanedias added inline comments. INLINE COMMENTS > graesslin wrote in test_remote_access.cpp:161 > use QCOMPARE to test two values. If it fails with QCOMPARE you get the values > it actually had. With QVERIFY you only see that it failed Got it >

[Differential] [Commented On] D1231: Add Remote Access interface to KWayland

graesslin added inline comments. INLINE COMMENTS > test_remote_access.cpp:161 > +// client fd is different, not subject to check > +QVERIFY(rbuf->width() == 50); > +QVERIFY(rbuf->height() == 50); use QCOMPARE to test two values. If it fails with QCOMPARE you get the values it

[Differential] [Updated, 1,473 lines] D1231: Add Remote Access interface to KWayland

Kanedias updated this revision to Diff 4387. Kanedias marked 8 inline comments as done. Kanedias added a comment. Changed getter methods to public as we'll need them from KWin side. + Doxygen typo REPOSITORY rKWAYLAND KWayland CHANGES SINCE LAST UPDATE

[Differential] [Updated] D1231: Add Remote Access interface to KWayland

Kanedias marked 8 inline comments as done. Kanedias added inline comments. INLINE COMMENTS > graesslin wrote in remote-access.xml:19 > I noticed that standard Wayland protocols also have a destructor request for > the manager interface. I'd suggest to also add it (it makes sense as then the >

[Differential] [Updated, 1,473 lines] D1231: Add Remote Access interface to KWayland

Kanedias retitled this revision from "Add Krfb interface to KWayland" to "Add Remote Access interface to KWayland". Kanedias updated the test plan for this revision. Kanedias updated this revision to Diff 4337. Kanedias added a comment. Restricted Application edited projects, added Plasma on