D1230: GBM remote access support for KWin

[Bhushan Shah]

bshah added a comment.


  In D1230#234333 , @Kanedias wrote:
  
  > I'll take a look once I'm home
  
  
  I've fixed the issue on git master so don't worry.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: bshah, bcooksley, kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, 
kwin, #kwin, davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, 
bwowk, ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  I'll take a look once I'm home

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: bcooksley, kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, 
#kwin, davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Ben Cooksley]

bcooksley added a comment.


  Sorry, that was the wrong log - I intended to post 
https://build.kde.org/job/Plasma%20kwin%20kf5-qt5%20FreeBSDQt5.9/121/consoleText

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: bcooksley, kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, 
#kwin, davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  @bcooksley this file was merged in D1231 

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: bcooksley, kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, 
#kwin, davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Ben Cooksley]

bcooksley added a comment.


  The introduction of this has broken the FreeBSD Build.
  Please see 
https://build.kde.org/job/Plasma%20kwin%20kf5-qt5%20FreeBSDQt5.9/120/consoleText

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: bcooksley, kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, 
#kwin, davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Done! Thanks for looking this through.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, 
davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias closed this revision.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, 
davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg accepted this revision.
romangg added a comment.
This revision is now accepted and ready to land.


  Looks fine to me. Tested runtime together with your KWayland patch.
  
  Probably you know this, but please push as one commit only to master.

INLINE COMMENTS

