[Wayland] Allow to take full resolution screenshot when scaling is used
AbandonedPublic

Authored by meven on Nov 20 2019, 6:22 PM.

Details

Reviewers
davidedmundson
Group Reviewers
KWin
Summary

BUG: 409762
FIXED-IN: 5.19.0

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D25427
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25195
Build 25213: arc lint + arc unit
meven created this revision.Nov 20 2019, 6:22 PM
Restricted Application added a project: KWin. · View Herald TranscriptNov 20 2019, 6:22 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Nov 20 2019, 6:22 PM

This approach will probably fail when the geometry stretches across multiple outputs with different scales.

meven added a comment.Nov 20 2019, 6:53 PM

This approach will probably fail when the geometry stretches across multiple outputs with different scales.

I am well aware, this a naive and basic approach to gather some learning feedback.
And Those changes are only needed for Wayland.
I need to break the geometry by screen get their scale and then put all together in a virtualScreenGeometry according to the passed Geometry.
But I need to learn how to get there, dig in the code and/or ask questions if I may.
I have a two screens system at home to test this.

This approach will probably fail when the geometry stretches across multiple outputs with different scales.

I am well aware, this a naive and basic approach to gather some learning feedback.
And Those changes are only needed for Wayland.
I need to break the geometry by screen get their scale and then put all together in a virtualScreenGeometry according to the passed Geometry.
But I need to learn how to get there, dig in the code and/or ask questions if I may.
I have a two screens system at home to test this.

It's great that you are working on this! This was just a thought that came to me upon realizing that everyting goes though this method.

Hmm further thinking about this. Assuming a geometry stretches across two screens. On has a scaling of two and the other not. Would then one side of the image be double the size than the other one because we want the full resolution? Or would you scale the lower resolution half of the image up?

On has a scaling of two and the other not. Would then one side of the image be double the size than the other one because we want the full resolution? Or would you scale the lower resolution half of the image up?

I think it has to be uniform.

effects/screenshot/screenshot.cpp
651

Good question.

Use of Qt methods is slightly weird, as kwin implements it's own QPA which means we're reading things via an abstraction layer rather than directly. In theory it should work, but you're going to struggle to tie a QScreen to what's being rendered.

Ideally we would want to use KWin::Screens() but that's not exposed to effects.

GLRenderTarget::virtualScreenScale() is currently only set to screen->scale during the paint method. This is postPaint. We could set it during pre/postPaint too.

656–657

Avoid the term scaled in any variable name.

It can mean scaled from logical to device, or scaled from device to logical. Which means it fails to convey the one piece of important information you're trying to say.

use something like"deviceGeometry"

meven updated this revision to Diff 70097.Nov 21 2019, 10:52 AM
meven marked an inline comment as done.

Rename variable to proper name

meven added a comment.Nov 21 2019, 2:14 PM

GLRenderTarget::virtualScreenScale() is currently only set to screen->scale during the paint method.

I guess you are evoking in your comment scene_opengl.cpp:662.
There for m_backend->perScreenRendering (wayland case) SceneOpenGL::paint() calls Scene::paintScreen() that then calls effects->postPaintScreen().
I don't get how the static data at GLRenderTarget::virtualScreenScale are not available.
I don't see where some other code might call postPaintScreen.
I am missing something. If you could just point me to the right direction.

This is postPaint. We could set it during pre/postPaint too.

I'd like to understand how to make it available to the effects.
I dug D8490 and D4948 for inspiration.

meven added a comment.Jan 6 2020, 8:42 PM

let me know if you'd have some time @davidedmundson

meven added a subscriber: zzag.Jan 29 2020, 6:05 AM

@zzag @davidedmundson any recommendation on how to make device pixel ratio/scale per screen available to effects ?

zzag added a comment.Jan 29 2020, 9:36 AM

I think it has to be uniform.

Yes, it has to be uniform. The most sensible choice is to use the maximum scale factor.

