[platforms/drm] Introduce Gl post-process output rotations
Needs ReviewPublic

Authored by romangg on Dec 11 2019, 8:29 PM.

Details

Reviewers
None
Group Reviewers
KWin
Maniphest Tasks
T6106: Support screen rotation in OpenGL compositor
Summary

In case the hardware is not able to rotate the output for the configured
rotation value do this rotation in a post-process step.

For that rendering the current view into a separate framebuffer bound to a
texture that then gets sampled to the default framebuffer in an additional
rendering pass through a simple shader rotating it.

This allows us to leave the Effects system and internal model-view-projection
matrix untouched. The rotation in the post-processing step is isolated.

BUG: 389665
FIXED-IN: 5.18

Test Plan

With KScreen all rotations work.

Diff Detail

Repository
R108 KWin
Branch
post-process-rotation
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 19759
Build 19777: arc lint + arc unit
romangg created this revision.Dec 11 2019, 8:29 PM
Restricted Application added a project: KWin. · View Herald TranscriptDec 11 2019, 8:29 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Dec 11 2019, 8:29 PM
romangg edited the summary of this revision. (Show Details)Dec 11 2019, 8:43 PM
zzag added a subscriber: zzag.Dec 11 2019, 9:49 PM

A client may provide buffers with already rotated contents. Will such clients be a big problem with this patch series?

In D25907#575911, @zzag wrote:

A client may provide buffers with already rotated contents. Will such clients be a big problem with this patch series?

I don't think so. Rotated content is meant for direct scanout. In this case we would not go through rendering path and likely also not through post-processing. But since we don't have direct scanout at the moment this possibility is not (yet) relevant. And on the other side if we go through rendering path we re-rotate the content already in the Scene render to place the content into it.

plugins/platforms/drm/egl_gbm_backend.h
80

Remove empty line

romangg added inline comments.Dec 11 2019, 10:07 PM
plugins/platforms/drm/egl_gbm_backend.cpp
72

Remove empty line

romangg added inline comments.Dec 12 2019, 12:04 AM
plugins/platforms/drm/egl_gbm_backend.cpp
293

Align indent.

romangg updated this revision to Diff 71371.Dec 12 2019, 4:21 PM
  • Fix rotations with multiple outputs
romangg updated this revision to Diff 71373.Dec 12 2019, 4:27 PM
romangg marked 3 inline comments as done.
  • Whitespace change
z3ntu added a subscriber: z3ntu.Dec 13 2019, 4:12 PM
davidedmundson added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
260

seems unused?
Personally I'd say it's the better name.

274

Can we make such GLSL assumptions?

I expect this would error on the phone.

ShaderManager::instance()->pushShader(ShaderTrait::MapTexture)

would be safer.

romangg added inline comments.Dec 16 2019, 10:10 AM
plugins/platforms/drm/egl_gbm_backend.cpp
260

I added this at some point because the GLShader and/or GLVertexBuffer classes were somewhat expecting a uniform variable of this name to be in the shader. But I don't remember if I also checked back one last time in the end without it.

274

Yea, good question about the versioning. While working on it I just chose the one that worked on my system, but you're right these won't work on every system. Since the shader is pretty simple would it be enough to rewrite it as a version 110 shader to get it to work everywhere (although I then don't understand why most effects have a 110 an 140 version and not just the 110)?

I didn't use the MapTexture trait because when doing that certain assumptions are being made about the shader that only fit shaders created for Scene rendering.

davidedmundson added a comment.EditedMon, Jan 6, 10:30 AM

I'm happy to accept this if we include D26371.

IMHO easiest is for @romangg to grab that and squash in here so history is cleanest but we can do whatever works

I'm happy to accept this if we include D26371.

IMHO easiest is for @romangg to grab that and squash in here so history is cleanest but we can do whatever works

Hmm, I couldn't get it to work with the MapTexture shader when I tried it. But great that you got it to work. You can put your patch on top, then we have both approaches in the history.

I'll give mine another message then and merge in on top then.
Have you been able to test with my patch.

There are review comments left on D25906 and D25904
It would be good to get up to here merged as-is, without tying it in to all of these other kscreen changes.