D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, ragreen, schernikov, michaelh, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  https://phabricator.kde.org/D1231?vs=30273=30534#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1231?vs=30273=30534

REVISION DETAIL
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, ragreen, schernikov, michaelh, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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 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.

INLINE COMMENTS

> remote_access.cpp:139
> +RemoteBuffer *q;
> +
> +qint32 fd = 0;

rm whitespace

> remote_access_interface.cpp:118
> +};
> +
> +class RemoteAccessManagerInterface::Private : public Global::Private

rm whitespace

> remote_access_interface.h:38
> + * (stored in manager's sent list)
> + *  2. Clients confirm that they wants this buffer, the RemoteBuffer 
> + * interfaces are then created and wrapped around BufferHandle.

rm whitespace (at the end)

> remote_access_interface.h:42
> + * RemoteBuffer notifies manager and release signal is emitted.
> + * 
> + * It's the responsibility of your process to delete this BufferHandle

rm whitespace

> remote_access_interface.h:55
> +void setFormat(quint32 format);
> +  
> +qint32 fd() const;

rm whitespace

> remote_access_interface_p.h:29
> +{
> +
> +/**

rm whitespace

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, ragreen, schernikov, michaelh, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/output.cpp
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/output_interface.cpp
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/output.cpp
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/output_interface.cpp
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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 kwayland-testRemoteAccess COMMAND testRemoteAccess)`

otherwise ctest doesn't find the test.

> remote_access_interface.cpp:206
> +// clients don't necessarily bind outputs
> +if (boundScreens.isEmpty())
> +return;

Use braces: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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 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.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Jaime Torres Amate]

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 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jtamate, jgrulich, romangg, ngraham, alexeymin, #frameworks, 
davidedmundson, plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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, #frameworks, davidedmundson, 
plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, davidedmundson, romangg
Cc: jgrulich, romangg, ngraham, alexeymin, #frameworks, davidedmundson, 
plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, romangg, ngraham, alexeymin, #frameworks, davidedmundson, 
plasma-devel, schernikov, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Jan Grulich]

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, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, schernikov, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  https://phabricator.kde.org/D1231?vs=21212=25976

BRANCH
  gbm-vnc

REVISION DETAIL
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson, romangg
Cc: jgrulich, romangg, ngraham, alexeymin, #frameworks, davidedmundson, 
plasma-devel, schernikov, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Nathaniel Graham]

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, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Nathaniel Graham]

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, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson
Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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: 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).

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/

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, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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 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).

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, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

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]);

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

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, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Nathaniel Graham]

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, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Alexey Min]

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(),
  >   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 https://phabricator.kde.org/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:
  
  - xcb plugin: 
https://cgit.kde.org/krfb.git/tree/framebuffers/xcb/xcb_framebuffer.cpp#n171
  - qt plugin: 
https://cgit.kde.org/krfb.git/tree/framebuffers/qt/qtframebuffer.cpp#n81
  
  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?

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson
Cc: alexeymin, #frameworks, davidedmundson, plasma-devel, leezu, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[David Edmundson]

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 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, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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.
  
  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.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/fakeinput.cpp
  src/client/fakeinput.h
  src/client/protocols/fake-input.xml
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/fakeinput_interface.cpp
  src/server/fakeinput_interface.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt
  src/tools/testserver/testserver.cpp

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[David Edmundson]

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: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/fakeinput.cpp
  src/client/fakeinput.h
  src/client/protocols/fake-input.xml
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/fakeinput_interface.cpp
  src/server/fakeinput_interface.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt
  src/tools/testserver/testserver.cpp

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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?

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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. 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.

INLINE COMMENTS

> davidedmundson wrote in remote-access.xml:23
> 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.

Gotcha, will do

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

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 the comments this week.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1231?vs=4401=15125

REVISION DETAIL
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/fakeinput.cpp
  src/client/fakeinput.h
  src/client/protocols/fake-input.xml
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/fakeinput_interface.cpp
  src/server/fakeinput_interface.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt
  src/tools/testserver/testserver.cpp

To: Kanedias, graesslin, davidedmundson
Cc: #frameworks, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, 
hein, lukas

D1231: Add Remote Access interface to KWayland

[David Edmundson]

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 gbm_handle event?
  Kwin should have all this information available. Or is it best for VNC do 
that sort of thing itself?

INLINE COMMENTS

> remote-access.xml:23
> +
> +
> +

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.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

D1231: Add Remote Access interface to KWayland

[David Edmundson]

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 can loop through them, 
and then match the wl_output ID to a buffer here.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

To: Kanedias, graesslin
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Martin Gräßlin]

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 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.

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, abetts, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Kanedias (Oleg Chernovskiy)]

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 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.

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, abetts, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Martin Gräßlin]

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 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.

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Martin Gräßlin]

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
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Kanedias (Oleg Chernovskiy)]

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
  https://phabricator.kde.org/D1231?vs=4387=4401

REVISION DETAIL
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Kanedias (Oleg Chernovskiy)]

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

> graesslin wrote in registry.h:144
> just a note: it's 5.24 since today...

Understood, changed

> graesslin wrote in remote_access.h:193
> hmm maybe parametersObtained?

Sounds reasonable, changed

> graesslin wrote in remote_access_interface.cpp:28
> you can just include "logging_p.h"

Noted, re-using categories from there.

> graesslin wrote in remote_access_interface.cpp:240
> qCDebug

Done

> graesslin wrote in remote_access_interface.cpp:263
> qCDebug

Done

> graesslin wrote in remote_access_interface.cpp:336
> you can use the new resourceDestroyedCallback in resource_p.h. It handles the 
> destroy correctly and that will trigger the unbind and deleteLater 
> automatically.

Reused, thanks

> graesslin wrote in remote_access_interface.cpp:346
> that would trigger a double delete as I had to learn very painfully lately.

Removed that code completely, thanks to `resourceDestroyedCallback`

> graesslin wrote in remote_access_interface.cpp:356-359
> you don't need that, it's already in Resource

Removed

> graesslin wrote in remote_access_interface.h:45
> Thinking out loud:
> 
> What if in future we pass non GbmBuffers? Should we then still call it 
> GbmBuffer or do we need a new class? Maybe we should make it a nested class 
> to RemoteAccessManagerInterface and then just call it Buffer? Or keep it 
> outside and call it more generic RemoteBuffer?

Yes, it will no longer be GBM. Seeing so much EGLStreams and Vulkan struggles 
around it would be good to rename it into something more generic... noted!

> graesslin wrote in remote_access_interface.h:77
> who's the owner of the GbmBuffer? Who will delete it?

The owner is the process that originally passed it. So KWin here. In my 
non-submitted PoC from KWin side it closes its fd and deletes it after 
`bufferReleased` call

> graesslin wrote in remote_access_interface_p.h:33
> could you please add a note about that it's an internal class

Done, improved Doxygen comment also

> graesslin wrote in remote_access_interface_p.h:58
> nitpick

Sure, fixed

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Martin Gräßlin]

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 actually had. With QVERIFY you only see that it failed

> registry.h:144
> +TextInputManagerUnstableV2, ///< Refers to 
> zwp_text_input_manager_v2, @since 5.23
> +RemoteAccessManager ///< Refers to 
> org_kde_kwin_remote_access_manager interface, @since 5.23
>  };

just a note: it's 5.24 since today...

> remote_access.cpp:66
> +rbuf->setup(requested);
> +qDebug() << "Client: Got buffer, server fd:" << buffer_id;
> +

qCDebug please

> remote_access.h:193
> +Q_SIGNALS:
> +void paramsObtained();
> +

hmm maybe parametersObtained?

> remote_access_interface.cpp:28
> +
> +#include 
> +#include 

you can just include "logging_p.h"

> remote_access_interface.cpp:240
> +
> +qDebug() << "Server: remote buffer returned, client" << 
> wl_resource_get_id(resource)
> + << ", id" << rbuf->id()

qCDebug

> remote_access_interface.cpp:263
> +// no more clients using this buffer
> +qDebug() << "Server: buffer released, fd" << bh.buf->fd();
> +emit q->bufferReleased(bh.buf);

qCDebug

> remote_access_interface.cpp:336
> +const struct org_kde_kwin_remote_buffer_interface 
> RemoteBufferInterface::Private::s_interface = {
> +releaseCallback
> +};

you can use the new resourceDestroyedCallback in resource_p.h. It handles the 
destroy correctly and that will trigger the unbind and deleteLater 
automatically.

> remote_access_interface.cpp:346
> +
> +p->q->deleteLater(); // also purges it from manager's list
> +}

that would trigger a double delete as I had to learn very painfully lately.

> remote_access_interface.cpp:356-359
> +if (resource) {
> +wl_resource_destroy(resource);
> +resource = nullptr;
> +}

you don't need that, it's already in Resource

> remote_access_interface.h:45
> + **/
> +class KWAYLANDSERVER_EXPORT GbmBuffer
> +{

Thinking out loud:

What if in future we pass non GbmBuffers? Should we then still call it 
GbmBuffer or do we need a new class? Maybe we should make it a nested class to 
RemoteAccessManagerInterface and then just call it Buffer? Or keep it outside 
and call it more generic RemoteBuffer?

> remote_access_interface.h:77
> + **/
> +void sendBufferReady(const GbmBuffer *buf);
> +/**

who's the owner of the GbmBuffer? Who will delete it?

> remote_access_interface_p.h:33
> + * @brief Holds data of passed buffer and client resource. Also controls 
> buffer lifecycle.
> + */
> +class RemoteBufferInterface : public Resource

could you please add a note about that it's an internal class

> remote_access_interface_p.h:58
> +#endif
> \ No newline at end of file


nitpick

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Kanedias (Oleg Chernovskiy)]

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
  https://phabricator.kde.org/D1231?vs=4337=4387

REVISION DETAIL
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Kanedias (Oleg Chernovskiy)]

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 
> server can destroy the resource).

