[plugins/qpa] Implement native offscreen surface
ClosedPublic

Authored by zzag on Jun 28 2019, 9:24 PM.

Details

Summary

Depending on whether the underlying platform supports offscreen surfaces,
QOffscreenSurface may create an invisible QWindow. In our case that's the
case, for each offscreen surface a native window is created. This may
lead to some funky results related to window decorations, see bug 407612.

There are several ways to implement offscreen surfaces - either use pbuffers
or utilize a surfaceless context extension. For the sake of simplicity
this change sticks with pbuffers, but it's a good idea to support both
methods.

CCBUG: 407612

Diff Detail

Repository
R108 KWin
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13483
Build 13501: arc lint + arc unit
zzag created this revision.Jun 28 2019, 9:24 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 28 2019, 9:24 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jun 28 2019, 9:24 PM
alexeymin added inline comments.
plugins/qpa/egl_helpers.cpp
70 ↗(On Diff #60810)

This reminds me of what I did in D11758 ... maybe if qpa also needs to be smart about this, the code from that revision could be refactored into a separate function and used here? Attributes were filled in differently there though. Mybe this code already has more restrictions to config attrs and always chooses right.

romangg added a subscriber: romangg.EditedJun 28 2019, 10:40 PM

I would put the code split up into the egl_helpers file into a separate commit. Other than that looks nice. Wondering why we didn't have this implemented before and it wasn't an issue before 5.16.

EDIT: Ok, I read your commit message on 61956025f080. Now it's clearer to me. Good find! When the patch here lands, can the guard in InternalClient be removed again? Or is there still an internal client being created for the offscreen surface?

plugins/qpa/sharingplatformcontext.cpp
98

pbuffers only have a back buffer and eglSwapBuffers is a no-op.

zzag added inline comments.Jun 28 2019, 10:57 PM
plugins/qpa/egl_helpers.cpp
70 ↗(On Diff #60810)

No, this FIXME comment talks about finding the most suitable config, i.e. with different attributes.

plugins/qpa/sharingplatformcontext.cpp
98

Yeah, perhaps we don't need it. QOffscreenSurface is used only to create rendering resources.

zzag updated this revision to Diff 60814.Jun 28 2019, 10:59 PM

Don't swap pbuffers.

zzag added a comment.EditedJun 28 2019, 11:03 PM

When the patch here lands, can the guard in InternalClient be removed again?

Yes, I think so.

zzag updated this revision to Diff 60815.Jun 28 2019, 11:38 PM

Include fixopengl.h

zzag updated this revision to Diff 60827.Jun 29 2019, 10:00 AM

Stick with the existing filename pattern (no underscores).

zzag added a comment.Jul 1 2019, 5:17 PM

I would put the code split up into the egl_helpers file into a separate commit.

I didn't go this road because the proposed change is relatively small, but if it were bigger I would definitely split this commit into several smaller.


I hope that I responded to all questions and comments.

I try to avoid mixing feature patches with chore even if the change is small. Sometimes it's not possible when there is a larger rewrite for a new feature to be done and otherwise the functionality would be broken but it looks possible here. I don't actually understand what the issue is. Splitting up one commit into two is easy. I'm not asking for 10 in a chain.

zzag added a comment.Jul 1 2019, 6:27 PM

I don't actually understand what the issue is.

Laziness. Okay, I'll split it.

zzag updated this revision to Diff 60950.Jul 1 2019, 6:37 PM

Split.

romangg accepted this revision.Jul 1 2019, 6:46 PM
romangg added inline comments.
plugins/qpa/sharingplatformcontext.cpp
85

You could check the opposite and return early. But not crucial.

96

Maybe put this debug line just inside window case in case fbo.isNull(), i.e. not remove but one line above. But not crucial.

101

qobject_cast?

This revision is now accepted and ready to land.Jul 1 2019, 6:46 PM
zzag added inline comments.Jul 1 2019, 6:49 PM
plugins/qpa/sharingplatformcontext.cpp
101

QPlatformWindow is not a QObject.

romangg added inline comments.Jul 1 2019, 6:56 PM
plugins/qpa/sharingplatformcontext.cpp
101

Ok, then better not. Push when you're ready.

zzag added inline comments.Jul 1 2019, 6:58 PM
plugins/qpa/sharingplatformcontext.cpp
101

FWIW we can do

if (surface->surface()->surfaceClass() == QSurface::Window) {
    Window *window = static_cast<Window *>(surface);
}

if dynamic_cast should go away.

zzag added inline comments.Jul 1 2019, 7:00 PM
plugins/qpa/sharingplatformcontext.cpp
96

Ack, I'll move it up into the if statement.

romangg added inline comments.Jul 1 2019, 7:05 PM
plugins/qpa/sharingplatformcontext.cpp
101

Also a good solution. Question is how often is this called if it's worth the potential performance gain. Decide before pushing what you like more and then push with or without it.

This revision was automatically updated to reflect the committed changes.