fix scaling of cover art on high pixel density screens #2247

Be-ing · 2019-08-20T18:04:58Z

Before, cover art images were getting scaled down to logical/device independent/1x pixel scale then automatically scaled up by the screen scale ratio which made them look blocky. By using QPixmap/QImage::setDevicePixelRatio and drawing QPixmaps/QImages at sizes scaled by the device pixel ratio, the cover art images only get scaled once to the size they are really shown at on screen.

Unfortunately there is a regression that somehow makes only one CoverArtDelegate show a cover at a time. I tried increasing the cache size by a factor of 100 and that made no difference. I would appreciate any hints where the problem might be with this.

Before:

After:

Before:

After:

Be-ing · 2019-08-20T18:10:44Z

src/library/coverartcache.cpp

@@ -48,6 +48,7 @@ CoverArtCache::~CoverArtCache() {
 QPixmap CoverArtCache::requestCover(const CoverInfo& requestInfo,
                                    const QObject* pRequestor,
                                    const int desiredWidth,
+                                    const double devicePixelRatio,


Note that the devicePixelRatio should be specified separately for each widget that accesses CoverArtCache. I thought about setting a global scale factor for CoverArtCache, but that would not work well in case widgets on different screens with different scale factors access the CoverArtCache singleton.

This is an important info why the design is as it is now. Please add the comment to the code as well.

uklotzde · 2019-08-21T14:54:26Z

The generated image now depends on devicePixelRatio and so this new parameter must be considered in pixmapCacheKey(). Maybe by scaling it with a fixed constant like 100 and then rounding to integer?

Be-ing · 2019-08-21T14:55:49Z

scaling it with a fixed constant like 100 and then rounding to integer

Why not just used the scaled size in the cache key?

uklotzde · 2019-08-21T17:01:00Z

But then I don't get the idea behind this PR. If only the image size matters, why let the cache know anything about the device pixel ratio if it doesn't affect the resulting images that are reused for matching requests? If you request the same size but with a different device pixel ratio you will always get the same image.

If on the other hand it does affect the cached image it must be reflected in the cache key. Otherwise the caches doesn't work as expected.

My first idea was that the cache should never care about where images are actually displayed. It should simply pre-compute and store images of different (pixel) sizes and the client needs to decide which size is appropriate for display. Maybe I don't understand how QImage::setDevicePixelRation() is working?

Be-ing · 2019-08-21T17:15:14Z

Maybe I don't understand how QImage::setDevicePixelRation() is working?

It's confusing and not documented very well IMO. Without CoverArtCache calling this, everywhere cover art is used would have to do it. As I understand it, if the devicePixelRatio of the QImage/QPixmap are not equal to the screen, Qt will automatically scale the QImage/QPixmap up when a QPainter draws it on screen. But if the QImage/QPixmap's devicePixelRatio is equal to the screen's, then Qt skips this automatic scaling. This also requires the real dimensions of the QImage/QPixmap to be scaled by the devicePixelRatio.

If on the other hand it does affect the cached image it must be reflected in the cache key.

I think you are right. I'll do that. However, this doesn't solve the issue with only one CoverArtDelegate's cover art getting drawn at a time. Do you have any ideas about that?

Be-ing · 2019-08-21T21:40:36Z

the issue with only one CoverArtDelegate's cover art getting drawn at a time

It turns out this had nothing to do with caching. I was inappropriately scaling the target QRect for the drawing target, so the covers were being drawn, but they were being drawn outside the boundaries of the CoverArtDelegate.

Ready to merge?

uklotzde · 2019-08-22T11:11:21Z

src/library/coverartcache.cpp

-            .arg(QString::number(hash)).arg(width);
+QString pixmapCacheKey(quint16 hash, int width, double devicePixelRatio) {
+    return QString("CoverArtCache_%1_%2_%3")
+            .arg(QString::number(hash)).arg(width).arg(devicePixelRatio);


I suggest to scale and round the device pixel ratio before formatting into an integer, e.g. 1.5 becomes "150":

...arg(static_cast<int>(std::round(devicePixelRatio * 100)))

Maybe add some assertions to detect unexpected values, e.g. devicePixelRatio >= 0.1 and devicePixelRatio <= 10.0 (for 10x up/down scaling)

uklotzde · 2019-08-22T11:40:25Z

I'm still not sure if the addition of device pixel ratio here is really necessary.

Assume that devicePixelRatio = 2.0 and you request the image with the original size by setting desiredWidth = 0. Then you get back a QImage, say 400x400. You also could have requested it with desiredWidth = 200 and devicePixelRatio = 2.0 and you still get the same 400x400 image. The parameter desiredWidth should actually be named desiredDeviceWidth:

desiredPixelWidth = desiredDeviceWidth * devicePixelRatio

For caching purposes only the pixel resolution of the image matters, but not the actual device resolution! Internally CoverArtCache could store all images with a devicePixelRatio of 1.0. If the image is requested with the separate parameters desiredDeviceWidth and devicePixelRatio it should invoke QImage::setDevicePixelRatio() just before returning the cached QImage by value. My hope is that the implicit sharing will not duplicate the actual image data when modifying this copied instance.

uklotzde · 2019-08-22T11:44:06Z

We need to ensure that the public API uses either actual pixels or (virtual) device pixels. If it uses the latter then the devicePixelRatio parameter is a mandatory parameter for all API calls that deal with sizes.

Be-ing · 2019-08-22T15:18:08Z

Internally CoverArtCache could store all images with a devicePixelRatio of 1.0. If the image is requested with the separate parameters desiredDeviceWidth and devicePixelRatio it should invoke QImage::setDevicePixelRatio() just before returning the cached QImage by value. My hope is that the implicit sharing will not duplicate the actual image data when modifying this copied instance.

Done. However, I think it is very unlikely that an image on another screen will be scaled to the same real pixel size as one on another screen with a different scale factor. The ratio of the device-independent scaled areas on different screens would need to be equal to the ratio of the devicePixelRatios of each screen. Also remember CoverArtCache only caches resized images; full size images do not get cached so there is nothing to share.

uklotzde · 2019-08-22T20:44:17Z

src/library/coverartcache.cpp

                                    const bool onlyCached,
                                    const bool signalWhenDone) {
    if (sDebug) {
        kLogger.debug() << "requestCover"
                 << requestInfo << pRequestor <<
-                desiredWidth << onlyCached << signalWhenDone;
+                deviceIndependentWidth << onlyCached << signalWhenDone;


New parameter devicePixelRatio is missing

uklotzde · 2019-08-22T20:44:40Z

src/library/coverartcache.cpp

        const bool signalWhenDone) {
    if (sDebug) {
        kLogger.debug() << "loadCover"
-                 << info << desiredWidth << signalWhenDone;
+                 << info << deviceIndependentWidth << signalWhenDone;


New parameter devicePixelRatio is missing

uklotzde · 2019-08-22T20:48:13Z

src/library/coverartcache.cpp

@@ -173,9 +182,10 @@ void CoverArtCache::coverLoaded() {
        // we have to be sure that res.cover.hash is unique
        // because insert replaces the images with the same key
        QString cacheKey = pixmapCacheKey(
-                res.cover.hash, res.cover.resizedToWidth);
+                res.cover.hash, res.cover.resizedToWidth * res.cover.devicePixelRatio);


The multiplication could be wrapped into a member function CoverArt::actualWidth()

uklotzde · 2019-08-22T20:50:19Z

src/library/coverartcache.h

@@ -26,11 +26,13 @@ class CoverArtCache : public QObject, public Singleton<CoverArtCache> {
    QPixmap requestCover(const CoverInfo& info,
                         const QObject* pRequestor,
                         const int desiredWidth,
+                         const double devicePixelRatio,


Please finish the renaming in the header file: desiredWidth -> deviceIndependentWidth

uklotzde · 2019-08-22T20:56:05Z

src/library/coverartdelegate.cpp

-        QRect source(0, 0, target.width(), target.height());
+        int width = math_min(pixmap.width(), option.rect.width()) * scaleFactor;
+        int height = math_min(pixmap.height(), option.rect.height()) * scaleFactor;
+        QRect target(option.rect.x(), option.rect.y(), width, height);


Why do we have to scale manually to the device resolution instead of letting Qt do the work by consistently using setDevicePixelRatio() and doing all sizing in device independent coordinates? This is how I thought it is supposed to work.

Hmm, I'll take another look at this logic. pixmap.width()/height() should be in scaled sizes already, but it looks like math_min picks the option.rect.width()/height(), which then needs to be scaled.

When I was first writing this, I multiplied by scaleFactor everywhere until I got it to work without really understanding what was going on. I may be able to simplify the code now...

All this manual scaling is confusing, I still don't get it. Why doesn't Qt provide a consistent measurement system? My assumption was that setting device pixel ratio does all the magic for us. These conversions back and forth between different coordinate systems are really error prone.

I agree, it is awfully confusing.

I think the original code is now outdated since we have non ^2 scale favtors.
Originally this code prevents us from upscaling an actual 15×15 to 20x20 for instance. This will likely look blurry. Instead we print the 15x15 and pad it for 20x20

This approach still makes sense, but we need to considder the scaleFactor.
If it is 1 or 2 the code works. If it is 1.5 the code fails.
20x20 is in this case 30x30 and the pixmap will look perfectly scaled by 2.

I must admit that this is a constructed example, but it is relevant.

uklotzde · 2019-08-23T10:40:07Z

No matter what you set in QImage::setDeviceImageRatio(), the size of the image is always in actual pixels according to the docs:

 QSize layoutSize = image.size() / image.devicePixelRatio()

Now we are doing the transformation between device independent pixels twice, both inside the cache and outside when calculating the layout before painting the image. This is bad.

We should clearly separate the responsibilities to avoid confusion:

The CoverArtCache doesn't need to care about painting and should use actual pixels, bot in the API and internally.
The Caller is responsible for calculating the required actual pixel size and invoke setDeviceImageRatio() before painting , because it needs to do it anyway when calculating and composing the layout

Would this help reduce the number of transformations and keep them within a single abstraction layer? Because now the multiplication (device independent size -> actual pixel size) is done within the cache and whereas the division (actual pixel size -> device independent size) is done by the widget. The widgets should solely be responsible for this transformation.

Or you wrap the QImage into some helper class DeviceIndependentImage that is responsible for these transformations and other than QImage presents its size in layout coordinates? Hacking this halfheartedly into QImage is a backward-compatible but rather ugly workaround that Qt has chosen here. In Rust the newtype pattern is a really powerful and ubiquitous idiom that enforces type safety at compile time 😄

daschuer

I had a brief look.

daschuer · 2019-08-23T10:43:25Z

src/library/coverartcache.cpp

@@ -48,6 +48,7 @@ CoverArtCache::~CoverArtCache() {
 QPixmap CoverArtCache::requestCover(const CoverInfo& requestInfo,
                                    const QObject* pRequestor,
                                    const int desiredWidth,
+                                    const double devicePixelRatio,


This is an important info why the design is as it is now. Please add the comment to the code as well.

src/library/coverartcache.cpp

daschuer · 2019-08-23T10:59:26Z

src/library/coverartdelegate.cpp

-        QRect target(option.rect.x(), option.rect.y(),
-                     width, height);
-        QRect source(0, 0, target.width(), target.height());
+        int width = math_min(pixmap.width(), option.rect.width()) * scaleFactor;


math_min compares the actualWidth of the pimamp with the device independent width from opition. I think this is desired here, but needs a comment.

The pixmap dimensions are measured in device coords, but what about option.rect, source, target?

I agree with Daniel that this code seems to be incorrect, because it mixes up different measurements. The final multiplication scales the pixmap's coords up again.

After some experimentation I realized QPainter::drawPixmap(QRect, QPixmap, QRect) wasn't working how I thought at all and it was also overcomplicated. The simpler QPainter::drawPixmap(QPoint, QPixmap) works fine without any complicated size conversions.

daschuer · 2019-08-23T11:17:11Z

src/library/coverartdelegate.cpp

-        QRect source(0, 0, target.width(), target.height());
+        int width = math_min(pixmap.width(), option.rect.width()) * scaleFactor;
+        int height = math_min(pixmap.height(), option.rect.height()) * scaleFactor;
+        QRect target(option.rect.x(), option.rect.y(), width, height);


I think the original code is now outdated since we have non ^2 scale favtors.
Originally this code prevents us from upscaling an actual 15×15 to 20x20 for instance. This will likely look blurry. Instead we print the 15x15 and pad it for 20x20

This approach still makes sense, but we need to considder the scaleFactor.
If it is 1 or 2 the code works. If it is 1.5 the code fails.
20x20 is in this case 30x30 and the pixmap will look perfectly scaled by 2.

I must admit that this is a constructed example, but it is relevant.

Be-ing · 2019-10-24T13:35:46Z

There ended up being commits that just reverted changes from earlier commits in this branch. Shall I rebase to condense this down to one commit?

uklotzde · 2019-10-24T13:51:25Z

Yes, that's a good idea.

Be-ing · 2019-10-30T22:07:35Z

@uklotzde @daschuer ping. I'd like to merge this and release 2.2.3.

uklotzde · 2019-10-31T10:10:22Z

Ok, I'll check this PR asap.

Unfortunately, I'm currently experiencing issues on Fedora 31: https://bugs.launchpad.net/mixxx/+bug/1850729

daschuer · 2019-10-31T11:55:26Z

I guess the "resize to a integer scale" code is not working for HiDPI screens in coverartdelegate.cpp. See the outdated discussion above.
We need to check.

src/library/coverartdelegate.cpp

src/widget/wcoverart.cpp

uklotzde · 2019-10-31T19:39:44Z

Just some minor nitpicks, otherwise reasonable and concise. LGTM.

Be-ing · 2019-11-06T01:12:58Z

I guess the "resize to a integer scale" code is not working for HiDPI screens in coverartdelegate.cpp. See the outdated discussion above.
We need to check.

I don't know what you mean. It looks fine on my screen, otherwise I would not have pushed the code.

Holzhaus · 2019-11-08T12:20:45Z

I tried to build this branch, but I'm getting errors:

$ rm -rf lin64_build/ .scon* cache/ config.log *.plist scons.txt
$ scons -j16
[...]
In file included from src/controllers/dlgcontrollerlearning.cpp:12:
src/controllers/dlgcontrollerlearning.h:16:10: fatal error: controllers/ui_dlgcontrollerlearning.h: No such file or directory
   16 | #include "controllers/ui_dlgcontrollerlearning.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
scons: *** [lin64_build/controllers/dlgcontrollerlearning.o] Error 1
In file included from src/controllers/dlgprefcontroller.h:18,
                 from src/controllers/dlgprefcontroller.cpp:16:
src/controllers/dlgcontrollerlearning.h:16:10: fatal error: controllers/ui_dlgcontrollerlearning.h: No such file or directory
   16 | #include "controllers/ui_dlgcontrollerlearning.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
scons: *** [lin64_build/controllers/dlgprefcontroller.o] Error 1
scons: building terminated because of errors.

uklotzde · 2019-11-08T13:05:47Z

@Holzhaus The 2.2.x build still requires Python 2. We didn't backport your Python 3 fixes.

Holzhaus · 2019-11-12T14:15:34Z

Ok, I patched in support for Python 3. Builds fine, and the coverart looks much less pixelated on my 4K screen. Unfortunately, the coverart in the bottom left corner (below the sidebar) is not centered anymore.

Before:

After:

daschuer · 2019-11-12T14:34:27Z

Can you also confirm the blurryness issue shown in #2247 (comment)
To see it you need to scale Mixxx for a cover art size slightly above it's maybe size.

Holzhaus · 2019-11-12T15:17:10Z

Can you also confirm the blurryness issue shown in #2247 (comment)
To see it you need to scale Mixxx for a cover art size slightly above it's maybe size.

No, but I don't know if I did it correctly. I opened the coverart display, and then made the window slightly larger.

This branch is on the left, master is on the right:

However, if I do not resize the cover art windows and keep them at their default size, the fonts on right one looks slightly better to read despite being more pixelated. Is that the issue what you're referring to?

daschuer · 2019-11-12T21:57:17Z

No, not exactly. It looks like you have discovered an other issue. My issue happens in the cover art column. I have just double checked the issue and all of my covers are big enough that the dicovered corner case is not that relevant.

The issue that you have discovered is that the window size is scaled from the original pixel size to the display pixel size. With a FullHD screen the cover is displayed unscaled pixel by pixel. In your case it is upscaled using anti aliasing which adds some noise. Do you think it makes sence to open the window by default pixel correct? This means the full screen cover will be smaller by default on HighDPI screens.

Holzhaus · 2019-11-12T22:16:30Z

Is there no way to query the current scale factor from the application and then multiply it with the scale factor of the image, so that the actual size of the window on HighDPI screens stays the same?

If that is not possible, I'd probably keep the current behavior.

daschuer · 2019-11-12T22:50:19Z

It is possible, but you cannot have both.

I think we need to be clear of the use case of the DlgCoverArtFullSize. I think we orignally have displayed it to see the cover at 100 % pixel correct. If you have a device pixel ratio of 2, the cover will appear smaller that it will appear on a ratio 1 screen.

If we just want to show the cover big, there is no reason to show a 50 x 50 cover smaller than a 500 x 500 cover.

Currently we are stuck in the middle. The HiDPI screes see a anti aliased blurry cover, with no easy way to see it pixel accurate. Covers with less resolution are still displayed small, but also blurry.

daschuer · 2019-11-12T22:58:33Z

src/library/dlgcoverartfullsize.cpp

@@ -131,8 +132,9 @@ void DlgCoverArtFullSize::slotCoverFound(const QObject* pRequestor,
            dialogSize.scale(availableScreenSpace.width(), dialogSize.height(),
                             Qt::KeepAspectRatio);
        }
-        QPixmap resizedPixmap = m_pixmap.scaled(size(),


The issue is in line 125 above, where the logical dialogSize is taken form the native pixel size.
Can you try to use
QSize dialogSize = m_pixmap.size() / getDevicePixelRatioF(this);
there?

daschuer · 2019-11-14T16:00:34Z

@Be-ing: The full size view of the cover is the last open thing. This should be pixel exact as discussed above. From this size the user can resize to any size.
Can you take care of it?

ronso0 · 2019-11-17T00:06:11Z

@Be-ing Does this branch touch the right-click behaviour of the cover art widgets, or spinnies with cover? I'm trying to get the right-click ux sorted in #2359

Be-ing · 2019-11-24T19:19:07Z

In Mixxx 2.1, the cover art was displayed pixel exact without scaling on high DPI screens, so many covers were shown quite small. I prefer how it is here. I don't care that it isn't pixel exact and may have some minor aliasing artifacts. I did mind before when it was scaled twice and there were lots of artifacts.

Be-ing · 2019-11-24T19:23:19Z

The full size view of the cover is the last open thing.

I don't think there is any problem here, so ready for merge? @Holzhaus what do you think?

uklotzde · 2019-11-24T19:50:57Z

LGTM. I rely on the opinion of those that are affected by scaling issues.

One option could be to scale the image up until at least one side of its dimensions is half the size of the corresponding screen dimension. Just an idea, does not need to be solved now.

Be-ing · 2019-11-24T19:53:14Z

I don't think there is a pressing need to make the scaling any more complicated. This is DJ software, not a raster graphics editor.

uklotzde · 2019-11-24T19:56:20Z

My remark was only intended to establish a sensible lower bound for the display to avoid diminishing small covers on HiDPI screens if you would decide to display the cover without scaling if possible. If we always apply a sensible scaling, then this distinction is not necessary.

daschuer · 2019-11-24T20:48:37Z

OK so we can merge. Thank you for clarifications.

Be-ing commented Aug 20, 2019

View reviewed changes

uklotzde reviewed Aug 22, 2019

View reviewed changes

Be-ing mentioned this pull request Aug 22, 2019

add hotcue text labels, position, and right click menu to overview waveform #2238

Merged

daschuer requested changes Aug 23, 2019

View reviewed changes

Be-ing force-pushed the coverart_scaling branch 4 times, most recently from 69b2d74 to 2998cae Compare August 27, 2019 21:05

Be-ing force-pushed the coverart_scaling branch from ccd039b to 6810aa4 Compare October 24, 2019 04:00

Be-ing added this to the 2.2.3 milestone Oct 24, 2019

fix scaling of cover art on high pixel density screens

17ce4c7

Be-ing force-pushed the coverart_scaling branch from 6810aa4 to 17ce4c7 Compare October 24, 2019 13:53

daschuer reviewed Oct 31, 2019

View reviewed changes

src/library/coverartdelegate.cpp Show resolved Hide resolved

uklotzde reviewed Oct 31, 2019

View reviewed changes

src/library/coverartdelegate.cpp Outdated Show resolved Hide resolved

uklotzde reviewed Oct 31, 2019

View reviewed changes

src/widget/wcoverart.cpp Show resolved Hide resolved

daschuer reviewed Nov 12, 2019

View reviewed changes

ronso0 mentioned this pull request Nov 17, 2019

pop up Spinny (with cover) context menu at click location #2359

Merged

use static_cast instead of dynamic_cast

71e7ce4

daschuer approved these changes Nov 24, 2019

View reviewed changes

daschuer merged commit e2eba3a into mixxxdj:2.2 Nov 24, 2019

ronso0 mentioned this pull request Nov 28, 2019

Skins :: adjust styles of skin context menus, tooltip… 2nd attempt #2363

Merged

10 tasks

mixxxbot mentioned this pull request Aug 23, 2022

CoverArt default cover doesn't scale well #9165

Open

fix scaling of cover art on high pixel density screens #2247

fix scaling of cover art on high pixel density screens #2247

Conversation

Be-ing commented Aug 20, 2019 • edited Loading

Be-ing Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Aug 21, 2019

Be-ing commented Aug 21, 2019

uklotzde commented Aug 21, 2019 • edited Loading

Be-ing commented Aug 21, 2019

Be-ing commented Aug 21, 2019

uklotzde Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

uklotzde commented Aug 22, 2019 • edited Loading

uklotzde commented Aug 22, 2019

Be-ing commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Aug 23, 2019

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Oct 24, 2019

uklotzde commented Oct 24, 2019

Be-ing commented Oct 30, 2019

uklotzde commented Oct 31, 2019

daschuer commented Oct 31, 2019

uklotzde commented Oct 31, 2019

Be-ing commented Nov 6, 2019

Holzhaus commented Nov 8, 2019

uklotzde commented Nov 8, 2019

Holzhaus commented Nov 12, 2019

daschuer commented Nov 12, 2019

Holzhaus commented Nov 12, 2019

daschuer commented Nov 12, 2019

Holzhaus commented Nov 12, 2019

daschuer commented Nov 12, 2019

Choose a reason for hiding this comment

daschuer commented Nov 14, 2019

ronso0 commented Nov 17, 2019

Be-ing commented Nov 24, 2019 • edited Loading

Be-ing commented Nov 24, 2019

uklotzde commented Nov 24, 2019

Be-ing commented Nov 24, 2019

uklotzde commented Nov 24, 2019 • edited Loading

daschuer commented Nov 24, 2019

Be-ing commented Aug 20, 2019 •

edited

Loading

Be-ing Aug 20, 2019 •

edited

Loading

uklotzde commented Aug 21, 2019 •

edited

Loading

uklotzde Aug 22, 2019 •

edited

Loading

uklotzde commented Aug 22, 2019 •

edited

Loading

Be-ing commented Nov 24, 2019 •

edited

Loading

uklotzde commented Nov 24, 2019 •

edited

Loading