This interface provides a way for clients to create generic dmabuf-based wl_buffers.
Details
- Reviewers
graesslin davidedmundson mart fredrik - Group Reviewers
KWin Plasma - Maniphest Tasks
- T8067: Support zwp_linux_dmabuf
- Commits
- R127:1dd57d909165: Implement zwp_linux_dmabuf_v1
Works with weston-simple-dmabuf-drm and weston-simple-dmabuf-egl in KWin.
Diff Detail
- Repository
- R127 KWayland
- Branch
- dmaBuf
- Lint
Lint Skipped Excuse: Ignore old issues. - Unit
No Unit Test Coverage - Build Status
Buildable 13159 Build 13177: arc lint + arc unit
+1 on my machine opengl apps seems to work well on wayland (after the first revision in which they were flipped upside down, now they are ok)
would need a final ship it by someone more familiar with that area tough :)
src/server/linuxdmabuf_v1_interface.h | ||
---|---|---|
40 | Do we want these nested namespaces? We could have LinuxDmabufFlags, LinuxDmabufBuffer etc. instead. | |
66 | Should the Buffer class use a d-pointer? | |
108 | 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. |
Please import the context. This makes reviews more easy. Either via -U99999 flag to git diff or (for future diffs) by using arc directly on a feature branch.
Regarding the "drm_fourcc.h" file: do we want to copy it in KWayland's code base or could we use the system one? It's only available on Linux? In this case could we include it as a dummy only on non-Linunx systems? This way we could use the system one on Linux.
Should LinuxDmabufParams subclass Resource?
src/server/linuxdmabuf_v1_interface.h | ||
---|---|---|
40 | Since there are not yet any namespaces in KWayland below the client/server level, I would prefer it without the namespace. | |
66 | I think yes. Together with a Private class implemented in the cpp file. | |
108 | I'm not sure what's the canonical way in KWayland to do this. I assume the supported formats and modifiers could be saved in a field of the interface's Private class on creation. The buffer could be imported through a signal to the compositor and a function in the interface to be called by the compositor afterwards to proceed. |
I don't know if it's available on all platforms we support, and copying it was the path of least resistance.
Should LinuxDmabufParams subclass Resource?
I considered doing that, but Resource is designed in such a way that it requires the use of a d-pointer, and LinuxDmabufParams is a private class.
src/server/linuxdmabuf_v1_interface.h | ||
---|---|---|
108 | I considered that, but it seems ugly to have the compositor call back into KWayland from the slot. It would also be necessary to pass a pointer to the LinuxDmabufParams object in a signal parameter, just so the compositor can pass it back to KWayland. A slightly less ugly option would to make the LinuxDmabufParams class public, and emit the signal from the params object. But that would require the compositor to track the creation of those objects so it can connect the signal to a slot. |
src/server/linuxdmabuf_v1_interface.cpp | ||
---|---|---|
365 | is this defined? I can't find it I'd expect it be used on line 408. | |
src/server/linuxdmabuf_v1_interface.h | ||
66 | doesn't this need exporting? | |
100 | One of kwayland's functions is to act as an abstraction layer Generally all exported class names aren't called with UnstableV1 or whatever. (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) | |
108 | 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. |
- D-pointer DmaBufBuffer
- Only allow one create request in any case
- Params in private interface class
- typedef for LinuxDmabufUnstableV1Interface
- Small cleanup
- Set buffer privates and delete impl on interface destroy
If there are no objections I would push this soon.
There is one remaining annoyance I have with the current version: the name of the class inside the interface the compositor has to subclass for interfacing with KWayland. I changed the name from Bridge to Impl since Bridge can be anything and as we use the name Impl in KWin already for stuff like this. But there it's used for the subclass and not for the abstract implementation base class. Any other idea for a good name?