[Differential] [Commented On] D1231: Add Remote Access interface to KWayland
graesslin added a comment. In https://phabricator.kde.org/D1231#39191, @Kanedias wrote: > 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? > 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. > 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. 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D1231: Add Remote Access interface to KWayland
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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D1231: Add Remote Access interface to KWayland
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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D1231: Add Remote Access interface to KWayland
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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D1231: Add Remote Access interface to KWayland
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
> +TextInputManagerUnstableV2, ///< Refers to
> zwp_text_input_manager_v2, @since 5.23
> +RemoteAccessManager ///< Refers to
> org_kde_kwin_remote_access_manager interface, @since 5.23
> };
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
> +Q_SIGNALS:
> +void paramsObtained();
> +
hmm maybe parametersObtained?
> remote_access_interface.cpp:28
> +
> +#include
> +#include
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
> + * @brief Holds data of passed buffer and client resource. Also controls
> buffer lifecycle.
> + */
> +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
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
