Add highdpi support for RDP and VNC
Needs ReviewPublic

Authored by volkov on Apr 24 2019, 3:19 PM.

Details

Reviewers
davidedmundson
Group Reviewers
KDE Applications
Summary

In highdpi mode coordinates are specified in device independent pixels,
and Qt auto-scales everything by devicePixelRatio, i.e. the ratio between
the device independent and device pixel coordinate systems.

Thus for RDP we should pass the size in device pixels to xfreerdp.

For VNC we should avoid auto-scaling, which can be done by setting
devicePixelRatio of the paint device for the painted image, and
the size of the image should be converted to device independent
pixels where it's needed.

Diff Detail

Repository
R436 KRDC
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12932
Build 12950: arc lint + arc unit
volkov requested review of this revision.Apr 24 2019, 3:19 PM
volkov created this revision.
volkov updated this revision to Diff 57055.Apr 26 2019, 4:33 PM

polished

volkov retitled this revision from WIP: Add highdpi support for RDP and VNC to Add highdpi support for RDP and VNC.Apr 26 2019, 4:35 PM
volkov added a reviewer: KDE Applications.
aacid added a subscriber: aacid.May 1 2019, 9:26 AM

it looks sane to me, but otoh i don't have a hidpi display to test so not really an expert in this field.

Would you mind writing a long-ish comment in the commit (use arc diff --edit --verbatim to reupload the new text to phabricator) for the next person that tries to read the code?

I could test with hidpi screens, but I need to understand what was the problem and how is it fixed.

ngraham added a subscriber: ngraham.May 1 2019, 6:19 PM

BTW you can test even without a high DPI screen just by using KScreen's scaling or running QT_SCALE_FACTOR=<some real between 1.0 and 2.0> <executable name/path> It will simply be bigger, but any graphical glitches can still be seen.

I'm going to rebase it on D21096. Please, review that change first.

volkov updated this revision to Diff 57810.May 9 2019, 3:17 PM

rebased

volkov edited the summary of this revision. (Show Details)May 9 2019, 7:23 PM
aacid added a comment.Jun 9 2019, 10:29 PM

I don't know if it's what you where trying to fix or not, but when running with

QT_SCALE_FACTOR=2 krdc

i get this weird behaviour where the screen goes white while repainting.

See https://i.imgur.com/omnnTJD.gif

I don't get that running krdc without QT_SCALE_FACTOR

This change prevents highdpi scaling of the remote desktop.
It's double-scaled on your gif and looks pixelated.

alexeymin set the repository for this revision to R436 KRDC.Jun 13 2019, 10:48 AM
$ arc patch D20790
[INFO] Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D20790.


    This diff is against commit f35e70e99da9a59b95e27ad0a9bd8c824cbce8b1, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (87f3b07546387e2b78433581c9c26d2deaf0c36a)
    [Y/n]

Checking patch vnc/vncview.cpp...
error: while searching for:
    QPainter painter(this);
    painter.setRenderHint(QPainter::SmoothPixmapTransform);

    QRectF dstRect = event->rect();
    QRectF srcRect(dstRect.x() / m_horizontalFactor, dstRect.y() / m_verticalFactor,
                   dstRect.width() / m_horizontalFactor, dstRect.height() / m_verticalFactor);
    painter.drawImage(dstRect, m_frame, srcRect);

    RemoteView::paintEvent(event);

error: patch failed: vnc/vncview.cpp:537
Hunk #10 succeeded at 605 (offset -1 lines).
Checking patch vnc/vncclientthread.h...
Checking patch vnc/vncclientthread.cpp...
Checking patch rdp/rdpview.cpp...
Applying patch vnc/vncview.cpp with 1 reject...
Hunk #1 applied cleanly.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Hunk #4 applied cleanly.
Hunk #5 applied cleanly.
Hunk #6 applied cleanly.
Hunk #7 applied cleanly.
Hunk #8 applied cleanly.
Rejected hunk #9.
Hunk #10 applied cleanly.
Applied patch vnc/vncclientthread.h cleanly.
Applied patch vnc/vncclientthread.cpp cleanly.
Applied patch rdp/rdpview.cpp cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!
volkov updated this revision to Diff 59984.EditedJun 17 2019, 10:42 AM

rebased
thanks for noticing

This adds new warnings:

../vnc/vncclientthread.h: In constructor ‘VncClientThread::VncClientThread(QObject*)’:
../vnc/vncclientthread.h:191:19: warning: ‘VncClientThread::m_stopped’ will be initialized after [-Wreorder]
     volatile bool m_stopped;
                   ^~~~~~~~~
../vnc/vncclientthread.h:184:11: warning:   ‘qreal VncClientThread::m_devicePixelRatio’ [-Wreorder]
     qreal m_devicePixelRatio;
           ^~~~~~~~~~~~~~~~~~
../vnc/vncclientthread.cpp:323:1: warning:   when initialized here [-Wreorder]
 VncClientThread::VncClientThread(QObject *parent)

member declaration order in .h-file is mismatches initialization order in constructor.

The patch compiles and works.
I've built master branch and with this patch, and run both with QT_SCALE_FACTOR=2. During connection I chose maximum quality option (local network). The results look identical to me:
master on the left, patched D20790 on the right

How should it look like?

volkov updated this revision to Diff 60000.Jun 17 2019, 3:31 PM

fix initialization order

Have you installed libkrdc_vncplugin.so into the system?
KRDC doesn't search for plugins in the build tree.

Have you installed libkrdc_vncplugin.so into the system?
KRDC doesn't search for plugins in the build tree.

No, I was running from the build tree. :)

Now I see, local window is scaled, remote contents are not, is that the intention of this patch?

Yes, remote contents should not be scaled.

I have no questions to code, but.. This is a behavioral change and I think we need a mantainer's opinion here. Or at least somebody else's opinion.

My first thought was that if you have a 4K monitor and setup a scaling (150% or 200%) - you expect everything to become bigger, window elements, controls, fonts, .. window contents too. With this patch krdc window itself will look OK on your 4K screen on your 200% scaling, but remote contents will look tiny, twice smaller than you would expect. Is that OK? I'll try to make a screenshot later today or tomorrow..

This change allows to show remote contents as is.
For example, remote desktop may be started with QT_SCREEN_SCALE_FACTORS=2 and it will look good on highdpi monitor.