[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: D10747: Implement zwp_linux_dmabuf_v1
From: David Edmundson <noreply () phabricator ! kde ! org>
Date: 2018-04-30 12:18:41
Message-ID: 20180430121841.1.142EEBFA3B688B27 () phabricator ! kde ! org
[Download RAW message or body]
davidedmundson added inline comments.
INLINE COMMENTS
> linuxdmabuf_v1_interface.cpp:364
> +
> + static void unbind(wl_client *client, wl_resource *resource);
> + static void createParamsCallback(wl_client *client, wl_resource *resource, \
> uint32_t id);
is this defined? I can't find it
I'd expect it be used on line 408.
> linuxdmabuf_v1_interface.h:65
> + */
> + class Buffer {
> + public:
doesn't this need exporting?
> linuxdmabuf_v1_interface.h:99
> + */
> +class KWAYLANDSERVER_EXPORT LinuxDmabufUnstableV1Interface : public Global
> +{
One of kwayland's functions is to act as an abstraction layer
Generally all exported class names aren't called with UnstableV1 or whatever.
This would be LinuxDmabufInterface and then we'd handle the V1 stuff in the private \
implementation.
(Personally, I think it's far more effort than it's worth to abstract something that \
isn't guaranteed to be compatiable, and would be ok for you argue that it's \
deliberate)
> fredrik wrote in linuxdmabuf_v1_interface.h:107
> Is this the solution we want for interfacing with the compositor?
>
> My preference would be to use std::function callbacks, with setters in \
> LinuxDmabufUnstableV1Interface. Setting up the interface could then look like this: \
> m_linuxDmabuf = m_display->createLinuxDmabufInterface(m_display);
> m_linuxDmabuf->setQuerySupportedFormats([]{ return \
> Compositor::self()->scene()->supportedDrmFormats(); });
> ...
> m_linuxDmabuf->create();
>
> This can also be extended without breaking binary compatibility. But I don't think \
> we can use std::function in frameworks. There are also BIC issues when mixing \
> different STL implementations, which we may or may not care about.
I don't think I fully understand the issue.
I assume the problem we're solving is that we need to provide supportedFormats on \
client bind, and as per the spec we need to do that immediately but we don't have \
that information before the scene is created which comes after we create the global?
Looking at the current kwin patch we'd just return wrong values / even crash if we \
were called before the scene was created. Given that's currently the case, why can't \
we just have the compositor call a normal setSupportedFormats(...) when the scene is \
first set up.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D10747
To: fredrik, #kwin, #plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, #frameworks, ragreen, Pitel, schernikov, michaelh, \
ZrenBot, bruns, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, \
sebas, apol, mart, hein
[Attachment #3 (unknown)]
<table><tr><td style="">davidedmundson added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: \
right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: \
#F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: \
inline-block; border: 1px solid rgba(71,87,120,.2);" \
href="https://phabricator.kde.org/D10747">View Revision</a></tr></table><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/D10747#inline-64342">View Inline</a><span \
style="color: #4b4d51; font-weight: \
bold;">linuxdmabuf_v1_interface.cpp:364</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: rgba(151, 234, 151, .6);"> <span style="color: \
#aa4000">static</span> <span style="color: #aa4000">void</span> <span style="color: \
#004012">unbind</span><span class="p">(</span><span class="n">wl_client</span> <span \
style="color: #aa2211">*</span><span class="n">client</span><span class="p">,</span> \
<span class="n">wl_resource</span> <span style="color: #aa2211">*</span><span \
class="n">resource</span><span class="p">);</span> </div><div style="padding: 0 8px; \
margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: \
#aa4000">static</span> <span style="color: #aa4000">void</span> <span style="color: \
#004012">createParamsCallback</span><span class="p">(</span><span \
class="n">wl_client</span> <span style="color: #aa2211">*</span><span \
class="n">client</span><span class="p">,</span> <span class="n">wl_resource</span> \
<span style="color: #aa2211">*</span><span class="n">resource</span><span \
class="p">,</span> <span style="color: #aa4000">uint32_t</span> <span \
class="n">id</span><span class="p">);</span> </div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">is \
this defined? I can't find it</p>
<p style="padding: 0; margin: 8px;">I'd expect it be used on line \
408.</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/D10747#inline-64341">View \
Inline</a><span style="color: #4b4d51; font-weight: \
bold;">linuxdmabuf_v1_interface.h:65</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: rgba(151, 234, 151, .6);"><span style="color: #74777d"> \
*/</span> </div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, \
151, .6);"> <span class="n">class</span> <span class="n">Buffer</span> <span \
class="p">{</span> </div><div style="padding: 0 8px; margin: 0 4px; background: \
rgba(151, 234, 151, .6);"> <span style="color: #a0a000">public</span><span \
class="p">:</span> </div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: \
8px;">doesn't this need exporting?</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/D10747#inline-63153">View Inline</a><span \
style="color: #4b4d51; font-weight: bold;">linuxdmabuf_v1_interface.h:99</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: rgba(151, 234, \
151, .6);"><span style="color: #74777d"> */</span> </div><div style="padding: 0 8px; \
margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">class</span> \
<span class="n">KWAYLANDSERVER_EXPORT</span> <span style="color: \
#a0a000">LinuxDmabufUnstableV1Interface</span> <span class="p">:</span> <span \
class="n">public</span> <span class="n">Global</span> </div><div style="padding: 0 \
8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span> \
</div></div></div> <div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; \
margin: 8px;">One of kwayland's functions is to act as an abstraction layer</p>
<p style="padding: 0; margin: 8px;">Generally all exported class names aren't \
called with UnstableV1 or whatever.<br /> This would be LinuxDmabufInterface and then \
we'd handle the V1 stuff in the private implementation.</p>
<p style="padding: 0; margin: 8px;">(Personally, I think it's far more effort \
than it's worth to abstract something that isn't guaranteed to be \
compatiable, and would be ok for you argue that it's \
deliberate)</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/D10747#inline-51262">View Inline</a><span \
style="color: #4b4d51; font-weight: bold;">fredrik</span> wrote in <span \
style="color: #4b4d51; font-weight: \
bold;">linuxdmabuf_v1_interface.h:107</span></div> <div style="margin: 8px 0; \
padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Is this the \
solution we want for interfacing with the compositor?</p>
<p style="padding: 0; margin: 8px;">My preference would be to use std::function \
callbacks, with setters in LinuxDmabufUnstableV1Interface. Setting up the interface \
could then look like this:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" \
data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px \
"Menlo", "Consolas", "Monaco", monospace; padding: \
12px; margin: 0; background: rgba(71, 87, 120, 0.08);">m_linuxDmabuf = \
m_display->createLinuxDmabufInterface(m_display); \
m_linuxDmabuf->setQuerySupportedFormats([]{ return \
Compositor::self()->scene()->supportedDrmFormats(); });
...
m_linuxDmabuf->create();</pre></div>
<p style="padding: 0; margin: 8px;">This can also be extended without breaking binary \
compatibility. But I don't think we can use std::function in frameworks. There \
are also BIC issues when mixing different STL implementations, which we may or may \
not care about.</p></div></div> <div style="margin: 8px 0; padding: 0 12px;"><p \
style="padding: 0; margin: 8px;">I don't think I fully understand the issue.</p>
<p style="padding: 0; margin: 8px;">I assume the problem we're solving is that \
we need to provide supportedFormats on client bind, and as per the spec we need to do \
that immediately but we don't have that information before the scene is created \
which comes after we create the global?</p>
<p style="padding: 0; margin: 8px;">Looking at the current kwin patch we'd just \
return wrong values / even crash if we were called before the scene was created. \
Given that's currently the case, why can't we just have the compositor call \
a normal setSupportedFormats(...) when the scene is first set \
up.</p></div></div></div></div></div><br \
/><div><strong>REPOSITORY</strong><div><div>R127 KWayland</div></div></div><br \
/><div><strong>REVISION DETAIL</strong><div><a \
href="https://phabricator.kde.org/D10747">https://phabricator.kde.org/D10747</a></div></div><br \
/><div><strong>To: </strong>fredrik, KWin, Plasma, graesslin, davidedmundson, mart<br \
/><strong>Cc: </strong>romangg, plasma-devel, Frameworks, ragreen, Pitel, schernikov, \
michaelh, ZrenBot, bruns, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, \
eliasp, sebas, apol, mart, hein<br /></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic