Implement Wayland support using PipeWire and xdg-desktop-portal
Needs ReviewPublic

Authored by jgrulich on Apr 9 2019, 7:13 AM.

Details

Reviewers
Kanedias
romangg
Summary

Adds a new framebuffer implementation, which uses xdg-desktop-portal to support remote
desktop on Wayland and uses PipeWire to deliver the screen content. So far only mouse
support is implemented, because keyboard support is missing on KWin side.

Diff Detail

Repository
R437 Desktop Sharing
Branch
jgrulich/wayland-pipewire
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11967
Build 11985: arc lint + arc unit
jgrulich requested review of this revision.Apr 9 2019, 7:13 AM
jgrulich created this revision.
jgrulich updated this revision to Diff 55800.Apr 9 2019, 7:14 AM

Remove unwanted kdev4 file

Note that this change requires KWin and xdg-desktop-portal-kde from master (upcoming Plasma 5.16) for mouse control, otherwise only screen content will be seen, but you won't be able to control it with your mouse.

PW framebuffer looks okay, it's the event system that worries me.

  1. We kind of force krfb to link with DBus, even if our plugin is not present.
  2. Are we moving mouse via D-Bus?..

The thing is, we are stuck with old event system in krfb that was designed with X11 in mind.
What I tried to do in the past is break it down into plugins, this allowed me to get rid of clauses like "are we on X11?"
and "do this, but only if we have that one protocol" and so on. Unfortunately no one was there to review it.

Take a look at the last commits in this repo: https://gitlab.com/Kanedias/KRfb/commits/gbm-vnc,
especially those that are related to event system.

  • We already have fakeinput protocol (at least for mouse) baked into KWin so you can use it, it's faster than D-Bus for sure.
  • We can implement D-Bus based events plugin without raising potential bugs in whole event system.
  • You can move linking against D-Bus into the events plugin, so distro maintainers will have more flexibility over deps.
krfb/CMakeLists.txt
24

Redundant change fragment

krfb/events.cpp
42

What's supposed to be here?

krfb/rfbserver.cpp
149

Doesn't QClipboard work on Wayland?

Kanedias added a subscriber: romangg.

@romangg, as you are now maintaining KWin, take a look at this as well.

P.S.

@jgrulich I also have to say that I'm not against DBus implementation in general, merely worried a bit. I see you also have keyboard protocol, that's a breakthrough! Never managed to grasp keysyms conversion myself.

And about event system again, is it okay that it retrieves custom properties from framebuffer plugin? Maybe we can have it establish other connection just for events?

krfb/rfbservermanager.cpp
137

Not sure KRfb codebase was converted but we are using C++11 for 8 years now, we can have for (auto server: d->servers)

PW framebuffer looks okay, it's the event system that worries me.

  1. We kind of force krfb to link with DBus, even if our plugin is not present.
  2. Are we moving mouse via D-Bus?..

Yes, we are, the reason why I use portals for this is that it will work in Flatpak/Snap without any modifications and it will also work in Gnome if someone decides to use it there.

The thing is, we are stuck with old event system in krfb that was designed with X11 in mind.
What I tried to do in the past is break it down into plugins, this allowed me to get rid of clauses like "are we on X11?"
and "do this, but only if we have that one protocol" and so on. Unfortunately no one was there to review it.

Take a look at the last commits in this repo: https://gitlab.com/Kanedias/KRfb/commits/gbm-vnc,
especially those that are related to event system.

I see, I will look and try to improve this D-Bus dependency situation.

  • We already have fakeinput protocol (at least for mouse) baked into KWin so you can use it, it's faster than D-Bus for sure.

I know and it's what I use in xdg-desktop-portal-kde, the reason why I don't use fakeinput directly is mentioned above.

P.S.

@jgrulich I also have to say that I'm not against DBus implementation in general, merely worried a bit. I see you also have keyboard protocol, that's a breakthrough! Never managed to grasp keysyms conversion myself.

I don't have keyboard protocol, it's still missing. There are defined methods in xdg-desktop-portal for keyboard, but we don't have a KDE implementation, because there is no support for this in fakeinput protocol in KWayland.

And about event system again, is it okay that it retrieves custom properties from framebuffer plugin? Maybe we can have it establish other connection just for events?

I did this only to be able to initialize framebuffer after we establish a PW connection, this cannot be done in advance and I need to know the resolution. Once I receive information about the resolution from PW/Portals, I can initialize framebuffer.

krfb/events.cpp
42

Keyboard support, once it's implemented . I should maybe remove this completely until there is keyboard support we can use.

krfb/rfbserver.cpp
149

Nope, or at least this way it was breaking VNC protocol and I wasn't able to connect.

Kanedias added inline comments.Apr 11 2019, 5:52 PM
krfb/rfbserver.cpp
149

Hmm I think I know the reason. Try to write the password without copying it from anywhere or selecting anything.
We need to disable sending paste events to the client before password ask request is answered.

jgrulich updated this revision to Diff 57246.Tue, Apr 30, 10:40 AM

Make Krfb event handlers as plugins

jgrulich added inline comments.Tue, Apr 30, 10:41 AM
krfb/rfbserver.cpp
149

I always type the password, I don't paste it and I don't have any text selected.

Oh, only now noticed this patch. Should I take directly a look at it or is there some more work to do at the moment?

Oh, only now noticed this patch. Should I take directly a look at it or is there some more work to do at the moment?

No more work is needed or in plan at this moment. There is missing support for keyboard, but this is missing also on xdg-desktop-portal-kde and KWayland side.

pino added a subscriber: pino.Sat, May 18, 10:18 AM

Btw, please remove all the translations (ru, and x-test) from .desktop, and .json files. There is a system to handle them, so these manually injected ones will be removed the day after this work is merged.

jgrulich updated this revision to Diff 58328.Mon, May 20, 6:10 AM

Remove [ru] and [x-test] translations from desktop and json files

pino added a comment.Mon, May 20, 6:19 AM

Remove [ru] and [x-test] translations from desktop and json files

Almost, there are still krfb/krfb-events.desktop, krfb/krfb-events.json.

Also, please remove the whitespace-only changes in main.cpp, and rfbservermanager.cpp.

Furthermore: since D21267: Do not crash on wayland, gracefully exit with error instead was merged, you must add a new check to allow wayland as display server, otherwise krfb will not even start... Even a simple

else if (is_wayland /* whatever is the proper check */) {
  /* nothing to check */
}

will do it.