> egl_gbm_backend.cpp:160
> +{
> +if (qEnvironmentVariableIsSet("KWIN_NO_REMOTE"))
> +return;

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

REPOSITORY
  R108 KWin

BRANCH
  fix-clang

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, 
davidedmundson, plasma-devel, ragreen, schernikov, iodelay, zzag, bwowk, 
ZrenBot, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 30189.
Kanedias added a comment.


  Remove override fix

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=30092=30189

BRANCH
  fix-clang

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

AFFECTED FILES
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, 
davidedmundson, plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  This makes sense. Thanks, will try with KDecoration master once I'm home

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, 
davidedmundson, plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D1230: GBM remote access support for KWin

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In D1230#230642 , @romangg wrote:
  
  > In D1230#230613 , @Kanedias wrote:
  >
  > > They don't override anything and compile fails for me if they are 
present. GCC 7.3.1.
  >
  >
  > I believe this is an unrelated regression you just ran into because of 
D11209 . I hadn't yet updated my 
KDecoration clone, that's why it still compiled for me. In any case please 
remove the unrelated override keyword removal from the patch and let's hope the 
regression will be quickly fixed in KDecoration.
  
  
  No, D11209  should be innocent here. Not 
exporting the symbols of the pimpl classes has no effect on the virtualness of 
the methods of the normal classes.
  
  The issue is rather that those tooltip-related methods only got added 
recently to master, see D7246 . So KWin 
master also expects KDecoration master.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: kossebau, jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, 
davidedmundson, plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, 
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  I don't have KDecoration checked out. The problem is much more simple. These 
functions don't override anything but there's override keyword present where it 
shouldn't be. The fix is still required for them, regardless of KDecoration 
status.
  
  Any other issues apart from this one? I'll remove these lines once I'm home.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added a comment.


  In D1230#230613 , @Kanedias wrote:
  
  > They don't override anything and compile fails for me if they are present. 
GCC 7.3.1.
  
  
  I believe this is an unrelated regression you just ran into because of D11209 
. I hadn't yet updated my KDecoration 
clone, that's why it still compiled for me. In any case please remove the 
unrelated override keyword removal from the patch and let's hope the regression 
will be quickly fixed in KDecoration.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias marked an inline comment as done.
Kanedias added a comment.


  They don't override anything and compile fails for me if they are present. 
GCC 7.3.1.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added a comment.


  Why did you remove the override specifier? It compiles for me also with them. 
And they are certainly in no connection to this patch.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias requested review of this revision.
Kanedias marked an inline comment as done.
Kanedias added inline comments.

INLINE COMMENTS

> romangg wrote in drm_output.h:135
> This gives me an compile error: invalid static_cast from type 'QObject*' to 
> type 'KWayland::Server::OutputInterface*'
> 
> I assume you can just pass m_waylandOutput as QPointer and then access data() 
> in passBuffer. This compiled for me.

Fixed. Also removed `override` in places where it didn't compile because of it.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 30092.
Kanedias added a comment.


  - Fix clang compilation
  - Fix QPointer

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=29434=30092

BRANCH
  fix-clang

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

AFFECTED FILES
  decorations/decoratedclient.h
  kcmkwin/kwindecoration/declarative-plugin/previewclient.h
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, zzag, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> drm_output.h:135
> +const KWayland::Server::OutputInterface* getWaylandInterface() const {
> +return m_waylandOutput.data();
> +}

This gives me an compile error: invalid static_cast from type 'QObject*' to 
type 'KWayland::Server::OutputInterface*'

I assume you can just pass m_waylandOutput as QPointer and then access data() 
in passBuffer. This compiled for me.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 29434.
Kanedias added a comment.


  Typo

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=29433=29434

BRANCH
  gbm-vnc

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

AFFECTED FILES
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added inline comments.

INLINE COMMENTS

> romangg wrote in egl_gbm_backend.cpp:160
> Should be the default not directly activated remote funcitonality? And if one 
> wants to deactivate remote set `KWIN_NO_REMOTE` or something.

There's no authentication protocol, so any malicious application can get whole 
screencast for free.
But that's another story as we're getting full input control already in 
fakeinput-protocol.
Changed.

> romangg wrote in remoteaccess_manager.cpp:85
> This will spam the debug because it is called on every present.

Removed both this and the one at line 77

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 29433.
Kanedias marked 6 inline comments as done.
Kanedias added a comment.


  Review fixes

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=28973=29433

BRANCH
  gbm-vnc

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

AFFECTED FILES
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> romangg wrote in drm_output.h:139
> Is it only a friend class to access `m_waylandOutput.data()`? In this case 
> better create a getter for it in DrmOutput.
> 
> Or better do the `passBuffer` call in `DrmBackend::present` and give instead 
> of the DrmOutput the `KWayland::Server::OutputInterface` from there to 
> `passBuffer`.

Regarding the "better" part: you would need the `m_remoteaccessManager` in 
`DrmBackend`.  Only with EGL/GBM this would then be set. I.e. in QPainter 
`DrmBackend` would ignore it.

But that's maybe a bit too much (also you would need to change the buffer cast 
in passBuffer). So better just add a getter in DrmOutput.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> main_wayland.cpp:780
>  pluginName = KWin::automaticBackendSelection();
> +std::cerr << "Selected backend " << pluginName.toStdString() << 
> std::endl;
>  }

Put this after the if clause (such that it shows the selected backend also on 
manual setting). But it's unrelated to GBM remote accesss, so better remove it 
and commit it as separate patch.

