D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  > I hope you don't mind that.
  
  On the contrary, I'm very glad it was picked up and even got to some mass 
media :)
  Sure, just ping me for review, I have a vacation in October.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  In D6096#333791 , @Kanedias wrote:
  
  > @jgrulich In my regularly-irregular wanderings I noticed you already picked 
up changes from this diff and that's totally okay. Should I close this?
  
  
  I started looking into this because I work now on the remote desktop support 
in xdg-desktop-portal-kde and the best way how to implement it is to have 
support in some application. I forked your branch and added support for 
PipeWire 0.2.x and started rewriting it to use remote desktop portal. I hope 
you don't mind that. Feel free to close this one and we can later open a new 
review with full remote desktop support.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @jgrulich In my regularly-irregular wanderings I noticed you already picked 
up changes from this diff and that's totally okay. Should I close this?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  Now I realized what you mean, you mean that I didn't change libspa version as 
I did for pipewire, right? That hasn't change in released PW tarballs, but it's 
already changed now in master (Wim started versioning it) so I will have to fix 
this as well.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  In D6096#315732 , @Kanedias wrote:
  
  > @jgrulich you didn't change FindSPA, is it ok?
  
  
  I assume it is correct now, I got some fixes from Christophe Giboudeaux.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @jgrulich you didn't change FindSPA, is it ok?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @jgrulich thanks, I'll definitely use that. Have my hands fullatm but will 
definitely return to this diff and refresh once have some free time

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  There is now new release of PipeWire (version 0.2.0), which breaks API a bit. 
I would suggest to bump PipeWire requirements to 0.2.0 and change your code. 
There is also now a version file included so in future we can use this to check 
PW version during build time and support more versions of the code. 
Unfortunately this header file was not included before and thus you cannot use 
it now, you would need to do the check in CMake in case you would want to 
support both PW 0.1.9 and PW 0.2.0. I'll be changing my code in 
xdg-desktop-portal-kde today, trying to support both in stable and only new PW 
0.2.0 in master.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  One more possible thing to improve, I don't know if there is interest in 
supporting Gnome, but Gnome creates streams in BGRx format, which means it 
wouldn't mach your stream, if you want to support it you need to change it to 
something like :
  ":",  pwType->format_video.format, "Ieu", pwType->video_format.RGBx, 
SPA_POD_PROP_ENUM(2, pwType_->video_format.RGBx,  pwType_->video_format.BGRx)

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @jgrulich thanks a lot! I'll address these on weekend

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  I have two things I observed when implementing same in WebRTC to have this 
working in Firefox and Chrome:
  
  1. Please use "Fru" for max_framerate and "Rru" for size, as it allows some 
negotiation when matching streams, I used the same in xdg-desktop-portal-kde 
now and in WebRTC and same will be used in Mutter
  2. Does it always stop streaming in x-d-p-kde when you stop receiving stream 
in KRfb? I've been experiencing this in WebRTC where when I disconnected my 
consuming stream, it still kept streaming in x-d-p-kde which you can see in the 
log. I solved this by calling Session->Close() in WebRTC and stop streaming in 
x-d-p-kde when the session has been closed.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> pw_framebuffer.h:27
> +PWFrameBuffer(WId winid, QObject *parent = nullptr);
> +virtual ~PWFrameBuffer() override;
> +

`virtual` is redundant. (also, it doesn't make sense to have both `virtual` and 
`override`)

> pw_framebufferplugin.h:36
> +   PWFrameBufferPlugin(QObject *parent, const QVariantList &args);
> +   virtual ~PWFrameBufferPlugin() override;
> +

`virtual` is redundant.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: zzag, jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, 
mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @alexeymin This is a working version but I plan on adding stuff in the coming 
weekends. Just wanted to make sure there's someone motivated to review and 
merge this.
  Thanks for looking at my diffs.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  1. Make sure you have latest versions of KWin, XDG-KDE and Pipewire installed
  2. Services should be started by DBus-activation, if that doesn't happen by 
whatever reason, first start `$ pipewire`, then `$ 
/usr/lib(exec)/xdg-desktop-portal`, it should in turn start `$ 
/usr/lib(exec)/xdg-desktop-portal-kde` by itself
  3. Compile KRfb with this patch and install it, select "pw" in the list of 