You could use QGuiApplication::screens() to get a list of all screens or if we want use our own output classes, then someone needs to introduce EffectOutput or something to libkwineffects and use it here.

meven updated this revision to Diff 79135.Apr 2 2020, 12:51 PM

Handle the single screen case, passing through the screen scale

meven edited the summary of this revision. (Show Details)Apr 2 2020, 2:36 PM
meven added a comment.Apr 6 2020, 3:00 PM

How

In D25427#602293, @zzag wrote:

I think it has to be uniform.

Yes, it has to be uniform. The most sensible choice is to use the maximum scale factor.

Mm not sure, this is supposed to get used by spectacle that will use this output to fill the screens for its QuickEditor, scaling it up for lower density screen will mess this up.

meven added a comment.Apr 6 2020, 3:15 PM

How

In D25427#602293, @zzag wrote:

I think it has to be uniform.

Yes, it has to be uniform. The most sensible choice is to use the maximum scale factor.

Mm not sure, this is supposed to get used by spectacle that will use this output to fill the screens for its QuickEditor, scaling it up for lower density screen will mess this up.
And since the screenshot would not be pixel-perfect, this will make things like bug report less precise.

meven updated this revision to Diff 79493.Apr 6 2020, 3:17 PM

Allow to make screenshots of multiple screens, each using their scaling factor

meven retitled this revision from [WIP][Wayland] Allow to take full resolution screenshot when scaling is used to [Wayland] Allow to take full resolution screenshot when scaling is used.Apr 6 2020, 3:49 PM

Mm not sure,

Let's decide this before we spend more time writing and reviewing more code.

this is supposed to get used by spectacle that will use this output to fill the screens for its QuickEditor, scaling it up for lower density screen will mess this up.

I don't understand. Does spectacle map back to the original geometry? How does it resolve the problem of windows being cut in half and basically unusable.

Note also that this method is exposed to xdg-desktop-portal so potentially any app.

meven updated this revision to Diff 79503.Apr 6 2020, 3:55 PM

Undo change to GLRenderTarget::blitFromFramebuffer

To make sure we're talking about the same thing, this is what I'm expecting that we'll generate that will be problematic

http://static.davidedmundson.co.uk/blog/kwin_wayland_dpi/to_render.png

meven edited the summary of this revision. (Show Details)Apr 6 2020, 3:56 PM
meven updated this revision to Diff 79589.Apr 7 2020, 3:29 PM

Don't need set to set a DevicePixelRatio on blipped images

meven added a comment.EditedApr 7 2020, 3:30 PM

To make sure we're talking about the same thing, this is what I'm expecting that we'll generate that will be problematic

http://static.davidedmundson.co.uk/blog/kwin_wayland_dpi/to_render.png

At the moment my patch does this kind of output (that is not ideal).
(the smaller screen has a 1.25 dpr)

The position of the image are just ones on top of each other.
While I'd like to get them at the virtualGeometry positions them.