> drm_backend.cpp:103
>  {
> +qCInfo(KWIN_DRM) << "Initializing DRM backend";
>  LogindIntegration *logind = LogindIntegration::self();

Unrelated to GBM remote access. Remove.

> drm_output.h:139
>  private:
> +friend class RemoteAccessManager;
>  friend class DrmBackend;

Is it only a friend class to access `m_waylandOutput.data()`? In this case 
better create a getter for it in DrmOutput.

Or better do the `passBuffer` call in `DrmBackend::present` and give instead of 
the DrmOutput the `KWayland::Server::OutputInterface` from there to 
`passBuffer`.

> egl_gbm_backend.cpp:160
> +{
> +if (!qEnvironmentVariableIsSet("KWIN_REMOTE"))
> +return;

Should be the default not directly activated remote funcitonality? And if one 
wants to deactivate remote set `KWIN_NO_REMOTE` or something.

> remoteaccess_manager.cpp:85
> +
> +qCDebug(KWIN_DRM) << "Buffer passed: bo" << gbmbuf->getBo() << ", fd" << 
> buf->fd();
> +

This will spam the debug because it is called on every present.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Please re-review this and D1231  as I'm 
starting to work on KRfb support for this and it will be the 3rd patch 
depending on this.
  Or suggest reviewers with more spare time so they can look this through.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias marked 2 inline comments as done.
Kanedias added a comment.


  @romangg , I had to recreate it after rebase, cause I don't have force-push.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 28973.
Kanedias added a comment.


  Review comments

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=25979=28973

BRANCH
  gbm-vnc

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

AFFECTED FILES
  main_wayland.cpp
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_backend.cpp
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added a comment.


  Pls remove the whitespace and rebase your branch on current master (it merges 
without conflict).

INLINE COMMENTS

> drm_backend.cpp:793
>  }
> +

rm whitespace

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  @graesslin , @davidedmundson , please approve this once again, this was 
updated numerous times after initial review

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Jan Grulich]

jgrulich added a dependent revision: D10965: Add screen cast portal.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a task: T7785: PipeWire support in remote access to KWin.

REPOSITORY
  R108 KWin

BRANCH
  gbm-vnc

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, harmathy, schernikov, iodelay, bwowk, ZrenBot, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 25979.
Kanedias added a comment.


  - Merge branch 'master' into gbm-vnc
  - Fix compilation on Clang

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=21211=25979

BRANCH
  gbm-vnc

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

AFFECTED FILES
  decorations/decoratedclient.h
  kcmkwin/kwindecoration/declarative-plugin/previewclient.h
  main_wayland.cpp
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_backend.cpp
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Hi @jgrulich!
  
  There are actually two sides of this dime.
  
  The one that I wanted to take is implementing pipewire server in kded. It 
will be responsible for converting GBM fds to PW video stream fds and will 
announce this interface via DBus like Mutter does 
.
  The second one is implementing actual screen recorder based on KF5 with 
pipewire sink. It will consume the stream from kded and save it.
  
  You can take the second subtask as it doesn't actually depend on 
KWin/KWayland.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added a comment.


  In https://phabricator.kde.org/D1230#192516, @Kanedias wrote:
  
  > After it is working we'll have an implicit confirmation that these patches 
indeed have not regressed and can be merged.
  
  
  I don't fully understand what this means, but in general I think it sounds 
like a good plan. I just talked to David as well, and we agreed on making it 
our top priority after 5.12 release to finish this review.
  
  In https://phabricator.kde.org/D1230#192706, @jgrulich wrote:
  
  > I see you have made some progress here, I had similar intentions to bring 
support for PipeWire to KWin as it's something we develop in Red Hat and trying 
to push it forward, but I'm still a newbie in this area and trying to 
understand the stuff around still. Anyway, I just wanted to tell you that I 
have an opportunity to talk to Wim Taymans (author of PipeWire) and attend a 
small PipeWire hackfest we will have in Brno's Red Hat office in two weeks so 
if there is anything I can help with, what should I discuss, suggest, whatever, 
just let me know, I would really like to help here.
  
  
  Great! Can you or @Kanedias  then open a new KWin task here on Phabricator 
for everything related to the additional PipeWire support? We could then use 
this task to store questions for Wim and later the answers.

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Jan Grulich]

jgrulich added a comment.


  In https://phabricator.kde.org/D1230#192516, @Kanedias wrote:
  
  > @romangg , let's go with following approach: I'll upstream all changes I 
