[Differential] [Commented On] D1231: Add Krfb interface to KWayland

[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
> +     * @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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160525/a2b0b232/attachment-0001.html>