Include only the largest size for each wallpaper
ClosedPublic

Authored by ngraham on Jan 6 2019, 5:09 AM.

Details

Summary

In this patch, we remove all the versions of each wallpaper that are not the largest size. Here are the reasons why:

  • It's no longer necessary: The old method appears to have been an artifact of the days before wallpapers could differ from the aspect ratio of the screen without looking bad. This is no longer a technical constraint that we face.
  • Simplicity: It's easier to manage wallpapers, add new ones, and edit existing ones if you only have to do it for one image, not 15 or more.
  • Size savings: installed size on disk falls from 152 MB to 25 MB.

There is no increase in memory use since Plasma automatically trims the image to the screen size before caching it in memory.

Test Plan

All wallpapers are still installed and all still work and look good with the new "Scaled And Cropped" default setting.

Installed size is significantly smaller:
Before:

$ du -ch /usr/share/wallpapers/ | grep total
152M    total

After:

$ du -ch /usr/share/wallpapers/ | grep total
25M     total

Diff Detail

Repository
R131 Plasma Wallpapers
Branch
keep-only-largest-size (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6690
Build 6708: arc lint + arc unit
ngraham created this revision.Jan 6 2019, 5:09 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 6 2019, 5:09 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Jan 6 2019, 5:09 AM
ndavis added a subscriber: ndavis.EditedJan 6 2019, 5:24 AM

What happens if my wallpaper is set to the 1080p version of a wallpaper and then that version is removed with only the highest resolution version left?

This comment was removed by ngraham.

Just tested that out and the results aren't great: you get a black background instead of the wallpaper until you go to the wallpaper chooser and re-choose it. :/

However, just removing files from the repo doesn't actually remove them from users' machines; their packaging system does that. Perhaps we should alert packagers that for the 5.15 release, they shouldn't remove these files from users' machines when they disappear from the repo.

ndavis added a comment.Jan 6 2019, 5:43 AM

However, just removing files from the repo doesn't actually remove them from users' machines; their packaging system does that. Perhaps we should alert packagers that for the 5.15 release, they shouldn't remove these files from users' machines when they disappear from the repo.

Sounds reasonable.

ndavis added a comment.Jan 6 2019, 5:44 AM

Should I accept now or should I wait until that gets sorted out?

Feel free to accept, but I really want some input from a Plasma person on this whole idea before it lands. They have been silent on the matter so far but now that there are patches, I think it's important that we get the full story before going ahead.

ndavis accepted this revision.Jan 6 2019, 6:00 AM
This revision is now accepted and ready to land.Jan 6 2019, 6:00 AM
cfeck added a subscriber: cfeck.Jan 6 2019, 3:28 PM

Maybe symlinking all to the largest version?

What previous technical constraint are you referring to?

And we've had the multiple resolutions since 4.x so it dates back a long way.

It's about a few things:

  • CPU usage to load an image is a sizable amount of our loading. Especially for JPEG images.

You might see that less in 5.x as it now happens in another thread, and we don't necessarily max out all the cores - but still.

  • Artists wanted some control over /how/ it's cropped. i.e if you have a mountain on the right and lots of sea to the left, you want to always cut off the boring sea part (i.e the spray folder)
  • Our run-time resizes will look worse than gimp/inkscape doing it.
filipf added a subscriber: filipf.EditedJan 6 2019, 11:02 PM

What previous technical constraint are you referring to?

See D18006#387372. In short, images were blurry when scaled and cropped.

Our run-time resizes will look worse than gimp/inkscape doing it.

Aside from Plasma doing non-artist style cropping, they seem fine to me tbh.

CPU usage to load an image is a sizable amount of our loading. Especially for JPEG images.
You might see that less in 5.x as it now happens in another thread, and we don't necessarily max out all the cores - but still.

In your estimate, how bad would the added load be?

Artists wanted some control over /how/ it's cropped. i.e if you have a mountain on the right and lots of sea to the left, you want to always cut off the boring sea part (i.e the spray folder)

+1 but we need to do some sort of a trade off here. IMO I'd keep all of sizes for "Next", but for T9817 and T10220 we're put in a pretty bad situation if having to ship all sizes because we'll be want to shipping large resolution wallpapers and quite a few of them.

For instance, just shipping a certain old Plasma wallpaper adds 20 MiB to the plasma-wallpapers package, which is already almost half of its current size (a bit over 40 MiB).

For comparison's sake Deepin, GNOME and Elementary also just offer one size in this extra package from what I can tell.

Also might be worth mentioning that the new photographic wallpapers in T10220 are CC0 or similar licence (Unsplash) and picked from third-party sites so there's no author involvement with KDE. We're also trying to select them based on whether or not they will get messed up with cropping.

In short, images were blurry when scaled and cropped.

I want the "In long" version. QImage resizing hasn't changed, and with the source size set that's what we'll be using.

In short, images were blurry when scaled and cropped.

I want the "In long" version. QImage resizing hasn't changed, and with the source size set that's what we'll be using.

What I can say is that this Kubuntu experience wasn't a one off, I had to crop my wallpapers manually for the longest because Plasma would make them blurry if they didn't match the screen resolution. However, it's been working lately.

I know this isn't the greatest info, could you help me investigate what actually changed in the meantime? I can test whatever's relevant.

I can list bugs that you might have seen in 5.x.

I can't name things in 4.x. which is when it was introduced. Its before my plasma time.

@filipf That looks about right!

@davidedmundson Reading through that bug report, it looks like we do some magic to avoid storing huge pixmaps in memory, so is it true that this change would avoid increasing users' memory consumption? I was thinking that on future memory constrained mobile devices especially, this might otherwise cause a deal-breaking regression and I don't want to be responsible for that. :)

