Add Wayland RemoteAccess capabilities to KRfb
AbandonedPublic

Authored by Kanedias on Jun 4 2017, 10:15 PM.

Details

Summary

Patch for KRfb to work with RemoteAccess KWin Wayland -> XDP-KDE -> Pipewire stream

Works:

  • Screen in read-only mode
Test Plan
  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

Diff Detail

Repository
R437 Desktop Sharing
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Kanedias created this revision.Jun 4 2017, 10:15 PM
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptJun 4 2017, 10:15 PM
alexeymin added a comment.EditedAug 27 2017, 3:48 AM

There was a 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? D1230? D1231?) so this work will not be forgotten in the annals of history :)

CMakeLists.txt
58

"ouput" => "output"?

events/fakeinput/krfb_events_fakeinput.desktop
5 ↗(On Diff #15147)

xx at the beginning?

framebuffers/gbm/gbmframebuffer.cpp
296 ↗(On Diff #15147)

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

framebuffers/gbm/logging.h
25 ↗(On Diff #15147)

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

framebuffers/x11/x11framebufferplugin.cpp
23 ↗(On Diff #15147)

no x11 plugin anymore :(

krfb/krfb-events.json
4 ↗(On Diff #15147)

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

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

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?

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

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.

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

@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?

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

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?

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?

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.

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?

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

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

Kanedias added a comment.EditedMay 17 2018, 8:04 PM

@jgrulich , thanks for the pointers

but I guess you shouldn't be able to call it with less parameters then you supposed to, right?

still, it shouldn't just crash the XDP right away... Just wanted to confirm this isn't normal

Kanedias updated this revision to Diff 34665.May 22 2018, 7:17 PM
Kanedias edited the summary of this revision. (Show Details)

Rewrite everything from scratch with Pipewire connectivity in mind

@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
alexeymin added a comment.EditedMay 30 2018, 6:32 PM

How exactly I can test if something works? And what are the requirements? PipeWire? Something else?
Or is this only for code review for now?

  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

Kanedias added a comment.EditedMay 30 2018, 7:07 PM

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

Kanedias edited the summary of this revision. (Show Details)Jun 18 2018, 5:48 PM
Kanedias edited the test plan for this revision. (Show Details)
zzag added a subscriber: zzag.Jun 18 2018, 7:52 PM
zzag added inline comments.
framebuffers/pipewire/pw_framebuffer.h
27

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

framebuffers/pipewire/pw_framebufferplugin.h
36

virtual is redundant.

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.

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

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)

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.

@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

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

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

I assume it is correct now, I got some fixes from Christophe Giboudeaux.

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.

@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?

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

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.

Kanedias abandoned this revision.Sep 29 2018, 7:40 PM