have left hanging (e.g. https://phabricator.kde.org/D9708) by the end of this 
week and will try to come up with some PoC work with PipeWire solution 
this/next weekend.
  >  After it is working we'll have an implicit confirmation that these patches 
indeed have not regressed and can be merged.
  >  And after they're merged I'll continue the work on PipeWire integration 
and discuss it in detail.
  >
  > How does it sound?
  
  
  Hi,
  
  I see you have made some progress here, I had similar intentions to bring 
support for PipeWire to KWin as it's something we develop in Red Hat and trying 
to push it forward, but I'm still a newbie in this area and trying to 
understand the stuff around still. Anyway, I just wanted to tell you that I 
have an opportunity to talk to Wim Taymans (author of PipeWire) and attend a 
small PipeWire hackfest we will have in Brno's Red Hat office in two weeks so 
if there is anything I can help with, what should I discuss, suggest, whatever, 
just let me know, I would really like to help here.
  
  Jan

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  @romangg , let's go with following approach: I'll upstream all changes I have 
left hanging (e.g. https://phabricator.kde.org/D9708) by the end of this week 
and will try to come up with some PoC work with PipeWire solution this/next 
weekend.
  After it is working we'll have an implicit confirmation that these patches 
indeed have not regressed and can be merged.
  And after they're merged I'll continue the work on PipeWire integration and 
discuss it in detail.
  
  How does it sound?

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Roman Gilg]

romangg added a comment.


  I talked to @graesslin some time ago and he said, that @davidedmundson and I 
should do the review.
  
  My plan is to get this review going after 5.12 has landed. For Plasma 5.13 
then.
  
  I would like to see at least some progress regarding PipeWire before that 
though. This has one reason mainly:  I don't want to send out the signal to the 
public, that we would be only interested in our own solutions and would be not 
willing to cooperate with other DEs to get to unified ones on Wayland. On the 
other hand I can understand @Kanedias, that maintaining this patch set about 
such a long time was annoying and he wants to get done with it. So maybe we can 
find a compromise in setting up a separate task with a plan on what needs to be 
done after the patch has landed to integrate this solution with PipeWire. Could 
you do this @Kanedias? Just with some bullets points on what would probably be 
your next steps to integrate PipeWire with your solution here after the patches 
have landed?

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: jgrulich, romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Nathaniel Graham]

ngraham added a comment.


  @graesslin, would you mind reviewing this so we can push forward with the 
feature? Thanks!

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  @ngraham I'm still waiting for a review to land this

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Nathaniel Graham]

ngraham added a comment.


  What's the status of this?

REPOSITORY
  R108 KWin

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

To: Kanedias, graesslin, davidedmundson, romangg, #kwin
Cc: romangg, ngraham, alexeymin, aacid, kwin, #kwin, davidedmundson, 
plasma-devel, iodelay, bwowk, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 21211.
Kanedias marked 2 inline comments as done.
Kanedias added a comment.


  Updated against latest master

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=18859=21211

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

AFFECTED FILES
  main_wayland.cpp
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_backend.cpp
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Fixed review comments

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Ok @subdiff, I've got your idea. I'll align my patches with newest sources 
ASAP, check that they work, then we'll talk about what to do with KDE Daemon 
and Screen Recorder.
  I just don't want to spread my efforts too much, maintaining more than one 
patchset in everchanging environment is quite troublesome.
  
  Hope this finally gets merged :)

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added a comment.


  In https://phabricator.kde.org/D1230#159026, @Kanedias wrote:
  
  > > On the other side PipeWire is pretty new and its documentation is nearly 
non-existent. I don't know if with PipeWire it really could work in the way I 
described above. Your current solution works right now. It might still be an 
idea if you are interested to check out if you could integrate your approach 
with PipeWire in the way I described above or something similar and if you 
could do this now or only after the current version version of your patches has 
landed.
  >
  > Why is PipeWire preferred solution for this?
  
  
  The prefered solution from my side is one that is to clients the same on 
every Wayland compositor, so that clients only have to write one new backend 
for Wayland and not n for n Wayland compositors. The other important compositor 
in real world at the moment besides KWin is GNOME's mutter. Since they already 
have a working solution using PipeWire we should try to align our one with 
their one if it is feasible.
  
  > GNOME has to support EGLStreams which have no such thing as buffer 
