<div>graesslin added a comment.</div><br /><div><div><p>Ups, sorry for the late review. Somehow it got into my backlog and I forgot about it.</p>

<p>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.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6484" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">remote-access.xml:19</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #304a96">  ]]></span><span style="color: #00702a"></copyright></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span style="color: #00702a"><interface</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"org_kde_kwin_remote_access_manager"</span> <span style="color: #354bb3">version=</span><span style="color: #766510">"1"</span><span style="color: #00702a">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">        <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"Protocol for managing rendered GBM buffers passing"</span><span style="color: #00702a">/></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6482" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">remote-access.xml:33</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">        <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"This interface allows finer control of remote buffer lifecycle"</span><span style="color: #00702a">/></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">        <span style="color: #00702a"><event</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"gbm_handle"</span> <span style="color: #354bb3">since=</span><span style="color: #766510">"1"</span><span style="color: #00702a">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">            <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"This is sent after binding to remote access manager"</span> <span style="color: #00702a">/></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6483" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">remote-access.xml:41</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">        <span style="color: #00702a"></event></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">        <span style="color: #00702a"><request</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"no_longer_needed"</span> <span style="color: #354bb3">type=</span><span style="color: #766510">"destructor"</span> <span style="color: #354bb3">since=</span><span style="color: #766510">"1"</span><span style="color: #00702a">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">          <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"This request comes once client no longer needs this buffer."</span><span style="color: #00702a">/></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Just for thought: in other interfaces the destructor is mostly called "release"</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6485" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:128</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; ">        <span class="n">Idle</span><span class="p">,</span> <span style="color: #74777d">///< Refers to org_kde_kwin_idle_interface interface</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">        <span class="n">RemoteAccessManager</span><span class="p">,</span> <span style="color: #74777d">///< Refers to org_kde_kwin_remote_access_manager interface</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">        <span class="n">FakeInput</span><span class="p">,</span> <span style="color: #74777d">///< Refers to org_kde_kwin_fake_input interface</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please add as last interface otherwise it breaks API</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6486" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:379</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #74777d">     * @see createRemoteAccessManager</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #74777d">     * @since 5.7</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #74777d">     **/</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">we moved to frameworks which means we are now at 5.23 - sorry about that. I also had to rename a bunch of <span class="phabricator-remarkup-mention-unknown">@since</span> 5.7 ;-)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6487" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">remote_access.h:191</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #a0a000">Q_SIGNALS</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span style="color: #aa4000">void</span> <span class="n">paramsObtained</span><span class="p">(</span><span class="n">qint32</span> <span class="n">fd</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">width</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">height</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">stride</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">format</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would make the arguments getters in the interface and rather have an empty signal.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6489" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">remote_access_interface.h:46</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #74777d"> **/</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #aa4000">struct</span> <span class="n">GbmBuffer</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">For ABI compatibility it's better to only have d-ptr-ized classes/structs in the public header.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1231#inline-6488" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">remote_access_interface.h:49-50</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span style="color: #74777d">// relevant for server</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span class="n">gbm_surface</span> <span style="color: #aa2211">*</span><span class="n">surface</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span class="n">gbm_bo</span> <span style="color: #aa2211">*</span><span class="n">bo</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span class="n">qint32</span> <span class="n">fd</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span> <span style="color: #74777d">//< also buffer_id</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">why do we need gbm_surface and gbm_bo? This looks like adding not needed details to the interface</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rKWAYLAND KWayland</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D1231" rel="noreferrer">https://phabricator.kde.org/D1231</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>Kanedias, graesslin<br /><strong>Cc: </strong>plasma-devel, sebas<br /></div>