fix: Do not additionally reduce the resolution of images that fit into the resolution-limit and are larger than the file-size-limit

[72374]

See: https://support.delta.chat/t/high-quality-avatar/1052/8

This removes an additional resolution-reduction for images that fit within the resolution-limit.

The additional reduction of the image-resolution was added in b7864f2.
It seems that back then only resolution-reduction was used to make the file-size of images smaller, and the purpose was to skip an unnecessary encoding-step.

However, encoding images with jpeg-quality set to 75, as it is currently done, can significantly reduce the file-size, even if the resolution is the same. As the resolution of the image that will be encoded is rather low when it fits within the resolution-limit, further reducing it can significantly reduce the quality of the image.
Thus it is better to not skip the encoding-step at the original resolution.

[Hocuri]

Wow, thanks for figuring this out! Code-wise, this definitely makes sense. Unfortunately, at least when I just tested it on my phone, the avatar was still blurred unnecessarily, maybe there is another bug.

Or, do you have some example image where this PR helps? Would also be nice to be able to add some test for this.

BTW, don't worry about the failing CI - this is just because this PR is from an outside repository, not because there is something wrong with the PR.

[72374]

One can see the issue by sending the following images in a chat while media-quality is set to balanced.

Original test-images - larger than file-size-limit:

Details

Fitting within the resolution-limit - before sending:

Larger than the resolution-limit - before sending:

Resulting test-images - fitting within the file-size-limit:

Details

Fitting within the resolution-limit - after sending:

Larger than the resolution-limit - after sending:

The effect should be similar for avatar-images, though i did not try that. The original images for testing that, would have to be larger than 60kB, fit within 512x512 pixels, and be smaller than 60KB after being encoded to JPG with quality set to 75 at the original resolution, if i remember correctly.

[r10s]

cc @iequidoo who did quite some changes in that area iirc

/me did as well, but currently busy with other things, so i did not follow

[iequidoo]

The right fix should be removing matches!(fmt, ImageFormat::Jpeg) || from here. If encoded is nonempty, we've already tried to recode the image and it still needs downscaling if we got to this place. The check for JPEG was added because usually images don't have "JPEG quality" much more than 75, but i agree that it's not always true, so removing it makes sense. However, note that for most images this will result in an extra encoding iteration, consuming CPU and time. Overall, if the user wants to send an image in a good quality, they should send it as "File", this way no recoding is done.

[72374]

The intended resolution-reduction is done in a loop below, as long as the image is too large in file-size.

https://github.com/chatmail/core/pull/7760/files#diff-d31a5503e9289e3de5355046908467f334cbb78c652768595a5f47b7eb195ea5R446-R460

However, note that for most images this will result in an extra encoding iteration, consuming CPU and time.

The image-rs-library copies data instead of resampling it, when using the resize-function and the resolution is identical to the original, so it should not require meaningful processing-time:
https://docs.rs/image/latest/src/image/imageops/sample.rs.html#984-989

The check for JPEG was added because usually images don't have "JPEG quality" much more than 75

Usually photos (and other images) tend to have a jpeg-quality around 90, from what i remember.
My old compact-digital-camera encodes images at jpeg-quality 92 or higher, the pre-installed camera-app (a fork of OpenCamera) on /e/OS is preset to jpeg-quality 90.

[iequidoo]

Ok, i didn't notice that it's done only for avatars below. Obviously the intention was to not reduce the width/height of non-avatar images below BALANCED_IMAGE_SIZE/WORSE_IMAGE_SIZE at all, but a check for is_avatar was forgotten here. So the right condition should be

if !encoded.is_empty() && is_avatar {

to be consistent with the check in the loop. But also we should break from the loop if the image is already recoded (!encoded.is_empty()) and doesn't need downscaling.

However, note that for most images this will result in an extra encoding iteration, consuming CPU and time.

This isn't important then, an extra iteration for avatars is fine, they aren't changed frequently.

[72374]

The code-block was originally a small performance-improvement, because it was not even attempted to encode with a low jpeg-quality, so that it would be likely that the encoded image would not be smaller after encoding at the original resolution. But now that encoding is done with a low jpeg-quality, this code-block unnecessarily lowers the resolution of images that would fit into the file-size-limit, when encoded at the original resolution with jpeg-quality set to 75.

This code-block is only used once, before the encoding-loop begins, it only happens for images with a resolution of 1280x1280 or less, and with a file-size above 500,000 bytes, and the only thing it does, is skip encoding at the original resolution with jpeg-quality set to 75, for only those images. Such original images are likely to have been encoded with a higher jpeg-quality than 75.

The limits are lower for avatar-images, but it is the same issue, caused by the same code, that happens when sending images in chats.

It does not require a long processing-time to resize an image with a resolution of 1280x1280 (the largest resolution that the code is used for, ~1.6 Megapixels) or lower, to the same or a lower resolution. Typical cameras store images with resolutions much higher than that, often around 3840×2160 (~5 times the resolution, ~8.3 Megapixels) or more.

[Hocuri]

While I can't recognize any difference in the images sent by @72374, I do see that this PR here makes a visible positive difference when sending this screenshot:

When setting avatars that are smaller than 512x512 but bigger than 60k, I couldn't manage to see any improvement with this PR. But it's an improvement already, so, let's merge it.

[Hocuri]

Thanks a lot @72374!

[72374]

Happy to help. :)

A quick (unverified) guess for why this does not change the quality of avatar-images set with the Android-app is, that the Android-app resizes images to be 640x640 pixels with jpeg-quality 100 (which increases the file-size), before making it available to Chatmail Core. So perhaps changing 640 to 512 or removing the override for the resolution there (and in the other places it is done) would improve the quality for avatar-images.

[iequidoo]

I still don't understand why the code block was just removed instead of fixing the condition as suggested in #7760 (comment). I didn't test the suggestion however, but with this change now images bigger than 500K, but smaller than 1280 px are upscaled to 1280 px (tested in Desktop). I don't think it's an expected behavior, anyway the code still looks unclear. When removing the code, it's good to add a test which covers it to see that the behavior is changed in an expected way.

Wrt upscaling, see https://docs.rs/image/latest/image/imageops/fn.thumbnail.html -- thumbnail() isn't accurate, it uses a "nearest pixel" interpolation:

In case the current width is smaller than the new width or similar for the height, another strategy is used instead. For each pixel in the output, a rectangular region of the input is determined, just as previously. But when no input pixel is part of this region, the nearest pixels are interpolated instead.

Anyway thanks to everyone, but let's add more tests here.

EDIT: #7769 fixes this.

[Hocuri]

@iequidoo then with #7760 everything is fine? (except for the mentioned problem in DC Android)

[Hocuri]

let's add more tests here

+1, more tests would definitely help

[iequidoo]

So perhaps changing 640 to 512 or removing the override for the resolution there (and in the other places it is done) would improve the quality for avatar-images.

Maybe * 2 / 3 is too hard then, * 4 / 5 may be better, but may increase the number of iterations, though this is unlikely, but each iteration adds resizing artifacts currently.