management like GBM, they can't just do buffer fd passing.
  >  Martin said he's not interested in EGLStreams and this whole patchset was 
prepared in agreement with him.
  
  Neither am I interested in EGLStreams. This is also not about not using GBM. 
My sketched concept with the KDE Daemon would still use GBM buffers for passing 
data between KWin and the Daemon. Only afterwards the Daemon would stream the 
Gl texture to interested clients via PipeWire instead of using GBM.
  
  > Yes, adding additional copying would solve many things, but I guess it's 
out of scope for this patch.
  
  Not sure what you mean with additional copying. If you mean the Gl copy in 
the Daemon from my concept: As far as I've understood it with your current 
approach you need to do a Gl texture copy anyway in the KRfb backend (and 
probably in every other client using the protocol at the same time).
  
  > I'll take a look at PipeWire in my spare time. Is there any rush? I'm under 
strong impression something is happening.
  
  Thank you. There is no rush and nothing is happening out of the orderly.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  > On the other side PipeWire is pretty new and its documentation is nearly 
non-existent. I don't know if with PipeWire it really could work in the way I 
described above. Your current solution works right now. It might still be an 
idea if you are interested to check out if you could integrate your approach 
with PipeWire in the way I described above or something similar and if you 
could do this now or only after the current version version of your patches has 
landed.
  
  Why is PipeWire preferred solution for this? GNOME has to support EGLStreams 
which have no such thing as buffer management like GBM, they can't just do 
buffer fd passing.
  Martin said he's not interested in EGLStreams and this whole patchset was 
prepared in agreement with him. Yes, adding additional copying would solve many 
things, but I guess it's out of scope for this patch.
  
  I'll take a look at PipeWire in my spare time. Is there any rush? I'm under 
strong impression something is happening.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added a task: T5653: [kwin] Screen recording in Wayland session.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added a comment.


  In https://phabricator.kde.org/D1230#158987, @Kanedias wrote:
  
  > > The thing is I would like to see your solution also be applicable to 
other apps that need access to the buffer contents, like screen recording tools 
(SimpleScreenRecorder for example) and so on. If you say KRfb does the 
heavy-lifting, I assume such a screen recording tool would need to do this as 
well in a nontrivial additional backend.
  >
  > But it is. By heavy-lifting I mean calling glReadPixels on that BO.
  
  
  I'm not sure that I understood you correctly but the problem I mean is that 
every other app (SSR, OBS, ...) also needs to:
  
  - Speak to the proposed Wayland protocol extension
  - Have logic to handle GBM buffers and copy these buffers into Gl textures or 
something similar
  
  While this is manageable, I would like to have such apps only need to write 
one backend for all Wayland compositors. PipeWire maybe could provide this.
  
  I could imagine the following (broad strokes):
  
  - Use your KWin code
  - Use your KWayland code as a Plasma only protocol
  - Move your KRfb only code into plasma-workspace
  - Let a KDE Daemon process talk with KWin via your protocol
  - Let this Daemon handle client requests (we should align its interface with 
GNOME)
  - If clients want screen capture do a copy //once// per frame to Gl texture 
(if needed by the next step)
  - Pipe the Gl textures (or if PipeWire can handle GBM buffers these directly) 
into a PipeWire stream
  
  On the other side PipeWire is pretty new and its documentation is nearly 
non-existent. I don't know if with PipeWire it really could work in the way I 
described above. Your current solution works right now. It might still be an 
idea if you are interested to check out if you could integrate your approach 
with PipeWire in the way I described above or something similar and if you 
could do this now or only after the current version version of your patches has 
landed.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  > The thing is I would like to see your solution also be applicable to other 
apps that need access to the buffer contents, like screen recording tools 
(SimpleScreenRecorder for example) and so on. If you say KRfb does the 
heavy-lifting, I assume such a screen recording tool would need to do this as 
well in a nontrivial additional backend.
  
  But it is. By heavy-lifting I mean calling glReadPixels on that BO.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added a comment.


  In https://phabricator.kde.org/D1230#158816, @Kanedias wrote:
  
  > Well, it has `active` property in KWayland that is true only if at least 
