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
> + * @see createRemoteAccessManager
> + * @since 5.7
> + **/
we moved to frameworks which means we are now at 5.23 - sorry about that. I
also had to rename a bunch of @since 5.7 ;-)
> remote_access.h:191
> +Q_SIGNALS:
> + 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
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
