Implement wl_eglstream_controller Server Interface
ClosedPublic

Authored by ekurzinger on Feb 7 2019, 7:11 PM.

Details

Summary

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.

Diff Detail

Repository
R127 KWayland
Lint
Lint Skipped
Unit
Unit Tests Skipped
ekurzinger created this revision.Feb 7 2019, 7:11 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 7 2019, 7:11 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ekurzinger requested review of this revision.Feb 7 2019, 7:11 PM
romangg added inline comments.Feb 9 2019, 2:15 PM
src/server/CMakeLists.txt
350

alphabetical sorted

src/server/display.cpp
528

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
54

Why is attribs unused in this implementation?

55

Use Global::Private::cast.

59

Rename controller and m_controller below to q for enhanced pimpl street cred.

src/server/eglstream_controller_interface.h
29

Cleanup includes

romangg requested changes to this revision.Feb 9 2019, 2:16 PM

Impressive. Some small issues to resolve.

This revision now requires changes to proceed.Feb 9 2019, 2:16 PM
ekurzinger updated this revision to Diff 51709.Feb 14 2019, 7:28 PM

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.

ekurzinger marked 7 inline comments as done.Feb 14 2019, 7:29 PM
mati865 added a subscriber: mati865.Mar 6 2019, 7:29 PM

@romangg, are you satisfied with this now?

zzag added inline comments.Apr 11 2019, 5:04 PM
src/server/eglstream_controller_interface.cpp
58

Please don't use c style casts.

zzag resigned from this revision.Apr 11 2019, 5:06 PM

I don't have any NVIDIA hardware to test these patches so I'm kinda useless.

ekurzinger updated this revision to Diff 56009.Apr 11 2019, 6:57 PM

removed C-style casts

ekurzinger marked an inline comment as done.Apr 11 2019, 6:57 PM
romangg added a comment.EditedApr 12 2019, 10:08 AM

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).

ekurzinger updated this revision to Diff 56067.Apr 12 2019, 2:06 PM

Re-added protocol XML file to diff and corrected version in display.h

davidedmundson accepted this revision.Apr 12 2019, 2:16 PM
davidedmundson added inline comments.
src/server/eglstream_controller_interface.h
49

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

ekurzinger marked an inline comment as done.Apr 12 2019, 2:51 PM

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?

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.

ekurzinger updated this revision to Diff 56075.Apr 12 2019, 3:56 PM

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.

ekurzinger marked an inline comment as done.Apr 12 2019, 3:57 PM
zzag added a subscriber: zzag.Apr 12 2019, 4:12 PM
zzag added inline comments.
src/server/eglstream_controller_interface.cpp
104

Can we avoid dynamic casting a reference? Is it possible to use static_cast instead?

src/server/eglstream_controller_interface_p.h
34

Should be marked with Q_DECL_HIDDEN.

ekurzinger updated this revision to Diff 56078.Apr 12 2019, 4:28 PM

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.

ekurzinger marked 2 inline comments as done.Apr 12 2019, 4:28 PM
romangg accepted this revision.EditedApr 13 2019, 5:46 PM

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?

This revision is now accepted and ready to land.Apr 13 2019, 5:46 PM
This revision was automatically updated to reflect the committed changes.