Bug 409762

Can you attach an equivalent or similar screenshot taken by Spectacle on X11 for comparison's sake?

Created attachment 121539 [details]
Spectacle X11

Screenshot taken with spectacle on X11

Created attachment 121540 [details]
Spectacle Wayland

Screenshot taken with spectacle on wayland

@Nate Thanks a bunch for responding. I've added two screenshots with Konsole fully maximized on the left half of the screen.

One thing I noticed is that they wayland screenshot has only half the resolution. That's why the file sizes are wildly different. That peaked my curiosity and I took another screenshot with display scaling set to 1x instead of the default 2x. Et voila spectacle takes a screenshot with the right resolution.

Thanks, that's very helpful! It looks like the X11 screenshot is actually higher resolution than the Wayland one, which would explain why it's sharper. Can you confirm?

Yup, I can confirm that the X11 screen is the proper screen resolution and that the wayland screenshot exactly half of that. The bug only occurs when the scaling setting in KDE's "Display Settings" page is set to something other than 1x. In my case that's 2x so spectacle seems to think it'd only need to use half the resolution.

Thanks for the info!

*** This bug has been marked as a duplicate of bug 414296 ***

*** Bug 414296 has been marked as a duplicate of this bug. ***

I believe the fix needs to be made in kwin/effects/screenshot/screenshot.cpp:607 :