one client is connected (if I remember my code from 6 months back correctly), 
so I can expose it and use here. Will this suffice?
  
  
  I think you do already what I meant with the `isBound()` call. Sorry for the 
confusion. :)
  
  >> Then in general did you look at GNOME's way forward of screen 
recording/sharing on Wayland?
  > 
  > No, sorry. This patchset was ready long before the GNOME news. I use 
different idea here that was proposed by Martin Graesslin, with GBM and Wayland 
protocol we actually can pass file descriptors to video buffer objects from one 
app to another 
  >  so KWin is passing BOs to KRfb which then can do all heavy-lifting without 
hurting KWin performance or stability.
  
  The thing is I would like to see your solution also be applicable to other 
apps that need access to the buffer contents, like screen recording tools 
(SimpleScreenRecorder for example) and so on. If you say KRfb does the 
heavy-lifting, I assume such a screen recording tool would need to do this as 
well in a nontrivial additional backend.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added a comment.


  In https://phabricator.kde.org/D1230#158801, @davidedmundson wrote:
  
  > > The RemoteAccessManager is active all the time. It would be nice if it 
would be only active if there are clients who ask for buffers.
  >
  > How is a client going to ask for a buffer if there is no 
RemoteAccessManagerInterface? (which this owns)
  
  
  Yea, I meant that `passBuffer` is allways called. But I just realized on the 
`isBound` call it returns if there are no clients interested.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  In https://phabricator.kde.org/D1230#158795, @subdiff wrote:
  
  > Hi Oleg
  
  
  Hi. Haven't been here in a long time.
  
  > - Please merge current master.
  
  Will do this and fix review comments on weekend (or earlier if I can).
  
  > - The RemoteAccessManager is active all the time. It would be nice if it 
would be only active if there are clients who ask for buffers.
  
  Well, it has `active` property in KWayland that is true only if at least one 
client is connected (if I remember my code from 6 months back correctly), so I 
can expose it and use here. Will this suffice?
  
  > Then in general did you look at GNOME's way forward of screen 
recording/sharing on Wayland?
  
  No, sorry. This patchset was ready long before the GNOME news. I use 
different idea here that was proposed by Martin Graesslin, with GBM and Wayland 
protocol we actually can pass file descriptors to video buffer objects from one 
app to another 
  so KWin is passing BOs to KRfb which then can do all heavy-lifting without 
hurting KWin performance or stability.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[David Edmundson]

davidedmundson added a comment.


  > The RemoteAccessManager is active all the time. It would be nice if it 
would be only active if there are clients who ask for buffers.
  
  How is a client going to ask for a buffer if there is no 
RemoteAccessManagerInterface? (which this owns)
  
  > Then in general did you look at GNOME's way forward of screen 
recording/sharing on Wayland?
  
  They still glReadPixels out the framebuffer, which MG excplicitly didn't want 
and proposed this.
  
  Us using this iface in a helper and shoving it into a pipebuffer should still 
please everyone.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added reviewers: subdiff, KWin.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Roman Gilg]

subdiff added a comment.


  Hi Oleg,
  
  sorry that I haven't given you feedback to your patch series until now. First 
some smaller points:
  
  - Please merge current master.
  - What about the software cursor dynamically switching? I don't see where it 
does that.
  - The RemoteAccessManager is active all the time. It would be nice if it 
would be only active if there are clients who ask for buffers.
  
  Then in general did you look at GNOME's way forward of screen 