Added destructor and release callback.

> graesslin wrote in remote-access.xml:33
> so the idea here would be that if in future we want to support other buffers 
> (e.g. shared mem) we just add a new event?

Yes, with appropriate "since" clause

> graesslin wrote in remote-access.xml:41
> Just for thought: in other interfaces the destructor is mostly called 
> "release"

Renamed

> graesslin wrote in registry.h:128
> please add as last interface otherwise it breaks API

Moved to last in enum. Or did you mean in forward references declaration too?..

> graesslin wrote in registry.h:379
> we moved to frameworks which means we are now at 5.23 - sorry about that. I 
> also had to rename a bunch of @since 5.7 ;-)

Corrected

> graesslin wrote in remote_access_interface.cpp:146-149
> if really only one client should be allowed (why?) it would be better to send 
> a dedicated error state to inform it instead of "abusing" no memory.

Added ability to have multiple clients in the same time

> graesslin wrote in remote_access_interface.cpp:158
> this allows to have only one client bind it. As soon as a second client binds 
> the protocol it will get overwritten and breaks the existing one.
> 
> I think you need a QVector here.

Reimplemented

> graesslin wrote in remote_access_interface.h:46
> For ABI compatibility it's better to only have d-ptr-ized classes/structs in 
> the public header.

Implemented GbmBuffer with d-pointer

