fix: Don't upscale images and test that image resolution isn't changed unnecessarily

[Hocuri]

This adds a test for #7760.

Also, it fixes another bug which I uncovered with the test: If the resolution was already lower than the max resolution, then the image was upscaled to match the max resolution.

cc @72374

[iequidoo]

I'd also remove this line from here and add

if !encoded.is_empty() {
    target_wh = target_wh * 2 / 3;
}

to the beginning of the loop so that no extra iteration for avatars is done. It's a small optimization, but may make the code clearer, otherwise one may wonder why we recode avatars to the same size again.

[Hocuri]

Not sure, this may easily lead to another issue where we unnecessarily reduce the target resolution.

I anyways find the code block above hard to understand:

            let do_scale = exceeds_max_bytes
                || is_avatar
                    && (exceeds_wh
                        || exif.is_some() && {
                            if mem::take(&mut add_white_bg) {
                                self::add_white_bg(&mut img);
                            }
                            encoded_img_exceeds_bytes(
                                context,
                                &img,
                                ofmt.clone(),
                                max_bytes,
                                &mut encoded,
                            )?
                        });

so... if the avatar does not exceed the max_bytes or width/height, but it has some exif, then we directly do an iteration of adding white background and recoding, and check the resulting bytes size. If the image is too big now (note that it wasn't too big before), then do_scale is true and we reduce the resolution.

So, I guess that the code block is there to make really sure that the avatar file can't be too big in the end

Which is good, of course, it's just hard to understand, and I am wary of adding code that depends on is behavior.

[72374]

Shouldn't the || exif.is_some() && { there be || exif.is_some() || {, so that it is checked if any of the three is true?
The way i understand it, avatar-images that are not larger than max_bytes and max_wh, but include exif-data, would only be re-encoded, if those also do not fit within the size after being encoded with a white background.
Usually the purpose of specifically re-encoding images with exif-data is, to remove metadata (though it can also be removed from the image-file without re-encoding the image).

[iequidoo]

[...] || exif.is_some() || { [...]

If it's an avatar w/o Exif, it should only be recoded if it exceeds any limits. White background is also not needed when not recoding an avatar, this is a w/a for transparent avatars recoded to JPEG, otherwise the image crate adds a black one.

[72374]

That would still be the same after the change. I opened a PR with a short explanation for the change there: #7772

[iequidoo]

Which is good, of course, it's just hard to understand, and I am wary of adding code that depends on is behavior.

Finally i think that it's fine to move the target_wh reduction above, if it's under !encoded.is_empty(), nothing will depend on the complex logic above: it is plainly "if we've already tried to recode and still inside the loop, we need to try a smaller size". Anyway, this is minor

[72374]

Seems correct.
A comment could make it more understandable why that is done; then it would be fine to have this optimisation, i think.

Something like this, before the beginning of the loop, as it only needs to be checked once:

if do_scale {
    if is_avatar && !encoded.is_empty() {
        // If this happens, the image was already encoded at the original resolution,
        // for the file-size-check for an avatar with an added white background,
        // and did not fit within the file-size-limit after that.
        // Skip encoding it again at the original resolution.
        target_wh = target_wh * 2 / 3;
    }
    loop { …

Which is similar to what you suggested in the previous PR, i think.

---

Actually, not having this can result in quality-reduction, because, when the file-size-check happens, it does not only check the file-size, it also changes the image that will be used, which, depending on the encoder, can result in encoding the image again with jpeg-quality 75 (or not, if the encoder does detect that it is the same format and quality, and skips re-encoding).

[72374]

Suggested change

	// img_wh_max is the maximum image width and height, i.e. the resolution-limit.
	// max_wh is the maximum image width and height, i.e. the resolution-limit.

[72374]

This seems to be an outdated explanation.

Suggested change

	/// If `!is_avatar`, then if `max_bytes` is exceeded, reduces the image to `max_wh` and proceeds
	/// with the result (even if `max_bytes` is still exceeded).
	///
	/// If `is_avatar`, the resolution will be reduced in a loop until the image fits `max_bytes`.
	/// If `max_bytes` is exceeded,
	/// the image will be re-encoded to `max_wh` or the original resolution, whichever is smaller.
	/// If it then is still larger than `max_bytes` it will be reduced in resolution,
	/// until it fits within `max_bytes`.
	///
	/// If `is_avatar`, then images with a higher resolution than `max_wh`,
	/// and images which include exif-data, will also be re-encoded.