--8a83c77a7e87477698fbdda3ca755a71 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="ascii" Mime-Version: 1.0 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 --8a83c77a7e87477698fbdda3ca755a71 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset="ascii" Mime-Version: 1.0 View Revision
davidedmundson added inline comments.

INLINE COMMENTS
View Inlinelinuxdmabuf_v1_interface.cpp:364
static void unb= ind(wl_client *client, wl_resource *resource);
static void createParams= Callback(wl_client= *client, wl_resource *resource= , uint32_t i= d);

is this defined? I can't find it

I'd expect it be used on line 40= 8.


*/
class Buffer {
public:

doesn't this need exporting?


= View Inlinelinuxdmab= uf_v1_interface.h:99
*/
class KWAYLANDSERV= ER_EXPORT LinuxDmabufUnstableV1Interf= ace : public Global
{

One of kwayland's functions is to act as an abstraction laye= r

Generally all exported class names ar= en't called with UnstableV1 or whatever.
This would be LinuxDmabufInterface and then we'd handle the V1 stuff i= n the private implementation.

(Personally, I think it's far mo= re effort than it's worth to abstract something that isn't guaran= teed to be compatiable, and would be ok for you argue that it's delibe= rate)


View Inlinefredrik 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::fu= nction callbacks, with setters in LinuxDmabufUnstableV1Interface. Setting u= p the interface could then look like this:

m_linuxDmabuf =3D m_display->createLinuxDmabufInterface(m_disp=
lay);
m_linuxDmabuf->setQuerySupportedFormats([]{ return Compositor::self()-&g=
t;scene()->supportedDrmFormats(); });
...
m_linuxDmabuf->create();

This can also be extended without bre= aking binary compatibility. But I don't think we can use std::function= in frameworks. There are also BIC issues when mixing different STL impleme= ntations, which we may or may not care about.

I don't think I fully understand the issue.

I assume the problem we're solvi= ng is that we need to provide supportedFormats on client bind, and as per t= he spec we need to do that immediately but we don't have that informat= ion before the scene is created which comes after we create the global?

Looking at the current kwin patch we&= #039;d just return wrong values / even crash if we were called before the s= cene was created. Given that's currently the case, why can't we j= ust have the compositor call a normal setSupportedFormats(...) when the sc= ene is first set up.


RE= POSITORY
R127 KWayland

REVISION DETAIL
https://phabricator.kde.org/D10747

To:= fredrik, KWin, Plasma, graesslin, davidedmundson, mart
Cc: romangg, plasma-devel, Frameworks, ragreen, Pitel, schernik= ov, michaelh, ZrenBot, bruns, alexeymin, lesliezhai, ali-mohamed, jensreute= rberg, abetts, eliasp, sebas, apol, mart, hein
--8a83c77a7e87477698fbdda3ca755a71--