recording/sharing on Wayland? They use PipeWire (http://pipewire.org/) for 
that. I'm not very knowledgeable on the whole desktop sharing / video stream 
side, so just a small info on what you think about it and how this relates to 
your code would be nice. I'm asking that, because I find joint work on single 
solutions very important.

INLINE COMMENTS

> remoteaccess_manager.cpp:77
> +auto buf = new BufferHandle;
> +buf->setFd(gbm_bo_get_fd(gbmbuf->getBo()));
> +buf->setSize(gbm_bo_get_width(gbmbuf->getBo()), 
> gbm_bo_get_height(gbmbuf->getBo()));

Local var for `gbmbuf->getBo()`

> remoteaccess_manager.h:24
> +#include "drm_backend.h"
> +#include "../../../wayland_server.h"
> +// KWayland

Do you need all these includes in the header file, or could you forward declare 
for example the KWayland::Server::RemoteAccessManagerInterface and then include 
in the cpp file?

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  In https://phabricator.kde.org/D1230#158542, @ngraham wrote:
  
  > With https://phabricator.kde.org/D6186 merged, is this now ready to go in?
  
  
  Yes, just waits for @graesslin's final review :)

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Nathaniel Graham]

ngraham added a comment.


  With https://phabricator.kde.org/D6186 merged, is this now ready to go in?

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 18859.
Kanedias added a comment.


  Fixes for review comments, rebased to latest version

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=15146=18859

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

AFFECTED FILES
  main_wayland.cpp
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_backend.cpp
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/drm_output.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

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

D1230: GBM remote access support for KWin

[Albert Astals Cid]

aacid added a comment.


  If this indeed needs fixing, can someone please mark it as "request changes" 
so it doesn't show up in the list of "patches that have been accepted but have 
not been commited"?

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Added auxiliary patch for mouse cursor in https://phabricator.kde.org/D6186

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a dependent revision: D6186: Implement software cursor in OpenGL 
backend   .

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  Wait, cursor uses DrmDumbBuffer, which has no GBM BO in it!

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added a comment.


  In https://phabricator.kde.org/D1230#114175, @davidedmundson wrote:
  
  > There is one issue we need to work out how to fix - with this system if 
you're running VNC you won't see the client's mouse.
  
  
  Nice catch, I didn't notice that until you pointed that out!
  
  > - We make kwin render the background + cursor into a new texture
  
  Sounds pretty heavy, we'd need to copy frame on each page flip...
  
  > - We provide the mouse buffer to the RemoteAccess buffer just like we pass 
the outputs. We would need to pass some sort of offset in 
org_kde_kwin_remote_buffer
  
  Offset - you mean, mouse offset, so acceptor knows where to render it?
  
  > - dynamically switch usesSoftwareCursor() in the backend when recording.
  
  Can it break something? As I see DrmBackend never used softwareCursor 
before...
  
  Okay, which option is more preferable here? I tend to think it's №2.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[David Edmundson]

davidedmundson added a comment.


  There is one issue we need to work out how to fix - with this system if 
you're running VNC you won't see the client's mouse.
  
  For VNC it's not a huge problem as you can see your own mouse, for screen 
recording it makes it useless.
  
  As I see it there are 3 options:
  
  - We make kwin render the background + cursor into a new texture
  - We provide the mouse buffer to the RemoteAccess buffer just like we pass 