> graesslin wrote in remote_access_interface.h:49-50
> why do we need gbm_surface and gbm_bo? This looks like adding not needed 
> details to the interface

Removed, reimplemented

> graesslin wrote in remote_access_interface.h:43
> for ABI compatibility we cannot have structs defined in the header. See 
> https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts
>  for explanation. Especially the last point of the Donts is relevant as it 
> means we are never able to change this again.
> 
> Thus I think the proper way has to be:
> 
>   class Buffer
>   {
>   public:
>   void setFd(int qint32);
>   void setSize(const QSize );
>   void setStride(qint32 stride);
>   enum class Format {
>   Format1,
>   Format2
>   };
>   void setFormat(Format format);
>   
>   private:
>   class Private;
>   QScopedPointer d;
>   };
> 
> and then in the Implementation have the Private class which holds the data 
> members.

Thanks for clarification.
Reimplemented.

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Kanedias (Oleg Chernovskiy)]

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 Wayland; removed Plasma.


  Now server can send buffers to multiple clients
  
  + GbmBuffer now has d-pointer
  + added autotests with edge cases

REPOSITORY
  rKWAYLAND KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1231?vs=4100=4337

REVISION DETAIL
  https://phabricator.kde.org/D1231

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_remote_access.cpp
  src/client/CMakeLists.txt
  src/client/protocols/remote-access.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/remote_access.cpp
  src/client/remote_access.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/remote_access_interface.cpp
  src/server/remote_access_interface.h
  src/server/remote_access_interface_p.h
  src/tools/mapping.txt

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel