[Differential] [Request, 550 lines] D1231: Add Krfb interface to KWayland

[Kanedias (Oleg Chernovskiy)]

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

REVISION SUMMARY
This commit adds an interface bridge from KWin to KRfb. The purpose of
this protocol is to pass a GBM fd of currently displayed buffer from
KWin. The buffer is expected to be fully drawn once it is passed.

Related to https://phabricator.kde.org/D1230

REPOSITORY
rKWAYLAND KWayland

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

AFFECTED FILES
src/client/CMakeLists.txt
src/client/protocols/remoteaccess.xml
src/client/registry.cpp
src/client/registry.h
src/client/remoteaccess.cpp
src/client/remoteaccess.h
src/server/CMakeLists.txt
src/server/display.cpp
src/server/display.h
src/server/remoteaccess_interface.cpp
src/server/remoteaccess_interface.h
src/tools/mapping.txt

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

To: Kanedias, graesslin
Cc: plasma-devel

[graesslin (Martin Gräßlin)]

graesslin added a comment.


Implementation of the protocol looks good, but please see my comment on whether the interface has a correct semantic. I fear it cannot properly handle the case that the compositor produces several buffers before the client consumed them.

Please also add an auto test for the code, we try to have a complete test coverage for KWayland.

INLINE COMMENTS
src/client/protocols/remoteaccess.xml:21 In Wayland it's common to use underscore names instead of camel case. Thus it would be buffer_no_longer_needed.

Do we need the request at all or can we just use a deconstructor?

Btw. I'm wondering how would the mapping between a buffer ready and a buffer no longer needed be done. It's possible that the producer provides more buffers than the consumer can consume.

Maybe we need a dedicated remote-buffer interface for each one?

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

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


Remade protocol to be able to pass multiple buffers to client.
Sent buffers are tracked inside interface object.

REPOSITORY
rKWAYLAND KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=2970&id=3620

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

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[graesslin (Martin Gräßlin)]

graesslin added a comment.


Ups, sorry for the late review. Somehow it got into my backlog and I forgot about it.

The protocol looks good to me. I only have a minor change request there. On Server side I would rethink how the buffers are tracked. I think with the struct GbmBuffer we are exposing too much implementation detail and maybe even getting KWin implementation detail into the public API.

INLINE COMMENTS

remote-access.xml:19
+ ]]></copyright>
+ <interface name="org_kde_kwin_remote_access_manager" version="1">
+ <description summary="Protocol for managing rendered GBM buffers passing"/>

I noticed that standard Wayland protocols also have a destructor request for the manager interface. I'd suggest to also add it (it makes sense as then the server can destroy the resource).

remote-access.xml:33
+ <description summary="This interface allows finer control of remote buffer lifecycle"/>
+ <event name="gbm_handle" since="1">
+ <description summary="This is sent after binding to remote access manager" />

so the idea here would be that if in future we want to support other buffers (e.g. shared mem) we just add a new event?

remote-access.xml:41
+ </event>
+ <request name="no_longer_needed" type="destructor" since="1">
+ <description summary="This request comes once client no longer needs this buffer."/>

Just for thought: in other interfaces the destructor is mostly called "release"

registry.h:128
Idle, ///< Refers to org_kde_kwin_idle_interface interface
+ RemoteAccessManager, ///< Refers to org_kde_kwin_remote_access_manager interface
FakeInput, ///< Refers to org_kde_kwin_fake_input interface

please add as last interface otherwise it breaks API

registry.h:379
+ **/
remote_access.h:191
+ void paramsObtained(qint32 fd, quint32 width, quint32 height, quint32 stride, quint32 format);
+

I would make the arguments getters in the interface and rather have an empty signal.

