Software rotation: orient the screen regardless of whether drm accepted it
AbandonedPublic

Authored by apol on Mar 18 2019, 5:00 PM.

Details

Reviewers
romangg
Group Reviewers
KWin
Summary

In case it fails, tell the OpenGL scene how to rotate the scene.

Test Plan

Rotated my system and the system rotated with me.

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D19860_2
Lint
Lint SkippedExcuse: x
Unit
No Unit Test Coverage
Build Status
Buildable 17518
Build 17536: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
zzag added a comment.Mar 26 2019, 8:47 PM

Software rotation still doesn't work correctly on my machine :'(

zzag added inline comments.Mar 28 2019, 7:36 AM
abstract_output.h
126
/**
 * Returns ...
 **/
virtual int softwareRotationAngle() const {
    return 0;
}
outputscreens.cpp
109

return 0

platformsupport/scenes/opengl/abstract_egl_backend.cpp
33

This is a kwin core include, thus it should go above.

platformsupport/scenes/opengl/abstract_egl_backend.h
81

Pointers have to be aligned to the right, i.e.

AbstractOutput *output,
plugins/platforms/drm/drm_output.h
86
int softwareRotationAngle() const {
    return m_softwareRotationAngle;
}
plugins/scenes/opengl/scene_opengl.cpp
665

const int rotation

1017–1018

Missing braces as well spaces after commas.

apol marked 6 inline comments as done.Mar 28 2019, 2:31 PM
apol updated this revision to Diff 54987.Mar 28 2019, 2:33 PM
apol marked an inline comment as done.

address zzag's comments

apol updated this revision to Diff 55009.Mar 29 2019, 12:09 AM

Address coding style issues pointed out by zzag

apol added a comment.Apr 2 2019, 1:06 PM

Ping, I'm not sure what we're holding off on here.

Software rotation still doesn't work correctly on my machine :'(

What's the status of this?

Also, I would really expect that we have work to do on:

  • blur, coverswitch, screenshot - anything that blits the framebuffer

it'll be taking untransformed contents.

zzag added a comment.Apr 2 2019, 1:54 PM

What's the status of this?

IIRC, the problem is that KWin can't correctly detect whether hardware rotation is supported, which seems still the case.

apol added a comment.EditedApr 2 2019, 4:03 PM
In D19860#442282, @zzag wrote:

What's the status of this?

IIRC, the problem is that KWin can't correctly detect whether hardware rotation is supported, which seems still the case.

So you want me to include [1] from D19731 here? I'm happy to do that, I didn't do it because I think they're separate issues but if that's what's keeping you from reviewing my patch I can include it.
[1]

zzag added inline comments.Apr 3 2019, 9:08 PM
plugins/platforms/virtual/virtual_output.h
53

Style.

zzag added inline comments.Apr 3 2019, 9:18 PM
plugins/platforms/virtual/virtual_output.h
53

Also, why 90?

apol updated this revision to Diff 55374.Apr 3 2019, 9:18 PM

Address vlad's comment

apol updated this revision to Diff 55375.Apr 3 2019, 9:19 PM

Actually no 90

apol marked 2 inline comments as done.Apr 3 2019, 9:20 PM
apol added inline comments.
plugins/platforms/virtual/virtual_output.h
53

I just added that to test locally rotation. Reverted to 0.

zzag added a comment.Apr 3 2019, 9:53 PM

Tweaking projection matrix seems to be very sensible solution, though it would lead to a bunch of other issues (probably not the full list):

  • Background Contrast, Blur, Screenshot, Glide and maybe some others effects would not work correctly on rotated screens;
  • Buffer age is most likely broken;
  • The cursor is corrupted.

Tackling those problems in this patch is not a good idea. Maybe create a new task?

outputscreens.cpp
107

I know that you follow the rest of the code, but could you please rename this variable?

According to the Frameworks coding style, one shall not use short variable names.

platformsupport/scenes/opengl/abstract_egl_backend.cpp
29

Please move it before "composite.h"

plugins/platforms/drm/drm_output.cpp
836–854

I know that you copied this code, but could we maybe re-format it accordingly to the Frameworks coding style?

Also, it would be great to use more const.

plugins/platforms/drm/egl_gbm_backend.cpp
235

Abuse of auto.

plugins/platforms/virtual/virtual_output.h
52

Please remove extra new line.

screens.cpp
292 ↗(On Diff #55375)

Please remove extra new line.

apol marked 5 inline comments as done.Apr 3 2019, 11:30 PM
apol added inline comments.
plugins/platforms/drm/drm_output.cpp
836–854

I would rather not.

apol updated this revision to Diff 55386.Apr 3 2019, 11:30 PM

Coding style

apol updated this revision to Diff 55433.Apr 4 2019, 6:13 PM

style

zzag added inline comments.Apr 4 2019, 6:39 PM
plugins/platforms/drm/egl_gbm_backend.cpp
235

Please align pointers to the right.

Side note: It would be great to use clang-format so we don't have to deal with coding style issues.

apol updated this revision to Diff 55447.Apr 4 2019, 11:55 PM

coding style

apol marked an inline comment as done.Apr 4 2019, 11:55 PM
apol added a comment.Apr 9 2019, 11:42 PM

Software rotation still doesn't work correctly on my machine :'(

What's the status of this?

Also, I would really expect that we have work to do on:

  • blur, coverswitch, screenshot - anything that blits the framebuffer it'll be taking untransformed contents.

No it's not. As you can see in SceneOpenGL2::paintSimpleScreen, m_screenProjectionMatrix which is the matrix used by blur (at least) is already taking the rotation into account.

Proof: https://www.youtube.com/watch?v=KWscNeq_AGA

apol updated this revision to Diff 55986.Apr 11 2019, 1:18 PM

Pass screen rotation information to effects and fix taking screenshots of rotated views

apol updated this revision to Diff 56028.Apr 12 2019, 2:50 AM

Clean patch

apol updated this revision to Diff 56343.Apr 16 2019, 2:07 AM

Detect when the display reports portrait by default

apol updated this revision to Diff 56906.Apr 24 2019, 4:37 PM

Disable blur and bg contrast when rotated

It doesn't transform entirely right yet, this way we can fix it down the line without weird corrupt renders

apol updated this revision to Diff 57766.May 8 2019, 3:25 PM

Fix background contrast when rotated

apol updated this revision to Diff 60361.Jun 22 2019, 3:29 PM

rebase on top of master

univerz added a subscriber: univerz.
Dr.X added a subscriber: Dr.X.Aug 6 2019, 6:16 AM
apol updated this revision to Diff 64752.Aug 27 2019, 4:40 PM

fix resolution regression

romangg requested changes to this revision.EditedAug 27 2019, 5:37 PM
romangg added a subscriber: romangg.

While the general idea of providing a rotation with compositing is good before more work is put into this diff let me make this clear: we need to restart the effort after we have established clear rotation API on AbstractOutput. A first step for this is T11459 to reduce the level of data duplication we currently have. The next step is to clearly define which output sizes we have and how they are passed along to the backend plugins. For example the term pixelSize is ambiguous since it's not clear if we mean the rotated or not rotated size of the output and its mode.

Then our API for output rotation must work with the libdrm API first and only if this works flawlessly we can think of a fallback in cases where it doesn't. That the current code in the DRM backend doesn't work should not mean to just add alternative code paths, in this case with composited rotation, added on top but means that it either needs to get removed or reworked.

The description of the change here and presentation of overall idea is not sufficient. To judge on the grand design there needs to be more thought put into presenting it. Without it this change is also too large and too invasive. It must be assumed that we need a different approach or structure if we would need to changed that much in effects and so on.

These concerns should have been raised before so many details have been nitpicked in this review and corrected. I am sorry for the lost time on that. Please in the future just ignore nitpicks on code style until the overall idea and structure has been approved.

This revision now requires changes to proceed.Aug 27 2019, 5:37 PM
apol added a comment.Aug 28 2019, 12:52 PM

While the general idea of providing a rotation with compositing is good before more work is put into this diff let me make this clear: we need to restart the effort after we have established clear rotation API on AbstractOutput. A first step for this is T11459 to reduce the level of data duplication we currently have. The next step is to clearly define which output sizes we have and how they are passed along to the backend plugins. For example the term pixelSize is ambiguous since it's not clear if we mean the rotated or not rotated size of the output and its mode.

It doesn't make any sense to offer a vertical resolution (where h>w) if we are asking for a portrait orientation.

Then our API for output rotation must work with the libdrm API first and only if this works flawlessly we can think of a fallback in cases where it doesn't. That the current code in the DRM backend doesn't work should not mean to just add alternative code paths, in this case with composited rotation, added on top but means that it either needs to get removed or reworked.

The description of the change here and presentation of overall idea is not sufficient. To judge on the grand design there needs to be more thought put into presenting it. Without it this change is also too large and too invasive. It must be assumed that we need a different approach or structure if we would need to changed that much in effects and so on.

I wouldn't say it makes sense to use code cleanness or API as an excuse to give a poorer experience to our users. Display rotation is something we need and through software rotation is how it's implemented on most implementations. I'm all ears (eyes?) as to how this patch could be done better, but it needs to be something along the lines.

apol updated this revision to Diff 67617.Oct 10 2019, 3:17 PM

Rebase on top of origin/master

Then our API for output rotation must work with the libdrm API first and only if this works flawlessly we can think of a fallback in cases where it doesn't.

I don't follow. If we have this, we can just kill the libdrm rotation.

odeda awarded a token.Dec 8 2019, 5:46 PM
odeda added a subscriber: odeda.
ngraham added a subscriber: ngraham.Dec 9 2019, 9:30 PM

Is there any way we can come up with an actionable path forward? T11459 was listed as a prerequisite but I see that everything in there is completed. @romangg?

apol abandoned this revision.Mar 9 2020, 4:52 PM