KRfb plugins. If all is ok, XDP-KDE should show you the dialog with screen 
selection
  4. Try to connect to KRfb through KRDC or any other VNC client
  
  Pay attention to logs, in case of difficulties use pipewire-monitor, 
qdbusviewer and launch desktop portals with `-v` options

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Alexey Min]

alexeymin added a comment.


  How exactly I can test if something works? And what are the requirements? 
PipeWire? Something else?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin, #plasma, #kde_applications
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @jgrulich @alexeymin  please review new framebuffer approach
  
  This is the first draft of the implementation. I intend to add more features 
and fix review comments in the process.
  
  Missing features:
  
  - Only knows about first screen for now
  - Doesn't fail if D-Bus is unavailable
  - Read-only

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 34665.
Kanedias edited the summary of this revision.
Kanedias added a comment.


  Rewrite everything from scratch with Pipewire connectivity in mind

REPOSITORY
  R437 Desktop Sharing

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6096?vs=15147&id=34665

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindPipeWire.cmake
  cmake/modules/FindSPA.cmake
  framebuffers/CMakeLists.txt
  framebuffers/pipewire/CMakeLists.txt
  framebuffers/pipewire/krfb_framebuffer_pw.json
  framebuffers/pipewire/pw_framebuffer.cpp
  framebuffers/pipewire/pw_framebuffer.h
  framebuffers/pipewire/pw_framebufferplugin.cpp
  framebuffers/pipewire/pw_framebufferplugin.h
  framebuffers/pipewire/xdp_dbus_interface.xml

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @jgrulich , thanks for the pointers

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  Ah, that's because I use custom built XDP-KDE executable, installed to prefix 
instead of /usr

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  Nailed it, XDP just thinks KDE is inferior:
  
XDP: loading /usr/share/xdg-desktop-portal/portals/kde.portal
XDP: portal implementation for KDE
XDP: portal implementation supports org.freedesktop.impl.portal.Access
XDP: portal implementation supports org.freedesktop.impl.portal.AppChooser
XDP: portal implementation supports org.freedesktop.impl.portal.Email
XDP: portal implementation supports org.freedesktop.impl.portal.FileChooser
XDP: portal implementation supports org.freedesktop.impl.portal.Inhibit
XDP: portal implementation supports org.freedesktop.impl.portal.Notification
XDP: portal implementation supports org.freedesktop.impl.portal.Print
  
  For GNOME it mentions ScreenCast though, interesting

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  In D6096#263873 , @Kanedias wrote:
  
  > Thanks for the xml, I was able to pass it to qdbus2cpp. Amusingly, if I 
just try `auto reply = dbusXdpService->CreateSession(QVariantMap());` (with 
empty parameter map), the main desktop portal app just segfaults! I mean, not 
the kde impl one but the main XDG Desktop Portal!
  >  I'm fixing my parameters thanks to your test app, but... should that 
really happen?
  
  
  To be honest, GLib + DBus is a mystery for me, I don't really know how this 
is implemented, it generates lots of code from XML files where you can easilly 
get lost so I have no idea whether they check parameters or not. It's better to 
ask someone on #flatpak  channel if 
you really want to know what's going on, but I guess you shouldn't be able to 
call it with less parameters then you supposed to, right?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  In D6096#263885 , @Kanedias wrote:
  
  > Hmm XDG Desktop Portal 0.11 just simply doesn't have ScreenCast interface 
on DBus... @jgrulich do I need some bleeding edge version or is it not yet in 
master branch?
  
  
  ScreenCast interface is build only when PipeWire is available during the 
build.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  Hmm XDG Desktop Portal 0.11 just simply doesn't have ScreenCast interface on 
DBus... @jgrulich do I need some bleeding edge version or is it not yet in 
master branch?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  Thanks for the xml, I was able to pass it to qdbus2cpp. Amusingly, if I just 
try `auto reply = dbusXdpService->CreateSession(QVariantMap());` (with empty 
parameter map), the main desktop portal app just segfaults! I mean, not the kde 
impl one but the main XDG Desktop Portal!
  I'm fixing my parameters thanks to your test app, but... should that really 
happen?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  The Screencast portal DBus interface is here: 
https://github.com/flatpak/xdg-desktop-portal/blob/master/data/org.freedesktop.portal.ScreenCast.xml

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Jan Grulich]

