This implements a wrapper class for the wl_eglstream_controller Wayland interface. It allows clients to inform the compositor when a new EGL Stream has been created with an Wayland surface attached as its producer. The compositor can then bind a GL texture as the stream's consumer allowing it access to the surface's buffer contents for presentation. The only client currently expected to make use of this interface is the NVIDIA EGL driver when running alongside a compositor supporting EGLStream-based buffer sharing.
Details
Diff Detail
- Repository
- R127 KWayland
- Lint
Lint Skipped - Unit
Unit Tests Skipped
src/server/CMakeLists.txt | ||
---|---|---|
349 | alphabetical sorted | |
src/server/display.cpp | ||
529 | There should be always a non-null object returned. Instead check somewhere before the create call of EglStreamControllerInterface if the library can be loaded. Also this would put the logic and the QLibrary include in the class file and not in display.cpp, what I would prefer. One could overwrite the Global::create method in the EglStreamControllerInterface child class. From there then call QLibrary::resolve and only if it succeeds the parent create method. Afterwards compositor needs to check Global::isValid. The control flow would then be in the compositor: auto *e = display->createEglStreamControllerInterface(); // other initial setup e->create(); if (!e->isValid()) { // error handling } It's unfortunate that the create method is not virtual in Global. We can change this in KF6. | |
src/server/display.h | ||
310 | 5.56 | |
src/server/eglstream_controller_interface.cpp | ||
53 | Why is attribs unused in this implementation? | |
54 | Use Global::Private::cast. | |
58 | Rename controller and m_controller below to q for enhanced pimpl street cred. | |
src/server/eglstream_controller_interface.h | ||
28 | Cleanup includes |
Thanks for taking a look, Roman. I've addressed your style suggestions and added support for passing stream attributes to the compositor. I had ignored these initially since the driver doesn't actually use this (fairly new) feature of the extension right now, but it probably is best to support it for future-proofness.
src/server/eglstream_controller_interface.cpp | ||
---|---|---|
59 | Please don't use c style casts. |
The xml file is missing form the diff now. Don't forget to add it when pushing.
In the first revision the interface failed to create when the dynamically loaded library was not present (we said the class should always be created though). Now it just fails silently on bind. Why is it ok to fail silently on bind?
src/server/display.h | ||
---|---|---|
310 | Now it would be 5.58 (change it when pushing). |
src/server/eglstream_controller_interface.h | ||
---|---|---|
50 | pedantically that should be: ~EglStreamControllerInterface() override; |
Now it just fails silently on bind. Why is it ok to fail silently on bind?
We wouldn't get that far.
If the library didn't load then we wouldn't have sent an interface to wl_global_create
I don't know how that behaves, but we would have either crashed already (which might warrant a guard in Global::Private::create) or the client wouldn't be seeing a global registered to bind to
Actually, that was an oversight on my part. It looks like wl_global_create() doesn't actually check whether the interface is null, so right now it'll just segfault if the library can't be loaded. Probably not what we want.
I'm re-working it right now to load the library in create() and bail out there if it's not successful to avoid this. I'll update the diff shortly.
Ok, now we will bail out early during create() if we fail to dynamically load the interface (before calling wl_global_create). The compositor can then use isValid() to check if whether creating the interface succeeded or not, it will also print a warning if it fails. Unfortunately, as Roman has pointed out, since Global::create is not virtual the compositor technically *could* still break things by calling the parent class method, but I'm not sure if it's worth changing that for just this one somewhat special case, especially considering that this will probably only ever actually be used by KWin's EGLStream backend anyway.
Changed the dynamic_cast in create() to static_cast. Since the Private is created in the constructor of EglStreamControllerInterface, it should be safe to assume it is of the correct type.
Also marked Private as Q_DECL_HIDDEN.
Thanks. Don't forget to add the isValid check to the KWin patches now. (checked it, it's already in there) Maybe also add a comment to the KWayland code, what's the reason for the separate create() call and that we want to change this in KF6 via a virtualized create method.
src/server/eglstream_controller_interface.h | ||
---|---|---|
57 | wl_resource *eglStream? |