remote_access_interface.h:46
+ **/
+struct GbmBuffer
+{

For ABI compatibility it's better to only have d-ptr-ized classes/structs in the public header.

remote_access_interface.h:49-50
+ // relevant for server
+ gbm_surface *surface = nullptr;
+ gbm_bo *bo = nullptr;
+ qint32 fd = 0; //< also buffer_id

why do we need gbm_surface and gbm_bo? This looks like adding not needed details to the interface

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

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


Updated against last M.Graesslin review.
Sorry, couldn't find a way to get rid of GbmBuffer struct (removed some redundant fields though).

Re-checked with latest KWin and KRfb.

REPOSITORY
rKWAYLAND KWayland

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

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

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[graesslin (Martin Gräßlin)]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


Overall I would also like to see autotests for it similar to the tests for other interfaces.

INLINE COMMENTS

remote_access_interface.cpp:146-149
+ if (bound) {
+ wl_client_post_no_memory(client);
+ return;
+ }

if really only one client should be allowed (why?) it would be better to send a dedicated error state to inform it instead of "abusing" no memory.

remote_access_interface.cpp:158
+ wl_resource_set_implementation(resource, &s_interface, this, unbind);
+ m_resource = resource;
+ bound = true;

this allows to have only one client bind it. As soon as a second client binds the protocol it will get overwritten and breaks the existing one.

I think you need a QVector<wl_resource*> here.

remote_access_interface.h:43
+ **/
+struct GbmBuffer
+{

for ABI compatibility we cannot have structs defined in the header. See https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts for explanation. Especially the last point of the Donts is relevant as it means we are never able to change this again.

Thus I think the proper way has to be:

class Buffer
{
public:
void setFd(int qint32);
void setSize(const QSize &size);
void setStride(qint32 stride);
enum class Format {
Format1,
Format2
};
void setFormat(Format format);

private:
class Private;
QScopedPointer<Private> d;
};

and then in the Implementation have the Private class which holds the data members.

remote_access_interface.h:94-112
+class KWAYLANDSERVER_EXPORT RemoteBufferInterface : public Resource
+{
+ Q_OBJECT
+ virtual ~RemoteBufferInterface() = default;
+
+ /**

I don't see this class exposed in any way in the API. So a user of the library cannot have access to it.

Thus I suggest to move it into a private header and remove the KWAYLANDSERVER_EXPORT. It's a private class to the library then, which makes life easier.

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

Kanedias retitled this revision from "Add Krfb interface to KWayland" to "Add Remote Access interface to KWayland".
Kanedias updated the test plan for this revision.
Kanedias updated this revision to Diff 4337.
Kanedias added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


Now server can send buffers to multiple clients

+ GbmBuffer now has d-pointer
+ added autotests with edge cases

REPOSITORY
rKWAYLAND KWayland

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

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

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

Kanedias marked 8 inline comments as done.
Kanedias added inline comments.

INLINE COMMENTS

graesslin wrote in remote-access.xml:19
I noticed that standard Wayland protocols also have a destructor request for the manager interface. I'd suggest to also add it (it makes sense as then the server can destroy the resource).

Added destructor and release callback.

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

Yes, with appropriate "since" clause

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

Renamed

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

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

graesslin wrote in registry.h:379

Corrected

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

Added ability to have multiple clients in the same time

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

Reimplemented

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

Implemented GbmBuffer with d-pointer

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

Removed, reimplemented

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

Thanks for clarification.
Reimplemented.

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

Kanedias updated this revision to Diff 4387.
Kanedias marked 8 inline comments as done.
Kanedias added a comment.


Changed getter methods to public as we'll need them from KWin side.
+ Doxygen typo

REPOSITORY
rKWAYLAND KWayland

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

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

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[graesslin (Martin Gräßlin)]

graesslin added inline comments.

INLINE COMMENTS

test_remote_access.cpp:161
+ // client fd is different, not subject to check
+ QVERIFY(rbuf->width() == 50);
+ QVERIFY(rbuf->height() == 50);

use QCOMPARE to test two values. If it fails with QCOMPARE you get the values it actually had. With QVERIFY you only see that it failed

registry.h:144
};

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

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

qCDebug please

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

hmm maybe parametersObtained?

remote_access_interface.cpp:28
+
+#include <QtCore/QDebug>
+#include <QHash>

you can just include "logging_p.h"

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

qCDebug

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

qCDebug

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

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

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

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

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

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

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

Thinking out loud:

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

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

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

remote_access_interface_p.h:33
+ */
+class RemoteBufferInterface : public Resource

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

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

nitpick

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

Kanedias marked 12 inline comments as done.
Kanedias added inline comments.

INLINE COMMENTS

graesslin wrote in test_remote_access.cpp:161
use QCOMPARE to test two values. If it fails with QCOMPARE you get the values it actually had. With QVERIFY you only see that it failed

Got it

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

Understood, changed

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

Sounds reasonable, changed

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

Noted, re-using categories from there.

graesslin wrote in remote_access_interface.cpp:240
qCDebug

Done

graesslin wrote in remote_access_interface.cpp:263
qCDebug

Done

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

Reused, thanks

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

Removed that code completely, thanks to `resourceDestroyedCallback`

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

Removed

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

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

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

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

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

Done, improved Doxygen comment also

graesslin wrote in remote_access_interface_p.h:58
nitpick

Sure, fixed

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[Kanedias (Oleg Chernovskiy)]

Kanedias updated the test plan for this revision.
Kanedias updated this revision to Diff 4401.
Kanedias marked 12 inline comments as done.
Kanedias added a comment.


Review comments & cleanup

REPOSITORY
rKWAYLAND KWayland

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

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

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

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

To: Kanedias, graesslin
Cc: plasma-devel, sebas

[graesslin (Martin Gräßlin)]

graesslin added a comment.


I just realized a possible problem: multi-screen. On multi-screen we have one buffer for each screen. But how does the client know for which screen the buffer is. I think we need to somehow pass the information for which wl_output the buffer is.

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, sebas

[graesslin (Martin Gräßlin)]

graesslin added a comment.


I had been thinking about multi-screen issue and how we can get it working in the protocol. The biggest problem is that we cannot really map to the wl_output in a way that it's useful to the client. Also caused by QtWayland not exposing the wl_output in the native interface. From server side we could send a wl_output resource of that client, but our Qt based clients would not know what to do with them :-(

Given that we need to have the client tell the server for which wl_output it wants to have the buffer. A possibility would be to pass the wl_output as argument to the get_buffer request. But then how would the buffer_ready event indicate for which wl_output it is? Maybe we need to do it like the with org_kde_kwin_dpms_manager. It would require to add another level of indirection. Which is nothing I want as it just sounds too complicated and requires quite some changes to the otherwise finished review here. Le sigh. I wish I had noticed that problem earlier. It's something one only notices when using Wayland in day-to-day with multi-screen setup.

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, sebas

[Kanedias (Oleg Chernovskiy)]

Kanedias added a comment.


Can't we pass screen index along with all the invocations? Krfb (and other recording tools) will know the screen configuration as they reside in same wayland session. They'll get the buffer and the screen index and will know exactly what to map. Am I missing something here?

Besides, I didn't find any mentions of multi-screen capabilities in Krfb at all. It currently works like this:

d->framebufferImage = XGetImage(QX11Info::display(),
id,
0,
0,
QApplication::desktop()->width(),
QApplication::desktop()->height(),
AllPlanes,
ZPixmap);

If that's the requirement, there will be huge amount of work to implement it from ground up.
Patchset for KRfb is already enormous and rewrites half of the input system into plugins instead of built-in libraries (to integrate it with fake-input). I doubt it will endure another set of additions, the review will take forever.
I think we should implement screen indexing in protocol but start with passing screen №1 only for now.

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, abetts, sebas

[graesslin (Martin Gräßlin)]

graesslin added a comment.

Post by Kanedias (Oleg Chernovskiy)
Can't we pass screen index along with all the invocations?

what is a screen index? You mean the id of the global referencing the wl_output?

Post by Kanedias (Oleg Chernovskiy)
Krfb (and other recording tools) will know the screen configuration as they reside in same wayland session. They'll get the buffer and the screen index and will know exactly what to map. Am I missing something here?

That might be a race condition. What if the output got removed? On the other hand if the events are queued, it might work.

Post by Kanedias (Oleg Chernovskiy)
d->framebufferImage = XGetImage(QX11Info::display(),
id,
0,
0,
QApplication::desktop()->width(),
QApplication::desktop()->height(),
AllPlanes,
ZPixmap);
If that's the requirement, there will be huge amount of work to implement it from ground up.

Right I see the problem. On X11 of course there is just one virtual screen for all outputs. Thus krfb just always get the whole screen. On Wayland we have the problem that the compositor is rendering to each output individually. So we end up with a buffer for each output. Argh that sucks. I'm not seeing a solution for it right now and would say just ignore it for the moment. Either we only support one output at the start or combine the image of all outputs to one.

REPOSITORY
rKWAYLAND KWayland

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

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

To: Kanedias, graesslin
Cc: plasma-devel, jensreuterberg, abetts, sebas

[David Edmundson]

davidedmundson added a comment.
This revision now requires changes to proceed.

Post by graesslin (Martin Gräßlin)
Also caused by QtWayland not exposing the wl_output in the native interface.

This might have been true at the time of writing,. It's not the case now.

nativeResourceForScreen will return a wl_output, we can loop through them, and then match the wl_output ID to a buffer here.

REPOSITORY
R127 KWayland

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

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

[David Edmundson]

davidedmundson added a comment.


Seaprate question,
In wl_surface when we attach a new buffer we also mark what areas are damaged.

Here we're passing an even bigger buffer.

Would it benefit from a series of damage events being sent in the org_kde_kwin_remote_buffer before the gbm_handle event?
Kwin should have all this information available. Or is it best for VNC do that sort of thing itself?

INLINE COMMENTS

remote-access.xml:23
+ <description summary="Signals about buffer ready to be consumed by clients"/>
+ <arg name="id" type="uint" summary="unique id of created buffer (you can use server-side fd number)"/>
+ </event>

Assuming we do output merging in the client we want an extra arg with the wl_output here.

We don't need want it in the request, as each output will get a different buffer and therefore a different ID.

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 15125.
Kanedias added a reviewer: davidedmundson.
Kanedias added a comment.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.


Rebased the protocol against latest KWayland branch.
Will update on the comments this week.

REPOSITORY
R127 KWayland

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

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

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

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

[Oleg Chernovskiy]

Kanedias added a comment.

Post by David Edmundson
Would it benefit from a series of damage events being sent in the org_kde_kwin_remote_buffer before the gbm_handle event?
Kwin should have all this information available. Or is it best for VNC do that sort of thing itself?

I wanted to implement dumb buffer retrieval from KWin and passing as a first step and then to implement damage.
Or do you think we should stabilize protocol now? I see KRfb X11 plugin indeed waits for XDamage events and adds them to query in `FrameBuffer::modifiedTiles` later.

INLINE COMMENTS

Post by David Edmundson
davidedmundson wrote in remote-access.xml:23
Assuming we do output merging in the client we want an extra arg with the wl_output here.
We don't need want it in the request, as each output will get a different buffer and therefore a different ID.

Gotcha, will do

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a dependent revision: D1230: GBM remote access support for KWin.

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a dependent revision: D6096: Add Wayland RemoteAccess capabilities to KRfb.

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

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

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


@davidedmundson , do we really have nativeResourceForScreen call? AFAIK KWin uses its own QPA, which implements only bits of functionality. We need to have nativeResourceForScreen to be able to pass wl_output, should I patch QPA also, or is there better way?

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

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


Fixed KWAYLAND -> Kwayland in CmakeLists.txt

REPOSITORY
R127 KWayland

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

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

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

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

[David Edmundson]

davidedmundson added a comment.


We don't want or need to go via the QPA on the server side. The wayland specific output management is in the DRM plugin, your code is in the DRM plugin. It should be simple.

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


How do I get ID from wl_output interface? I kinda got how I can get the needed info from DrmOutput instance, but not sure how to compare them on server and client side

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

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


Added wl_output reference for buffers as requested by @davidedmundson and @graesslin

REPOSITORY
R127 KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=15553&id=16318

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

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


I'll get rid of the fakeinput-related changes and test it with KRfb tomorrow.

@davidedmundson , can you test this with multiple outputs?

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

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


some -> const modifiers and typos

REPOSITORY
R127 KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=16318&id=16359

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

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


@davidedmundson , @graesslin , I cleaned up fake-input handling, fixed autotests.

Tested this manually with patched KWin and KRfb version - all works fine (only one screen though).
I'm able to retrieve wl_output from native interface as David suggested.

Regarding damage regions - I don't quite see how this works along with GBM buffer passing (as far as I can see we have only whole screens as GBM BOs in KWin,
but I may be missing something.

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


Btw, should I bump patches to KWin/KRfb to match this version?

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


Gentle reminder

REPOSITORY
R127 KWayland

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

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

[David Edmundson]

davidedmundson added a comment.


All good to me. But double check with Martin.

@graesslin this is a good week for merging bit stuff as the last frameworks was just released.

REPOSITORY
R127 KWayland

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

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

[David Edmundson]

davidedmundson added a comment.


Martin, I explicitly asked you to look at this on Monday.

It's being really unfair to a new contributor to make them jump through all sorts of hoops to do things the way you want them, and leave them hanging for literally over a year.

REPOSITORY
R127 KWayland

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

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

[Alexey Min]

alexeymin added a comment.

Post by Kanedias (Oleg Chernovskiy)
...
d->framebufferImage = XGetImage(QX11Info::display(),
id,
0,
0,
QApplication::desktop()->width(),
QApplication::desktop()->height(),
AllPlanes,
ZPixmap);
If that's the requirement, there will be huge amount of work to implement it from ground up.
Patchset for KRfb is already enormous and rewrites half of the input system into plugins instead of built-in libraries (to integrate it with fake-input). I doubt it will endure another set of additions, the review will take forever.
I think we should implement screen indexing in protocol but start with passing screen №1 only for now.

Speaking about krfb, after https://phabricator.kde.org/D5211 X11 plugin (and `XGetImage` code) does not exist anymore. And krfb is aware of having multiple screens, but it shares only primary screen area:

- xcb plugin: https://cgit.kde.org/krfb.git/tree/framebuffers/xcb/xcb_framebuffer.cpp#n171
- qt plugin: https://cgit.kde.org/krfb.git/tree/framebuffers/qt/qtframebuffer.cpp#n81

I cannot imagine how VNC server application can properly serve multiple monitors at once, especially if they have different resolutions (merge them into one big image covering all monitors at once, with black border around the smaller one?). So I think if krfb will ever support multiple monitors explicitly, there will be a combo box to select which screen to share.

Do you think other screen recording applications will need to capture several monitors at once?

REPOSITORY
R127 KWayland

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

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

[Nathaniel Graham]

ngraham added a comment.


Is this mergeable? @graesslin?

REPOSITORY
R127 KWayland

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

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

[Roman Gilg]

subdiff added inline comments.

INLINE COMMENTS

remote_access_interface.cpp:206
+ // no reason for client to bind wl_output multiple times
+ Q_ASSERT(boundScreens.size() == 1);
+ org_kde_kwin_remote_access_manager_send_buffer_ready(res, buf->fd(), boundScreens[0]);

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

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added inline comments.

INLINE COMMENTS

subdiff wrote in remote_access_interface.cpp:206
Can a rogue client do it though? This would crash the server then?
Can a rogue client do it though? This would crash the server then?

Yes, I guess so... What would you propose? Should we send it only to first bound? Or last one?

P.S. Even more: this interface has no authentication/authorization at all, so any client can connect and steal our video buffers.
Martin said that first version of protocol can go without it and we can readd it later (as with fakeinput protocol).

REPOSITORY
R127 KWayland

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

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

[Roman Gilg]

subdiff added inline comments.

INLINE COMMENTS

Kanedias wrote in remote_access_interface.cpp:206

Post by Roman Gilg
Can a rogue client do it though? This would crash the server then?

Yes, I guess so... What would you propose? Should we send it only to first bound? Or last one?
P.S. Even more: this interface has no authentication/authorization at all, so any client can connect and steal our video buffers.
Martin said that first version of protocol can go without it and we can readd it later (as with fakeinput protocol).

Only first bound like you do it now. Just remove the Q_ASSERT (and make sure `boundScreens.size() >= 1`, otherwise continue).

no authentication/authorization at all

That's a generic problem yet to be solved on Wayland / the Linux desktop. This also correlates with the push to containerized apps. I would just want something like the permission system in Android, but there might be better solutions. It's a bigger project for sure.

Also see here for some early thoughts on it, which to my knowledge until now did not lead to anything more: http://www.mupuf.org/blog/2014/02/19/wayland-compositors-why-and-how-to-handle/

REPOSITORY
R127 KWayland

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

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

[Roman Gilg]

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

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

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


Updated against latest master,
fixed review comments

REPOSITORY
R127 KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=16359&id=21212

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

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

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

[Oleg Chernovskiy]

Kanedias marked 5 inline comments as done.

REPOSITORY
R127 KWayland

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

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

[Nathaniel Graham]

ngraham added a comment.


What's the status of this? Are we waiting for something other than @graesslin's review?

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


@ngraham, yes, he didn't review this after changes were made

REPOSITORY
R127 KWayland

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

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

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R127 KWayland

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

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

[Roman Gilg]

romangg added a reviewer: romangg.

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 25976.
Kanedias edited the test plan for this revision.
Kanedias added a comment.


- Add RemoteAccess interface to KWayland
- Merge branch
- Fix compilation warnings uint -> int

REPOSITORY
R127 KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=21212&id=25976

BRANCH
gbm-vnc

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

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

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

[Oleg Chernovskiy]

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

REPOSITORY
R127 KWayland

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

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

[Jan Grulich]

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

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


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

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


It will soon be this patch 2nd birthday. Can we speed up things a bit? I've seen this is scheduled for Plasma 5.13, would be good if we have time to test it.

REPOSITORY
R127 KWayland

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

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

[Roman Gilg]

romangg added a comment.


Please rebase onto master (or if this leads to problems with your remote merge master).

REPOSITORY
R127 KWayland

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

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

[Jaime Torres Amate]

jtamate added a comment.


As I don't see anything related to security in this patch, I have two questions.

Could anyone with access to server:port manage the server wayland sessions or just create a new session?
The access control should be done in the firewall?

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

Kanedias added a comment.

Post by Jaime Torres Amate
As I don't see anything related to security in this patch, I have two questions.
Could anyone with access to server:port manage the server wayland sessions or just create a new session?
The access control should be done in the firewall?

What port? This patch doesn't expose any port.
No, nobody can manage server sessions with this protocol.

REPOSITORY
R127 KWayland

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

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

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

CMakeLists.txt:430
+target_link_libraries( testRemoteAccess Qt5::Test Qt5::Gui KF5::WaylandClient KF5::WaylandServer)
+add_test(kwayland-testRemoteAccess testRemoteAccess)
+ecm_mark_as_test(testRemoteAccess)

`add_test(NAME kwayland-testRemoteAccess COMMAND testRemoteAccess)`

otherwise ctest doesn't find the test.

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

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

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

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


- Implement releasing of client-freed output
- Review fixes

REPOSITORY
R127 KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=25976&id=30271

BRANCH
gbm-vnc

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

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

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

[Oleg Chernovskiy]

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


Remove already merged changes

REPOSITORY
R127 KWayland

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D1231?vs=30271&id=30272

BRANCH
gbm-vnc

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

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

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

[Oleg Chernovskiy]

Kanedias updated this revision to Diff 30273.
Kanedias marked an inline comment as done.
Kanedias added a comment.


- Merge branch 'master' into gbm-vnc

REPOSITORY
R127 KWayland

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

BRANCH
gbm-vnc

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

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

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

[Roman Gilg]

romangg accepted this revision.
romangg added a comment.


Looks fine to me. Tested compilation as well as runtime with KWin's DRM+EGL backend.

- Please change in comments the frameworks version number these changes are landing in.
- I would like to have @davidedmundson look over this patch a last time. If he doesn't have time until mid-week to do this, you can push.
- Probably you know this, but please push as one commit only.

INLINE COMMENTS

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

rm whitespace

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

rm whitespace

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

rm whitespace (at the end)

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

rm whitespace

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

rm whitespace

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

rm whitespace

REPOSITORY
R127 KWayland

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

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

[Oleg Chernovskiy]

This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R127:5116fe0c6345: Add Remote Access interface to KWayland (authored by Kanedias).

CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D1231?vs=30273&id=30534#toc

REPOSITORY
R127 KWayland

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

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

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

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

[Oleg Chernovskiy]

Kanedias added a comment.


- squashed before pushing
- fixed all versions to 5.45

Thanks gentlemen, it's was 1 day more for the second birthday of this patch :)

REPOSITORY
R127 KWayland

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

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