What a nice surprise, thanks for your contribution. My first impression is quite positive, I'll try to test more thoroughly in the next days. I think Lukas should comment, too.

Do you know about the list of issues in D7581#163655 (1. specifically)? Is this all expected to work with your patch?

Is there a chance you can access the window the thumbnail is displayed in? Depending on which screen the window is, it might have a different device pixel ratio. Only as a fallback should it use the one in QApplication.

In D9078#173968, @broulik wrote:

Is there a chance you can access the window the thumbnail is displayed in? Depending on which screen the window is, it might have a different device pixel ratio. Only as a fallback should it use the one in QApplication.

@abalaji Note that this topic has already been discussed in D7581. Is your code taking this into account?

Hi, sorry for replying this late, I'm glad you're happy about my contribution. I'm currently using QApplication rather than QWindow, QScreen etc, just like it is in the rest of the work by Lukas (basically I just looked at what he did and tried to do the same). So it's probably not the best solution, but it does take care of the thumbnails, which look much better now.

EDIT: Just looked at D7581, using QApplication is right when generating the thumbnails but I should be using QPaintDevice while painting them it seems, so I'll need to get that sorted out

In D9078#173937, @rkflx wrote:

What a nice surprise, thanks for your contribution. My first impression is quite positive, I'll try to test more thoroughly in the next days. I think Lukas should comment, too.

Sorry I haven't got around to this yet, busy with other reviews. To help speed things up, you could let us know:

1. which things work and which do not:

Do you know about the list of issues in D7581#163655 (1. specifically)? Is this all expected to work with your patch?

2. whether drawing of the frames around the thumbnail pixmaps is scaled correctly already (use KMag or pixeltool-qt5 for that).

Do you know about the list of issues in D7581#163655 (1. specifically)? Is this all expected to work with your patch?

1. is the only issue fixed in this patch. The frames around the thumbnail pixmaps seem to be already working correctly: I cannot make out any issues with it.

There are still a couple of issues, so I guess it might take some time until this patch can land. Note I have neither tested all things I want to test yet nor done a full code review. I'll do those once more things work.

Before getting to the details, something more general: I wonder how this works wrt. caching of the thumbnails. Gwenview is supposed to use the standard thumbnails directory (and sometimes even thumbnails embedded in images?), so displaying of thumbnails is fast. Now in a HiDPI situation, these might not have a large enough resolution so they get recomputed(?), which is slow. This is probably out-of-scope for your patch, but I'd love to know whether you thought about this performance problem. Also, did you look at other apps what their approach regarding rendering HiDPI thumbnails is? Before just scaling everything up (or always recomputing everything making the caching obsolete), we should at least determine whether the "old" thumbnail architecture is still making sense at all or if we'd need a bigger rework. Please comment.

Next, what about your comment from above, is this still relevant/open?:

I should be using QPaintDevice while painting them it seems, so I'll need to get that sorted out

Now for some of the issues I noticed:

• Thumbnail Bar broken, regardless if I apply your patch standalone on master, on the tip of the gsoc branch or on the commit before the tip. Am I holding this wrong? I wonder, because you said 1. is fixed, but the Thumbnail Bar is listed there…
• Video thumbnails still pixelated, even though you said 1. was fixed.
• Thumbnails should fit the space available, but when previewing a very low-res image the thumbnail is also very tiny. Should we upscale in this case?

The frames around the thumbnail pixmaps seem to be already working correctly: I cannot make out any issues with it.

I can ;) Don't be shy on the scaling factor, just use something wild like 6x.

Here you'll see that the there is an outer blue border and a more greyish line next to it. This is fine on the top right and bottom, but in the top left corner some of the lighter blue suddenly shines through. This should not happen, perhaps some issue with the border radius?

Also the corner of the inner dark rectangle does look a bit odd, not sure this is rendered at HiDPI. This suspicion comes also from looking at the next screenshot:

You see that the shadows are still totally pixelated. The same root cause might also be the reason for the next issue:

The thumbnail is rendered beautifully, but does not fit inside the frame at all. On the top left, there is a gap between inner frame and image, while on the lower right there is overlap. Perhaps the pixmap of the frame is still low-DPI in both rendering and positioning? Also note that neither thumbnail nor inner frame are symmetrical in relation to the outer frame, even though they should be.

My final issue has to do with when the hi-res thumbnail becomes available:

Here I am advancing to the next image with Space. You see that there is a slight change in rendering quality shortly after that (some recomputation? why?). After this I press F5, only then the final quality level is reached. Ideally, the final stage should be reached without two intermediary steps and without having to press a key of course. The effect can also be seen when just navigating around with the arrow keys, where the thumbnails keep flickering.

The same issue also affects the rendering of the folder icons. Again I'm just repeating F5:

Note this not only is about the thumbnails changing their quality, but also the folder icon itself (horizontal grey bar changes height, 45° angled line gets pixelated).

That said, a lot of issues from 1. are fixed indeed, which is great ;)

In D9078#181876, @rkflx wrote:

You see that there is a slight change in rendering quality shortly after that (some recomputation? why?).

Stumbled upon this today, so I thought I'll let you know: That's caused by SMOOTH_DELAY, so probably not a problem with your patch and maybe even intentional.

Still, any deficiencies Gwenview already has regarding thumbnail scaling are more pronounced in HiDPI situations. Perhaps it makes sense to split this into multiple Diffs, which would be easier to understand and test, too:

• Finding and fixing existing bugs with thumbnail rendering with no scaling, so they don't get in the way of any later HiDPI work.
• Changes required by the need for higher resolution of the on-disk thumbnail cache (both retrieving and saving).
• Rework/fixes regarding rescaling/smoothing of existing thumbnails to the size required by the viewport.
• The actual rendering of HiDPI pixmaps (that's the part with QPixmap::setDevicePixelRatio).
• Fixing rendering of frames and shadows.

Note that the summary of the Diff will become the commit message, so this should be very clear in describing what problem was fixed and in which context. Currently I miss the big picture / plan a bit, because obviously some dpr here and there is not enough.

navigating around with the arrow keys, where the thumbnails keep flickering.

That's not something I've seen before, so probably a regression of the patch.

Nevertheless, keep up the good work ;)

Hi,
Hope everyone is enjoying the holidays!

Fixing rendering of frames and shadows.

I figured these out, so these are not pixelated anymore :). Going up to like 6x scaling helps a lot with identifying and fixing these issues, thanks @rkflx for the tip!

I should be using QPaintDevice while painting them it seems, so I'll need to get that sorted out

This was pointed out in D7581#141493, so qApp->devicePixelRatio() gives the largest value of all the DPR's of the connected monitors, and QPaintDevice->devicePixelRatio() gives the DPR of the current screen, so while painting, I need to use this value to ensure that the scaling is correct. This needs to be fixed in the main patch as well

Thumbnail Bar broken, regardless if I apply your patch standalone on master, on the tip of the gsoc branch or on the commit before the tip. Am I holding this wrong? I wonder, because you said 1. is fixed, but the Thumbnail Bar is listed there…
Video thumbnails still pixelated, even though you said 1. was fixed.

I guess I just fixed part 1 of the 1, sorry I missed all these cases. I'll try and get these fixed soon as well, sound be similar enough

Thumbnails should fit the space available, but when previewing a very low-res image the thumbnail is also very tiny. Should we upscale in this case?

Well, so if the image itself is tiny, then I don't think we should upscale, since that's the way it is currently, and we shouldn't change the existing behaviour

Can we just multiply with dpr here and elsewhere? I'm asking because before this could have two distinct values, but now depending on a fractional scaling factor this can vary wildly.

Maybe this is okay if this is just a threshold internal to Gwenview, but in case this is related to an external standard which specifies this sizes (thumbnail caching?) it would be a problem.

Could you research this a bit?

The thumbnail sizes are specified in thumbnailgroup.h. I think the thumbnails are stored in the thumbnail cache somewhere. I'm not too sure right now about where exactly the logic governing that is, but I think we should be storing high DPI thumbnails in the cache if the user has a hidpi display, and let QPaintDevice->devicePixelRatio() handle the scaling while painting. Either way, I think we should let thumbnailgroup.h specify the base "logical" thumbnail size, and scale that up accordingly, like I've done here. What's your opinion on this?

Finally got a chance to try your latest changes, here's how it looks for me:

Great work on the pixel shadow :)

However, the positioning of various frames and backdrops still seems a bit off (see my comments above for details). I even get this weird grey rectangle.

Regarding the thumbnail pixmaps and the thumbnail rendering:

I'm not too sure right now about where exactly the logic governing that is

I don't know Gwenview's code regarding this either, but I'm sure you'll figure it out. Perhaps share here what you've found, makes reviewing the patch easier/faster. In the end I just want to make sure we don't break anything.

Currently it is a bit unclear (to me, at least) what sizes come from the freedesktop.org thumbnail caching spec, what sizes are "virtual" dpi and what sizes are in physical screen coordinates. This should be made very clear (via comments or variable names), as well as which pixmaps are generated on the fly and which are taken from a cache.

Did you have a chance yet to look at how other applications (does GNOME have an image viewer?) handle the fact that the maximum thumbnail size is restricted to 256px in said spec? (I assume you've read and understood the links I already mentioned regarding this.)

QPaintDevice->devicePixelRatio() gives the DPR of the current screen, so while painting, I need to use this value to ensure that the scaling is correct. This needs to be fixed in the main patch as well.

Does your patch actually depend on the other patch? If not (at least for me it compiles fine without), you could just apply anything you think is necessary in your own code without waiting for the other patch to be updated (whenever that will be?).

Last thing: I mentioned this before, but I would highly recommend to split the patch (currently claiming to fix "everything") into multiple Diffs, each targeting a distinct problem. This highly increases the chances to land any code to the repo eventually, makes the commit messages clearer and is much faster to test and to review.

Let me know if you have any specific questions, and I try to help the best I can. You've chosen quite a big task for your first contribution to KDE ;)

Another thought I had: The thumbnail slider in Browse mode has a tooltip displaying the thumbnail size in pixels. Suppose the user moves the window from a low-res display (set to 1x scaling) to a HiDPI display (set to 2x scaling). Obviously, the size of the thumbnails (as measured with a tape measure) should stay the same, meaning the pixmap used to achieve this must become larger.

Now, what should we do about the size displayed in the slider tooltip? Should it also change, or should it stay at the same "virtual" size? Keep in mind that all the other sizes displayed in pixels (e.g. the image dimensions) are related to the actual image and are not a virtual size. On the other hand, changing the value might confuse the user.

@abalaji D7581 has now landed in master, so please do feel free to continue working on this. :) This patch no longer applies cleanly so it may need a bit of cleanup work.

Alternatively would you like to hand it off to @volkov?

Another approach: D20267

@ngraham @volkov sure I don't mind, I already have too many things open haha, and I haven't been delivering :/. Life's been too hard on me lately and I can't catch a break. I expect to be freed up during the summer, so hopefully I can come back and finish some of the other work