Skip to content

fix: Change thumbnail-function to resize-function #6815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ghost
Copy link

@ghost ghost commented Apr 22, 2025

to reduce visual artifacts in resized images.


The thumbnail-function was designed to be fast instead of accurate:

For downscaling, this method uses a fast integer algorithm where each source pixel contributes to exactly one target pixel. May give aliasing artifacts if new size is close to old size.

For speed reasons, all interpolation is performed linearly over the colour values. It will not take the pixel colour spaces into account.

For resizing images, the resize-function is more appropriate.

I chose the Triangle-interpolation (linear interpolation) because it does not add more visual artifacts.
Gaussian seems to blur the image slightly, Lanczos3 and CatmullRom can add halos near edges.

Improves: deltachat/deltachat-desktop#5018 (comment)

Verified

This commit was signed with the committer’s verified signature.
to reduce visual artifacts in resized images.
@r10s
Copy link
Contributor

r10s commented Apr 22, 2025

thanks a lot! this is a very welcome and on point PR! we're getting somewhere and have options now :)

what needs to be considered is that the chosen algorithm is 10 times slower, according to https://docs.rs/image/latest/image/imageops/enum.FilterType.html a typical photo of nowadays cams takes 414 ms instead of 31 ms - with an impressive improvement in quality on that page

otoh, not sure, what thumbnail is really using - i tried the same image, it does by far not look that shitty as there:

Screenshot 2025-04-22 at 23 22 29

what's pretty nice is that we get the improvement in quality probably by not really larger image sizes.

this is welcome esp. for avatars, where we just these days are improving things, #6611 - i can imaging better interpolation will result in another boost

The thumbnail-function was designed to be fast instead of accurate:

as we're supporting also very old phones (which, however, usually have less MP), i am wondering, if thumbnail was chosen on purpose - @iequidoo @link2xt @Hocuri - do you remember?

@r10s r10s requested a review from iequidoo April 22, 2025 21:44
@ghost
Copy link
Author

ghost commented Apr 22, 2025

The 31ms are for Nearest Neighbor, which is faster because it does not interpolate much; it is not useful for downscaling photos. Linear interpolation is a very basic type of image-interpolation for resizing. According to the documentation, FilterType::Triangle is linear interpolation.

The file-sizes can actually be smaller with the Triangle-interpolation, because the visual artifacts that the thumbnail-function does generate, can be less compressable.

@iequidoo
Copy link
Collaborator

iequidoo commented Apr 23, 2025

Just to clarify, thumbnail() doesn't use FilterType::Nearest, it uses some fast integer algorithm where each source pixel contributes to exactly one target pixel for downscaling, and it's not reported in the documentation how fast it is as compared to Triangle, so we need to measure on our own. But obviously thumbnail() isn't good because it's not a fair algo in relation to pixels in the source image by its definition, so visual effects are expected. Probably we indeed need to switch to Triangle as it's the fastest fair algorithm (IIUC it's bilinear interpolation, but i didn't look into image code).

EDIT: Image scaling using thumbnail() was introduced in 1910093 by @r10s, before it was used only for scaling avatars. Anyway, it's not that important, we should apparently rethink this.

EDIT: For avatars it was introduced in a5f949c.

EDIT: I tried this on my Desktop and Triangle is ~2.82 times slower than thumbnail() on some random 8.5 Mpx image. Visually it's better, but more blurry, so i'm not sure that we should switch to it. You can use this code for experiments:

                    let now = std::time::Instant::now();
                    let new_img = if crate::tools::time() % 60 < 30 {
                        info!(context, "thumbnail()...");
                        img.thumbnail(img_wh, img_wh)
                    } else {
                        info!(context, "resize()...");
                        img.resize(img_wh, img_wh, image::imageops::FilterType::Triangle)
                    };
                    info!(context, "time: {}", (std::time::Instant::now() - now).as_nanos());

@r10s
Copy link
Contributor

r10s commented Apr 23, 2025

thanks a lot for explanations, @iequidoo - and for the snippet i tried on some real devices, see here for details. the summary:

i can confirm that things are about 4 times slower.

on many devices, the additional slowness is significant - on a modern iPhone it is 1 second instead of 0.3 seconds, on an older android7, it is 5 seconds instead of 1.5 seconds - both using the camera image as is.

note, that the given android device is by far not the oldest one we're supporting and caring for and that even newer budget phones are quite slow. we're often praised for good support of old hardware, this PR would make things worse for few benefit

the slowness is esp. worse as the image appears in the chat only after it is recoded, so it is also bad UX as for seconds the image is lost (sure, that part can be improved, but that is another effort). using this PR we would introduce waiting times for lots of devices that have instant photo sending up to now

so, really not sure, if the slight improvements in quality are worth the slowness.