jgrulich added a comment.


  In D6096#260229 , @Kanedias wrote:
  
  > @jgrulich I started to hack together a very simple 
krfb-with-dbus-with-xdp-kde-with-pipewire... thingie.
  >  Now I'm curious, you have `handle` and `session_handle` paths everywhere 
in Screencast interface inside XDP-KDE source. Why? I can understand why 
`session_handle` is there - to create and track sessions of course, but what is 
`handle` used for?
  
  
  You are not supposed to be using xdg-desktop-portal-kde, but just 
xdg-desktop-portal (which will call xdg-desktop-portal-kde or 
xdg-desktop-portal-gtk based on DE). The first handle is object path of 
org.freedesktop.portal.Request object, this is an object created for every 
portal call which is supposed to  be used e.g. to close the request. See how I 
use screencast portal here 
https://cgit.kde.org/xdg-portal-test-kde.git/tree/src/portaltest.cpp.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a subscriber: jgrulich.
Kanedias added a comment.


  @jgrulich I started to hack together a very simple 
krfb-with-dbus-with-xdp-kde-with-pipewire... thingie.
  Now I'm curious, you have `handle` and `session_handle` paths everywhere in 
Screencast interface inside XDP-KDE source. Why? I can understand why 
`session_handle` is there - to create and track sessions of course, but what is 
`handle` used for?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: jgrulich, alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  Too late did I realize that I'm making pipewire-based framebuffer, not 
xdp-kde based!
  I guess rewiring stuff will take another 3-4 days, sorry :D

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  I've digged through old diff and I must say, many of what's described here 
will be peeled off. For now we support read-only views, so I'll be focusing on 
it.

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: alexeymin, plasma-devel, ragreen, Pitel, schernikov, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  Sure, I'll be updating diff this week, will address concerns raised here

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: alexeymin, plasma-devel, ragreen, schernikov, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Alexey Min]

alexeymin added a comment.


  While I'm here and still remember some things that I don't like here - naming:
  
  There is a class `EventsPlugin` and files `eventsplugin.{h,cpp}`, class 
`EventsManager` and files `eventsmanager.{h,cpp}` and this is fine.
  But there is also class `EventHandler`, but files are called 
`events.{h,cpp}`, it makes it a little bit harder to quickly read the code; 
maybe it makes sense to rename file also to `eventhandler.h` ?
  Also class `FakeInputEventHandler` in file `fakeinputevents.h`; it may be 
also better to name the file `fakeinputeventhandler.h`...
  
  And the most important suggestion I have: can this work be split in 2 parts:
  
  - first part, split event handling into a separate type of plugin, and add a 
new event handler plugins, so we can test it on X11 first (even without 
wayland) and be sure it still works? (Hopefully this part can be done fast 
enough)
  - second part, add a new shiny GBM framebuffer plugin, and new dependencies 
on libdrm, libgbm, epoxy...
  
  Will be a bit easier to review and test, step by step.
  What do you think?

REPOSITORY
  R437 Desktop Sharing

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

To: Kanedias, davidedmundson, graesslin
Cc: alexeymin, plasma-devel, ragreen, schernikov, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a comment.


  @alexeymin , yeah, I've seen news about KRfb changes not so long ago. I'll 
fix this PR after support once remote access is upstreamed, it's quite 
troublesome to maintain so many changes around.
  Thanks for comments!
  
  P.S. The thing I'm stuck on is keyboard support for VNC... It almost 
explicitly requests rfbkeysyms (X11 ones), dunno how to work with these in 
Wayland.

REPOSITORY
  R437 Desktop Sharing

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

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

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Alexey Min]

alexeymin added a comment.


  There was a https://phabricator.kde.org/D5211, that added `xcb` framebuffer 
