fix: scaling of mirror pixmap

[letsfindaway]

fix: do not scale the already scaled pixmap when drawing in UBScreenMirror

[maehne]

This PR fixes issue #618.

[letsfindaway]

Background

Some more background on this simple PR.

The class UBScreenMirror is used to grab screen content and to display it on another screen. It decouples the grabbing, which is e.g. timer controlled, from the painting, which is controlled by the Qt rendering system. As the source screen and the target screen may have different geometry, it also takes care about scaling of the image. Two methods in this class are relevant: grabPixmap and paintEvent.

First grabPixmap gets the source content. Depending on the type of source, it gets the content either from the widget or by grabbing screen content. This is not important here. Relevant are the last lines of that function:

OpenBoard/src/gui/UBScreenMirror.cpp

Lines 108 to 109 in d4b91d8

	if (!mLastPixmap.isNull())
	mLastPixmap = mLastPixmap.scaled(width(), height(), Qt::KeepAspectRatio, Qt::SmoothTransformation);

Here the pixmap is scaled so that it fits to the target. Note that UBScreenMirror inherits QWidget, so the width() and height() functions return the dimensions of that widget. The flag Qt::KeepAspectRatio is used to avoid distortion. The pixmap is scaled so that one of its sides has the same size than the target, while the other is less or equal.

The paintEvent method looks as follows:

OpenBoard/src/gui/UBScreenMirror.cpp

Lines 61 to 76 in d4b91d8

	void UBScreenMirror::paintEvent(QPaintEvent *event)
	{
	Q_UNUSED(event);
	QPainter painter(this);
	painter.fillRect(0, 0, width(), height(), QBrush(Qt::black));
	if (!mLastPixmap.isNull())
	{
	int x = (width() - mLastPixmap.width()) / 2;
	int y = (height() - mLastPixmap.height()) / 2;
	painter.drawPixmap(x, y, width(), height(), mLastPixmap);
	}
	}

Here first a black background is drawn. Then an x and y offset is computed to place the pixmap centered. Finally the pixmap is drawn using drawPixmap.

The original code uses an overload with the following signature:

void QPainter::drawPixmap(int x, int y, int width, int height, const QPixmap &pixmap)

This overload scales the pixmap according to the width and height parameters unconditionally without keeping the aspect ratio. It then positions the scaled image according to x and y. So here occurs a second scaling, which distorts the image, if source and target have different aspect ratios.

As the image was already properly scaled when grabbing, this scaling is not necessary. By just using the overload

void QPainter::drawPixmap(int x, int y, const QPixmap &pixmap)

we can avoid scaling and just position the pixmap in the center of the target widget.

[maehne]

Looks good to me!

[kaamui]

As I already explained, what prevents me from merging it is that your commit reverts a fix I made for another issue, reported by the test team (and I reproduced it too) where in macOS, with two screens (can't remember the exact config, but probably with some differences in devicepixelratio, or resolution (but with same aspect ratio), or something else, didn't really investigate)), the "mirror pixmap" was only occupying a small part of the screen.

[letsfindaway]

Your comment brought me to the following excerpt from the Qt documentation:

Image buffers such as QImage and QPixmap represent the raw pixels, and as a result do not operate in the device independent coordinate system described earlier. A QImage of size 400x400, with a device pixel ratio of 2.0, will fit a 200x200 QWindow on a high density (2x) display, or will be automatically down-scaled to 200x200 during drawing if targeting a normal density (1x) display. See Drawing High Resolution Versions of Pixmaps and Images for more details.

This would mean that neither your fix nor mine is correct. I will investigate further and try to provide something which is able to handle HiDPI.

[kaamui]

Your comment brought me to the following excerpt from the Qt documentation:

Image buffers such as QImage and QPixmap represent the raw pixels, and as a result do not operate in the device independent coordinate system described earlier. A QImage of size 400x400, with a device pixel ratio of 2.0, will fit a 200x200 QWindow on a high density (2x) display, or will be automatically down-scaled to 200x200 during drawing if targeting a normal density (1x) display. See Drawing High Resolution Versions of Pixmaps and Images for more details.

This would mean that neither your fix nor mine is correct. I will investigate further and try to provide something which is able to handle HiDPI.

exact

[letsfindaway]

Here my idea of what happened:

• The originating screen was a HiDPI display. When grabbing this screen, a QPixmap with a devicePixelRatio of 2.0 was created. So when e.g. grabbing a 1920 x 1080 HiDPI monitor, a 3840 x 2160 pixmap was created.
• When scaled after grabbing in UBScreenMirror.cpp, the width and height of the target screen was used, measured in device independent pixels. If it had the same resolution, then the QPixmap was scaled down from 3840 x 2160 to 1920 x 1080 pixels. Still the devicePixelRatio is 2.0.
• When now drawing this pixmap, the devicePixelRatio is taken into account and the pixmap is displayed at half the size.

The problem is, that device independent pixels as used for measuring the width and height of a widget were mixed with the size of a QPixmap, which always counts actual pixels in storage. I pushed an updated fix which should take that into account. However I cannot test it.

Note that there are some other places where QPixmap is scaled:

• The magnifier tool.
• UBGraphicsPixmapItem::mousePressEvent.
• UBGraphicsSvgItem::mousePressEvent.

When shortly looking over these places I think it is not a problem for the magnifier. I have however no idea what the latter two are actually doing.

BTW: Your fix helped, because then the pixmap was again scaled up for painting. But it creates a wrongly positioned and distorted image when the two displays don't have the same aspect ratio.

[letsfindaway]

Perhaps you might have again a look on this, whether it fits to both cases:

• Screens with different aspect ratio (here the current implementation is broken)
• Screens with different device pixel ratio (this was the MacOS problem)

In the current status of this PR I tried to address both, but it needs testing on MacOS.

[kaamui]

Hi,

I can't prioritize on it but I didn't forget it. Also, the issue was not exactly the one you spotted as the ratio was more of 10 than 2, but still it was maybe somehow related to the devicepixelratio. I'll look at it when possible

[kaamui]

As we are no longer supposed to have macOS machines with multiple screens in our schools, I'll assume the issue can no longer be experienced on our side, thus I can merge this long awaited request. If the issue reappears, at least I'll be able to analyse it further