QImage ScreenShotEffect::blitScreenshot(const QRect &geometry)
{
    QImage img;
    if (effects->isOpenGLCompositing())
    {
        img = QImage(geometry.size(), QImage::Format_ARGB32);
        if (GLRenderTarget::blitSupported() && !GLPlatform::instance()->isGLES()) {
            GLTexture tex(GL_RGBA8, geometry.width(), geometry.height());
            GLRenderTarget target(tex);
            target.blitFromFramebuffer(geometry);
            // copy content from framebuffer into image
            tex.bind();
            glGetTexImage(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_BYTE, (GLvoid*)img.bits());
            tex.unbind();
        } else {
            glReadPixels(0, 0, img.width(), img.height(), GL_RGBA, GL_UNSIGNED_BYTE, (GLvoid*)img.bits());
        }
        ScreenShotEffect::convertFromGLImage(img, geometry.width(), geometry.height());
    }
...

To have an output image whose output image is scaled up.

Nice, feel free to send a patch.

*** Bug 418313 has been marked as a duplicate of this bug. ***

*** Bug 418881 has been marked as a duplicate of this bug. ***

Scaled Single screen screenshots are now fixed by :
https://phabricator.kde.org/D29010

(Multi screen is not ideal, rectangular selection does not work correctly ATM)

Not fixed. My display is 1080p, I changed its scale to 125% and Spectacle takes screenshot with 1.536x864 resolution.

Operating System: Arch Linux 
KDE Plasma Version: 5.18.90
KDE Frameworks Version: 5.70.0
Qt Version: 5.15.0 rc2

You are right Patrick, this depends on a spectacle patch, and change in :
https://phabricator.kde.org/D29282#inline-169560

*** Bug 425226 has been marked as a duplicate of this bug. ***

(In reply to Méven Car from comment #16)
> You are right Patrick, this depends on a spectacle patch, and change in :
> https://phabricator.kde.org/D29282#inline-169560

This linked patch looks to me like it is only (or mostly?) about multi-screen. Might there be a way to make this work on single screen before handling the special issues with multi-screen? Currently the conversation there looks like it might be either blocked currently or need some additional work to incorporate the suggestion from David Edmundson there.

(In reply to Claudius Ellsel from comment #18)
> (In reply to Méven Car from comment #16)
> > You are right Patrick, this depends on a spectacle patch, and change in :
> > https://phabricator.kde.org/D29282#inline-169560
> 
> This linked patch looks to me like it is only (or mostly?) about
> multi-screen. Might there be a way to make this work on single screen before
> handling the special issues with multi-screen? Currently the conversation
> there looks like it might be either blocked currently or need some
> additional work to incorporate the suggestion from David Edmundson there.

The right commit relating to this is https://phabricator.kde.org/D29010
But https://phabricator.kde.org/D29122 introduced a change since that introduced a regression.

https://invent.kde.org/plasma/kwin/-/merge_requests/194 will fix the "Current screen" case.

But for muliti-screen cases should we always take screenshots in native (not downscaled)  ?
For multiscreen screenshots, this results in distorted results.
Or Should I expose a choice, or default to non-native for multiscreen with a setting ?

Git commit 9a8b6f730d8d516594f159dbb41aa1357d65055d by Méven Car.
Committed on 19/08/2020 at 10:23.
Pushed by meven into branch 'master'.

Make ScreenShotEffect::screenshotScreen return native screen sized images

Aka not-downscaled images.

M  +2    -0    effects/screenshot/screenshot.cpp

https://invent.kde.org/plasma/kwin/commit/9a8b6f730d8d516594f159dbb41aa1357d65055d

(In reply to Méven Car from comment #19)
> https://invent.kde.org/plasma/kwin/-/merge_requests/194 will fix the
> "Current screen" case.

Thanks for the quick fix!

> But for muliti-screen cases should we always take screenshots in native (not
> downscaled)  ?
> For multiscreen screenshots, this results in distorted results.
> Or Should I expose a choice, or default to non-native for multiscreen with a
> setting ?

I guess with "distorted results" you are referring to problems like shown in https://phabricator.kde.org/D25427#648841 for example, where the screenshot taken looks completely different to what the user sees?

I like the idea to have a choice between native resolution and downscaled for multi-screen cases. Or maybe always downscale and only do native screenshots on multi-screen setups when the user does a "current screen" screenshot. But I guess having an option would not hurt if it does not make the interface too complicated for the user.

Also it might be good to have some info text for each option noting the downsides of the selected one (either downscaled or looking unproportional).

As a user I'd expect screenshots to be always taken in the native resolution, regardless of whether I have multiple screens attached or not. I'd be confused if there is any downscaling happening behind the scenes. When doing a screenshot the point is to capture everything as is, and anything else seems to take away from that.

It would help if snapshots were saved with DPI information. A viewer could then decide if the images need to be scaled.

(In reply to Marvin Hagemeister from comment #22)
> As a user I'd expect screenshots to be always taken in the native
> resolution, regardless of whether I have multiple screens attached or not.
> I'd be confused if there is any downscaling happening behind the scenes.
> When doing a screenshot the point is to capture everything as is, and
> anything else seems to take away from that.

So I have opposite opinions, a setting/option seems to be necessary.
It will be wayland only, not-native by default (current screen is always native now so it will be a fast path).

(In reply to Christoph Feck from comment #23)
> It would help if snapshots were saved with DPI information. A viewer could
> then decide if the images need to be scaled.

That's not really possible for multiple screens with multiple scale factors...

(In reply to Marvin Hagemeister from comment #22)
> As a user I'd expect screenshots to be always taken in the native
> resolution, regardless of whether I have multiple screens attached or not.
> I'd be confused if there is any downscaling happening behind the scenes.
> When doing a screenshot the point is to capture everything as is, and
> anything else seems to take away from that.

The problem with capturing everything "as is" is that you cannot really do that. From my understanding on multiscreen setups one large image will be created containing the content of all screens. With scaling enabled, you could either use the native resolution resulting in overlapping windows not matching for example (while they match perfectly when just viewing the screens because both have different DPIs). This is displayed in the post on Phabricator I linked to in comment #21.

To make things look as the user sees them, one has to use down- or upscaling in order to get multiple screens on one single image.

I just want to make sure you are aware of the facts, since I suspect you did not know that before? I think you agree that we at least need an option to scale in such circumstances so the user can get properly aligned result if they want to.

I think this also is the reason, why the suggestion from comment #23 will not work, since all screens are put into one single image.

(In reply to Méven Car from comment #24)
> (In reply to Marvin Hagemeister from comment #22)
> So I have opposite opinions, a setting/option seems to be necessary.
> It will be wayland only, not-native by default (current screen is always
> native now so it will be a fast path).

Sounds good to me!

Git commit 4a3cb5140cc58d73b1f0f0083d7f24f9f3a40b2a by Nate Graham, on behalf of Méven Car.
Committed on 20/11/2020 at 14:21.
Pushed by ngraham into branch 'release/20.12'.

Support Wayland rectangular selection HiDpi

When the drawn rectangle is on a single screen, the output image uses the screen native resolution.
When the drawn rectangle is on screens with multiple scale factor, the output image is upscaled to correspond to the screen with the hightest scale factor to keep the image undistorted.

Adds also a "scaled" full screen mode, to capture screens content downscaled to screens virtualsize allowing for undistorted content albeit loosing details.

Requires Plasma 5.19.80+
Related: bug 420863
FIXED-IN: 20.12

M  +17   -9    src/Gui/KSWidget.cpp
M  +7    -1    src/Platforms/Platform.h
M  +133  -21   src/Platforms/PlatformKWinWayland.cpp
M  +11   -2    src/Platforms/PlatformKWinWayland.h
M  +1    -1    src/Platforms/PlatformNull.cpp
M  +15   -1    src/Platforms/PlatformXcb.cpp
A  +41   -0    src/QuickEditor/ComparableQPoint.h     [License: LGPL (v2+)]
M  +274  -91   src/QuickEditor/QuickEditor.cpp
M  +15   -3    src/QuickEditor/QuickEditor.h
M  +2    -1    src/SpectacleCommon.h
M  +35   -11   src/SpectacleCore.cpp
M  +1    -0    src/SpectacleCore.h

https://invent.kde.org/graphics/spectacle/commit/4a3cb5140cc58d73b1f0f0083d7f24f9f3a40b2a

Verified. Woohoooooooooo!