plugin; and after that `x11` framebuffer was removed from source tree ( 
https://phabricator.kde.org/R437:f131f7ddba584779144e7f737a98b24de430824c ), so 
this patch of needs to be slightly updated to match the new krfb source.
  I'm not competent in all this Wayland/OpenGL/GBM things (yet) but at least I 
hope that i can test this, when kwin part is ready and merged (which one it is? 
https://phabricator.kde.org/D123? https://phabricator.kde.org/D1231?) so this 
work will not be forgotten in the annals of history :)

INLINE COMMENTS

> CMakeLists.txt:58
> +find_package(gbm)
> +set_package_properties(gbm PROPERTIES TYPE OPTIONAL PURPOSE "Required for 
> egl ouput of drm backend.")
> +set(HAVE_GBM FALSE)

"ouput" => "output"?

> krfb_events_fakeinput.desktop:5
> +Comment[ru]=Обработчик событий для KRfb на базе интерфейса FakeInput KWin
> +Comment[x-test]=KWin FakeInput based event handler for KRfbxx
> +Name=KWin FakeInput based event handler for KRfb

xx at the beginning?

> gbmframebuffer.cpp:296
> +{
> +format.bitsPerPixel = 32;
> +format.depth = 32;

Is the format *always* the same? True color 32bpp?

> logging.h:25
> +
> +Q_DECLARE_LOGGING_CATEGORY(KRFB_GBM)
> +#endif

maybe whole this logging thing can be ported to ecm_qt_declare_logging_category 
( https://api.kde.org/ecm/module/ECMQtDeclareLoggingCategory.html ) ?

> x11framebufferplugin.cpp:23
>  #include "x11framebuffer.h"
>  

no x11 plugin anymore :(

> krfb-events.json:4
> +"Description": "Events plugins for KRfb",
> +"Description[x-test]": "xxFrame Buffer plugins for KRfbxx"
> +},

"xxFrame Buffer plugins for KRfbxx" -> "xxEvents plugins for KRfbxx" ? :)

REPOSITORY
  R437 Desktop Sharing

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

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

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias added a dependency: D1231: Add Remote Access interface to KWayland.

REPOSITORY
  R437 Desktop Sharing

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

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

D6096: Add Wayland RemoteAccess capabilities to KRfb

[Oleg Chernovskiy]

Kanedias created this revision.
Kanedias added a project: Plasma on Wayland.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Patch for KRfb to work with RemoteAccess Wayland protocol.
  This does not yet work (I didn't test this), just rebased against latest KRfb 
source.
  
  Works:
  
  - Screen
  - Mouse movement

TEST PLAN
  Launch KWin on DRM screen with KWIN_REMOTE=1 and with 
preferredFrameBufferPlugin=gbm in krfbrc
  Try to connect with KRDC.

REPOSITORY
  R437 Desktop Sharing

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

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindLibdrm.cmake
  cmake/modules/Findepoxy.cmake
  cmake/modules/Findgbm.cmake
  events/CMakeLists.txt
  events/fakeinput/CMakeLists.txt
  events/fakeinput/fakeinputevents.cpp
  events/fakeinput/fakeinputevents.h
  events/fakeinput/fakeinputeventsplugin.cpp
  events/fakeinput/fakeinputeventsplugin.h
  events/fakeinput/krfb_events_fakeinput.desktop
  events/fakeinput/krfb_events_fakeinput.json
  events/x11/CMakeLists.txt
  events/x11/krfb_events_x11.desktop
  events/x11/krfb_events_x11.json
  events/x11/x11events.cpp
  events/x11/x11events.h
  events/x11/x11eventsplugin.cpp
  events/x11/x11eventsplugin.h
  framebuffers/CMakeLists.txt
  framebuffers/gbm/CMakeLists.txt
  framebuffers/gbm/gbmframebuffer.cpp
  framebuffers/gbm/gbmframebuffer.h
  framebuffers/gbm/gbmframebufferplugin.cpp
  framebuffers/gbm/gbmframebufferplugin.h
  framebuffers/gbm/krfb_framebuffer_gbm.json
  framebuffers/gbm/logging.cpp
  framebuffers/gbm/logging.h
  framebuffers/qt/qtframebufferplugin.cpp
  framebuffers/x11/x11framebufferplugin.cpp
  krfb/CMakeLists.txt
  krfb/events.cpp
  krfb/events.h
  krfb/eventsmanager.cpp
  krfb/eventsmanager.h
  krfb/eventsplugin.cpp
  krfb/eventsplugin.h
  krfb/framebuffermanager.cpp
  krfb/krfb-events.desktop
  krfb/krfb-events.json
  krfb/main.cpp
  krfb/rfbclient.cpp

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