libkwineffects/kwinglutils.cpp
1290 ↗(On Diff #79493)

Those lines looked to me suspicious, I think I corrected them and didn't notice any issue with my changes.

meven marked an inline comment as done.Apr 8 2020, 2:40 PM
This comment was removed by meven.
meven updated this revision to Diff 80077.Apr 14 2020, 10:25 AM

Allow to take multi screen screenshot wayland at proper positions, propagate globalPos to ScreenPaintData

Example correct result im Wayland with secondary smaller screen using 1.25 scaling :

No result change for Xorg.

meven updated this revision to Diff 80078.Apr 14 2020, 10:34 AM

Improved comments and code position

davidedmundson requested changes to this revision.Apr 15 2020, 11:16 AM

I said at the very start we don't want to have things at their native scales across spanned outputs because it will just return something completely unusable where windows in the same file appear different sizes.

Running this patch gives

which is exactly the same as what I predicted it would look like which is problematic.

I'm happy to argue a case of why we should do this and explore compromises, but doing any code changes or reviews before then is not a good use of time.

This revision now requires changes to proceed.Apr 15 2020, 11:16 AM

I'm happy to argue a case of why we should do this and explore compromises, but doing any code changes or reviews before then is not a good use of time.

Let's argue then.

For single screen, there is nothing to argue, I believe, the patch solves the issue : do not downscale.

For multi-screen, my two cents are that we need to cover both the uses cases : user scaled, and pixel precision.

  • pixel precision is needed for use cases such as debugging, "real" pixel screenshot, screenshot apps such as spectacle to allow them cropping anything on the screen without loss, last resort screenshot option.
  • user scaled: is needed for kde users to show off how beautiful their desktop is, an overview of , to show multi-screen apps (those are rare I'd say, games mostly), screenshot apps.

If that sound reasonable I will add a screenshotFullscreenScaled or a boolean scaled argument to screenshotFullscreen, to the screenshotEffect.

For single screen, there is nothing to argue, I believe, the patch solves the issue : do not downscale.

Ack. The changes to blitScreenshot are all good to go. +2 on that bit.

pixel precision is needed for use cases such as debugging, "real" pixel screenshot, screenshot apps such as spectacle

The spectacle part is where I'm a bit lost. Could be that you have additional plans there.

Spectacle (currently) shows one mega-window spanning across all screens showing one image for its crop/select area function. Because it's all one window all contents need to be at the same scale as that wl_surface only has 1 scale.

I don't see how Spectacle could do anything useful with this unless it got the image, then mapped it back to the screen geometry and split it up again into individual pieces.
At that point would it work better for spectacle to call DBus screenshotScreen(0); screenshotScreen(1); and getting native images back directly pre-split?

If that sound reasonable I will add a screenshotFullscreenScaled or a boolean scaled argument to screenshotFullscreen, to the screenshotEffect.

Remember I loathe the term "scaled", but in principle I would be ok with that if we can work out how spectacle would use this.

meven added a comment.Apr 15 2020, 2:30 PM

For single screen, there is nothing to argue, I believe, the patch solves the issue : do not downscale.

Ack. The changes to blitScreenshot are all good to go. +2 on that bit.

pixel precision is needed for use cases such as debugging, "real" pixel screenshot, screenshot apps such as spectacle

The spectacle part is where I'm a bit lost. Could be that you have additional plans there.

Spectacle (currently) shows one mega-window spanning across all screens showing one image for its crop/select area function. Because it's all one window all contents need to be at the same scale as that wl_surface only has 1 scale.

I don't see how Spectacle could do anything useful with this unless it got the image, then mapped it back to the screen geometry and split it up again into individual pieces.
At that point would it work better for spectacle to call DBus screenshotScreen(0); screenshotScreen(1); and getting native images back directly pre-split?

It could add latency to screens between screenshots, and it would suppose having some prior authorization to take screenshots.

A single call would be better I think.
Returning a list of images perhaps instead of all of them assembled.

If that sound reasonable I will add a screenshotFullscreenScaled or a boolean scaled argument to screenshotFullscreen, to the screenshotEffect.

Remember I loathe the term "scaled", but in principle I would be ok with that if we can work out how spectacle would use this.

I wonder what would a better word than scaled.
In spectacle, you would have another screenshot type "Multi Screen Linearly scaled".

Also assembling the images will be tricky, given we won't have any available geometry for this case.

Can you make sure your testing done from spectacle includes:

  • rectangular region
  • whole screen
  • window under cursor

We could also consider tackling this piecemeal and only changing the one screen case for now. I'd still like the testing above done, as I think this patch could break spectacle's region selector.


Returning a list of images perhaps instead of all of them assembled

It's a bit tricky as we take file descriptors in with the calling request.
So that would need to be taking N descriptors in, but I think that could work nicely.

meven planned changes to this revision.Apr 20 2020, 3:26 PM

After D29010 rebasing is in order

meven abandoned this revision.Apr 29 2020, 8:03 AM

In favor of D29122