Differential D22150

[plugins/qpa] Implement native offscreen surface
ClosedPublic
Actions

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

Details

Reviewers
romangg
Group Reviewers
KWin
Commits
R108:f5b66a583de4: [plugins/qpa] Implement native offscreen surface
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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 a subscriber: alexeymin.Jun 28 2019, 10:28 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
In D22150#487984, @romangg wrote:

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.

romangg added a comment.Jul 1 2019, 6:06 PM

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 ↗(On Diff #60950)

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 ↗(On Diff #60950)

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 ↗(On Diff #60950)

QPlatformWindow is not a QObject.

romangg added inline comments.Jul 1 2019, 6:56 PM
plugins/qpa/sharingplatformcontext.cpp
101 ↗(On Diff #60950)

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

zzag added inline comments.Jul 1 2019, 6:58 PM
plugins/qpa/sharingplatformcontext.cpp
101 ↗(On Diff #60950)

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 ↗(On Diff #60950)

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.

Closed by commit R108:f5b66a583de4: [plugins/qpa] Implement native offscreen surface (authored by zzag). · Explain WhyJul 1 2019, 7:09 PM
This revision was automatically updated to reflect the committed changes.

Showing Only Differences

This revision modifies 5 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

Diff 60954

View Options

plugins/qpa/sharingplatformcontext.cpp

Loading...