Also what exactly do you mean by "run-time resizes"? Where would these happen?

@filipf That looks about right!

It's not. That's only within 5.x.

This code has been around since at least ede349f8f2db9930c96ab4e56f9440ba82c24cb7 on kde-workspace which was before KDE 4.2

Reading through that bug report, it looks like we do some magic to avoid storing huge pixmaps in memory

http://doc.qt.io/qt-5/qml-qtquick-image.html#sourceSize-prop

Effectively it's a resize before the texture upload, rather than being done in the shader every frame.
Only the resized version will be in memory, but it will be the uncropped version which is why you sometimes still see a tiny increase.

I've booted up Debian 8, KDE SC version 4.14.2. The wallpapers scaled nicely there so I understand your curiosity.

Taking a look at this list (https://packages.debian.org/de/jessie/all/kde-wallpapers/filelist) I notice three things:

  1. some of the wallpapers are not manually cropped at all -> you don't see it here but the default Oxygen wallpaper Elarun also came only in one size (2560x1600), at least in Debian 8
  2. if the wallpapers are cropped, they're not cropped for all sizes
  3. the cropped sizes differ between wallpapers

This leads me to speculate that cropping was done mainly due to artistic reasons. The wallpaper chooser explicitly lists resolutions so this may hint at some problems before 4.14, but that's before my time as well.

rikmills added a comment.EditedJan 7 2019, 1:42 PM

However, just removing files from the repo doesn't actually remove them from users' machines; their packaging system does that. Perhaps we should alert packagers that for the 5.15 release, they shouldn't remove these files from users' machines when they disappear from the repo.

Leaving orphaned files on disk isn't really acceptable.

Instead Plasma should perhaps recognise that the image has gone, and seek and apply whatever single larger size now exists.

Perhaps we should alert packagers that for the 5.15 release, they shouldn't remove these files from users' machines when they disappear from the repo.

That's not do-able.

Why does it result in a black screen? If we are storing the image by path rather than package ID it kinda makes the whole system a joke.

filipf added a comment.EditedJan 8 2019, 2:16 AM
  • Our run-time resizes will look worse than gimp/inkscape doing it.

Letting Plasma scale this 3972x2979 wallpaper.

This is Plasma using the same wallpaper cropped in GIMP to 1920x1080 monitor resolution.

There is almost zero difference.


EDIT: I do see some slight difference in sharpness though, check in full-screen Gwenview flipping between the 2 pictures. This wallpaper is a worst case scenario test due to the details:

But it's neglible IRL (when not comparing), I guarantee there will be no one telling us the wallpapers are blurry.

ngraham edited the summary of this revision. (Show Details)Jan 8 2019, 8:20 PM

Instead Plasma should perhaps recognise that the image has gone, and seek and apply whatever single larger size now exists.

Why does it result in a black screen? If we are storing the image by path rather than package ID it kinda makes the whole system a joke.

Which would be sad, yeah. We'll need to fix this. Any idea where I should look?

FWIW I agree that the image editor scaling vs Plasma scaling is a non-issue. Even in the worst-case scenario, the only distinguishable differences are when you deliberately do a flip-between-them comparison. For most, the differences are negligible and non-regressive. No one is going to notice or care.

filipf added a subscriber: rooty.Jan 8 2019, 9:32 PM

It's a total non-issue. @rooty and I have been shifting through loads of wallpapers in the past month or so and it hasn't even occurred to us that this (image editor vs. Plasma cropping) might even be a thing. The difference is minor and not noticeable.

We also never noticed any performance issues, although people with greater expertise should expand on that.

As for packaging, my only concern (applies to D18078 as well) is if someone was using an old wallpaper (whichever the resolution was). This 1 image should ideally be retained somehow.

ngraham added a subscriber: mart.Jan 8 2019, 9:38 PM

So it sounds like our only technical blocker is that we don't want people who are using a wallpaper whose small sizes disappear on disk to get an ugly black background. @davidedmundson or @mart, any ideas on this front?

GB_2 added a subscriber: GB_2.Jan 8 2019, 9:53 PM

@cfeck had a good idea:

Maybe symlinking all to the largest version?

filipf added a comment.Jan 8 2019, 9:57 PM

Could Plasma always copy the used wallpaper to some location and link to that? This is actually an annoyance irrespective of this discussion, if you move your wallpaper on the disk you lose the wallpaper.

any ideas on this front?

[Containments][11][Wallpaper][org.kde.image][General]
Image=file:///home/opt/kde5/share/wallpapers/FallenLeaf/contents/images/1280x1024.jpg

Apparently we do store by full path which is quite a good argument for us stopping doing this thing.

Does @cfeck's idea to turn the smaller sizes into symlinks make sense as a transitional step to preserve the user experience for people who are current users of these wallpapers?

Could Plasma always copy the used wallpaper to some location and link to that?

"Could it" yes.

"Does it" no.

If we're going to save it, we could save the post-scaled version which would nullify the increased loading cost argument...and it would be a prerequisite for having a single wallpaper across screens which is also requested.

Getting a save cache into 5.15 would be tight but doable. Which would then allow this for 5.16

If we're going to save it, we could save the post-scaled version which would nullify the increased loading cost argument...and it would be a prerequisite for having a single wallpaper across screens which is also requested.

Getting a save cache into 5.15 would be tight but doable. Which would then allow this for 5.16

This sounds like a great feature, but would it be necessary if we make symlinks in the 5.15 timeframe?

This is no longer a technical constraint that we face.

I don't think this was ever a thing.


Ok, lets do it with the symlink thing.

ngraham updated this revision to Diff 49283.Jan 11 2019, 7:15 PM

Add compatibility symlinks for images shipped in prior releases

ngraham updated this revision to Diff 49285.Jan 11 2019, 7:35 PM

Really add symlinks

filipf added a comment.EditedJan 11 2019, 8:11 PM

For D18078 it would be best to wait until a save cache is implemented, right?

For D18078 it would be best to wait until a save cache is implemented, right?

I believe so. Once we have that, we can remove these compatibility symlinks too.

This revision was automatically updated to reflect the committed changes.