In case it fails, tell the OpenGL scene how to rotate the scene.
Diff Detail
- Repository
- R108 KWin
- Branch
- master
- Lint
Lint Skipped Excuse: aaa - Unit
No Unit Test Coverage - Build Status
Buildable 9851 Build 9869: arc lint + arc unit
abstract_output.h | ||
---|---|---|
121 | /** * Returns ... **/ virtual int softwareRotationAngle() const { return 0; } | |
outputscreens.cpp | ||
117 ↗ | (On Diff #54885) | return 0 |
platformsupport/scenes/opengl/abstract_egl_backend.cpp | ||
31 ↗ | (On Diff #54885) | This is a kwin core include, thus it should go above. |
platformsupport/scenes/opengl/abstract_egl_backend.h | ||
81 ↗ | (On Diff #54885) | Pointers have to be aligned to the right, i.e. AbstractOutput *output, |
plugins/platforms/drm/drm_output.h | ||
90 | int softwareRotationAngle() const { return m_softwareRotationAngle; } | |
plugins/scenes/opengl/scene_opengl.cpp | ||
659 | const int rotation | |
981–982 | Missing braces as well spaces after commas. |
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.
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]
plugins/platforms/virtual/virtual_output.h | ||
---|---|---|
53 ↗ | (On Diff #55009) | Style. |
plugins/platforms/virtual/virtual_output.h | ||
---|---|---|
53 ↗ | (On Diff #55009) | Also, why 90? |
plugins/platforms/virtual/virtual_output.h | ||
---|---|---|
53 ↗ | (On Diff #55009) | I just added that to test locally rotation. Reverted to 0. |
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 | ||
---|---|---|
115 ↗ | (On Diff #55375) | 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 | ||
27 ↗ | (On Diff #55375) | Please move it before "composite.h" |
plugins/platforms/drm/drm_output.cpp | ||
882–900 | 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 | ||
242 | Abuse of auto. | |
plugins/platforms/virtual/virtual_output.h | ||
52 ↗ | (On Diff #55375) | Please remove extra new line. |
screens.cpp | ||
292 | Please remove extra new line. |
plugins/platforms/drm/drm_output.cpp | ||
---|---|---|
882–900 | I would rather not. |
plugins/platforms/drm/egl_gbm_backend.cpp | ||
---|---|---|
242 | 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. |
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.
Pass screen rotation information to effects and fix taking screenshots of rotated views
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
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.
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.
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.