the outputs. We would need to pass some sort of offset in 
org_kde_kwin_remote_buffer
  - dynamically switch usesSoftwareCursor() in the backend when recording.

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> egl_gbm_backend.cpp:158
> +{
> +bool supportRemotePresent = false;
> +const QByteArray remoteOption = qgetenv("KWIN_REMOTE");

you can do
bool foo =qEnvironmentVariableIsSet("");

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 15146.
Kanedias edited projects, added Plasma on Wayland; removed Plasma.
Kanedias added a comment.
Restricted Application added a project: KWin.
Restricted Application added subscribers: KWin, kwin.


  Rebased against latest KWin, corrected implementation

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1230?vs=2969=15146

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

AFFECTED FILES
  input.cpp
  main_wayland.cpp
  plugins/platforms/drm/CMakeLists.txt
  plugins/platforms/drm/drm_backend.cpp
  plugins/platforms/drm/drm_backend.h
  plugins/platforms/drm/drm_buffer.h
  plugins/platforms/drm/drm_buffer_gbm.h
  plugins/platforms/drm/egl_gbm_backend.cpp
  plugins/platforms/drm/egl_gbm_backend.h
  plugins/platforms/drm/remoteaccess_manager.cpp
  plugins/platforms/drm/remoteaccess_manager.h

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

D1230: GBM remote access support for KWin

[Oleg Chernovskiy]

Kanedias added inline comments.

INLINE COMMENTS

> davidedmundson wrote in drm_buffer.cpp:131
> If it's deferred it means someone else is doing the gbm_surface_release.
> 
> But we still need to set m_bo to nullptr. Otherwise it's potentially left 
> dangling here after the RemoteAccessManager has deleted it.
> 
> (Alternately: if we changed DrmObjectPlane to store the buffers as 
> QSharedPointers we could just keep a reference to the DrmBuffer in the 
> RemoteAccessManager, which would be IMHO cleaner than doing low level GBM 
> stuff there and having the data released in one of two places. I'll look into 
> that)

I rewrote this part since this change and it doesn't touch buffers now, if you 
want to take a look at it prior to my resubmission, it's there:

https://gitlab.com/Kanedias/kwin/commit/9535f36bd09f8834be3773f25bf2075a720ba1c4

> davidedmundson wrote in remoteaccess_manager.h:49-50
> what's this signal for?

Forgot to get rid of it, thanks

REPOSITORY
  R108 KWin

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

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

D1230: GBM remote access support for KWin

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> drm_buffer.cpp:131
>  #if HAVE_GBM
> +if (m_deferred) {
> +return;

If it's deferred it means someone else is doing the gbm_surface_release.

But we still need to set m_bo to nullptr. Otherwise it's potentially left 
dangling here after the RemoteAccessManager has deleted it.

(Alternately: if we changed DrmObjectPlane to store the buffers as 
QSharedPointers we could just keep a reference to the DrmBuffer in the 
RemoteAccessManager, which would be IMHO cleaner than doing low level GBM stuff 
there and having the data released in one of two places. I'll look into that)

> remoteaccess_manager.h:49-50
> +
> +signals:
> +void bufferNoLongerNeeded(qint32 gbm_handle);
> +

what's this signal for?

REPOSITORY
  R108 KWin

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

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

[Differential] [Request, 231 lines] D1230: GBM remote access support for KWin

[Kanedias (Oleg Chernovskiy)]

Kanedias created this revision.
Kanedias added a reviewer: graesslin.
Kanedias set the repository for this revision to rKWIN KWin.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.

REVISION SUMMARY
  Implements a KWayland protocol to pass GBM fd from KWin to KRfb and
  addictions to relevant projects from both sides.
  
  Note that this patch does not affect default behaviour of mentioned projects. 
It can be used
  only with KWIN_REMOTE=1 in env from KWin side and with 
preferredFrameBufferPlugin=gbm in krfbrc from
  KRfb side. In all other aspects app behaviour remains unchanged.

TEST PLAN
  Launched KWin in Wayland mode, launched KRfb in it, launched KRDC on a 
laptop, connected in read-only mode, observed a correctly retrieved desktop 
with Krfb window

REPOSITORY
  rKWIN KWin

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

AFFECTED FILES
  backends/drm/CMakeLists.txt
  backends/drm/drm_backend.cpp
  backends/drm/drm_backend.h
  backends/drm/drm_buffer.cpp
  backends/drm/drm_buffer.h
  backends/drm/egl_gbm_backend.cpp
  backends/drm/egl_gbm_backend.h
  backends/drm/remoteaccess_manager.cpp
  backends/drm/remoteaccess_manager.h
  main_wayland.cpp

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

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