we are also usually hesitant to user options, unless there is some large impact and need, i do not see this in this case as well. also, the differnence is too small and it will cost resources in support, follow up, whatnot. but, we can consider using conditional compiling so that desktop is using resize - while android stays with thumbnail. or can we even estimate the speed of the processor somehow?

all in all, -1 for changing unconditonally - thumbnail seems to be a better compromise of speed vs quality

@iequidoo
Copy link
Collaborator

we are also usually hesitant to user options, unless there is some large impact and need, i do not see this in this case as well. but, we can consider using conditional compiling so that desktop is using resize - while android stays with thumbnail.

We can add a config option, w/o UI for it, at least on Desktop it's easy to set via environment, then advanced users can experiment. Triangle gives a bit more blurry images and their weight is smaller (by 11% in my experiment), as a compensation we can slightly increase linear image size, but only for usual images as currently avatars are limited to 512 px and i'm unsure it's good to break this limit. Not sure all this is worth adding more code.

@ghost
Copy link
Author

ghost commented Apr 23, 2025

can we even estimate the speed of the processor somehow?

I suppose one could encode a (generated) test-image as a performance-test (in the background) when Delta Chat is started and it has not been performed before, and if it was rather slow, ask if the setting should be changed.

Some indicator for showing that an image is being processed for sending, could be good, yes.

The quality-difference between thumbnail and triangle can be very noticeable, depending on the image.
Illustrations based on line-art, in a resolution close to the target-resolution, can have much more aliasing after resizing.
For example: For an illustration resized from 800 x 1600 to 640 x 1280 (by Delta Chat), the aliasing was much more noticeable than the same one from 2508 x 5016.

Comparisons

NoHalo:
Umbrella and flower - nohalo
triangle:
Umbrella and flower - triangle
thumbnail (from 800x1600 resolution - the straight lines, of the umbrella, are waves instead):
Umbrella and flower - 1600 thumbnail
thumbnail (from 2508x5016 resolution):
Umbrella and flower - 5016 thumbnail

Link to source of the image above.

NoHalo:
purple hair - NoHalo
triangle:
purple hair - DC triangle
thumbnail:
purple hair - DC thumbnail

@iequidoo
Copy link
Collaborator

I suppose one could encode a (generated) test-image as a performance-test (in the background) when Delta Chat is started and it has not been performed before, and if it was rather slow, ask if the setting should be changed.

I don't think there may be user-friendly wizard-like solutions, moreover they need some UI code, rather there should be a good default with a capability to change encoding. Alternatively, we can start with thumbnail() and if encoding takes very little time, switch to Triangle. If it suddenly takes too much time, switch back (users may upgrade/downgrade machines, image patterns may change etc., so we shouldn't stick with encoding method). Advanced users can fixate the setting in the env.

The good thing is that with Triangle we can increase images from 1280 to 1360 px because it compresses better, that would be a real quality improvement.

@r10s
Copy link
Contributor

r10s commented Apr 24, 2025

okay, thanks all for figuring out some facts! to sum up:

  1. it is clear, that Triangle creates better results more often than not

  2. sometimes, the improvement is not regarded as such (this is also the reason why there are many different algorithms in gimp etc). often the improvement it is minor

  3. the improvement comes at high costs on speed

i had some internal discussion with other devs meanwhile. weighted all points, it is clear that that this PR is not a good tradeoff. for more or less complex ideas mentioned above - we currently do not want that to consume more time and resources; it prevents us from doing more impactful things.

we might come back to some of the ideas at some point - and also, the PR was not wasted - if point 3. would not be there, it could have been an easy change :)

it is known and accepted that the scaled down image is lossy and not perfect. we buy speed with that.

if one really needs to transfer an image as close as possible, one should attach it as "File".

@MoonlightWave-12 i feel a bit bad to be - again - the guy with the bad news, saying what we cannot do atm. still, i need to have the full picture in mind. in general, it is more helpful to contribute on things that actually need to be done, picking issues from the issue tracker eg.

@r10s r10s closed this Apr 24, 2025
r10s added a commit that referenced this pull request Apr 24, 2025
this PR scaled avatars using the Triangle-filter,
resulting in often better image quality and smaller files (5%).

it comes at high costs,
therefore, we do not do that unconditionally for each image sent,
see comment in the code
and #6815
@r10s
Copy link
Contributor

r10s commented Apr 24, 2025

i created a PR that picks up this discussion to improve avatars, #6822

@ghost ghost deleted the change-thumbnail-function-to-resize-function branch April 24, 2025 17:29
@ghost
Copy link
Author

ghost commented Apr 24, 2025

I see. Well, at least the avatars will look better. :)

r10s added a commit that referenced this pull request Apr 24, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
this PR scaled avatars using the Triangle-filter,
resulting in often better image quality and smaller files (5%).

it comes at high costs,
therefore, we do not do that unconditionally for each image sent, see
comment in the code
and #6815

---------

Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants