feat: Don't scale down huge non-JPEGs, recode them first (#7977)

src/blob.rs

@@ -406,21 +406,30 @@ impl<'a> BlobObject<'a> {
             // TODO: Fix lost animation and transparency when recoding using the `image` crate. And
             // also `Viewtype::Gif` (maybe renamed to `Animation`) should be used for animated
             // images.
-            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,
-                            )?
-                        });
+            let do_scale =
+                // For avatars the limits are strict.
+                is_avatar && exceeds_wh
+                // Don't recode huge JPEGs w/o resizing:
+                // - It may be huge because of high JPEG quality, but we don't know that.
+                // - We don't want extra CPU work for most photos.
+                || exceeds_max_bytes && fmt == ImageFormat::Jpeg
+                // Otherwise check if we want to try encoding w/o resizing and perform it.
+                || (exceeds_max_bytes || 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,
+                    )?
+                }; // If the encoded image is unluckily huge, we might want to save CPU for
+            // non-avatar JPEGs: `&& (is_avatar || fmt != ImageFormat::Jpeg)`. However, if the
+            // original JPEG quality was much lower, the image may be too huge now. Should be a rare
+            // case, so keep it simple and downscale the image. Have done one encoding iteration,
+            // can do one more, the CPU won't die.
 
             if do_scale {
                 let longest_side_len = max(img.width(), img.height());
@@ -497,8 +506,7 @@ impl<'a> BlobObject<'a> {
                     }
                 }
             }
-
-            if do_scale || exif.is_some() {
+            if !encoded.is_empty() || exif.is_some() {
                 // The file format is JPEG/PNG now, we may have to change the file extension
                 if !matches!(fmt, ImageFormat::Jpeg)
                     && matches!(ofmt, ImageOutputFormat::Jpeg { .. })

src/blob/blob_tests.rs

@@ -4,7 +4,9 @@ use super::*;
 use crate::message::{Message, Viewtype};
 use crate::param::Param;
 use crate::sql;
-use crate::test_utils::{self, AVATAR_64x64_BYTES, AVATAR_64x64_DEDUPLICATED, TestContext};
+use crate::test_utils::{
+    self, AVATAR_64x64_BYTES, AVATAR_64x64_DEDUPLICATED, TestContext, TestContextManager,
+};
 use crate::tools::SystemTime;
 
 fn check_image_size(path: impl AsRef<Path>, width: u32, height: u32) -> image::DynamicImage {
@@ -151,13 +153,43 @@ async fn test_add_white_bg() {
                 .unwrap()
                 .decode()
                 .unwrap();
-            assert!(img.width() == img_wh);
-            assert!(img.height() == img_wh);
+            assert_eq!(img.width(), img_wh);
+            assert_eq!(img.height(), img_wh);
             assert_eq!(img.get_pixel(0, 0), Rgba(color));
         });
     }
 }
 
+/// A non-JPEG image mayn't be huge, but after removing Exif it may become huge if it can't be
+/// encoded to JPEG effectively and must be downscaled.
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_downscale_after_removing_exif() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let t = &tcm.unconfigured().await;
+    let bytes = include_bytes!("../../test-data/image/avatar900x900-q35.jpg").as_slice();
+
+    let avatar_src = t.dir.path().join("avatar.png");
+    fs::write(&avatar_src, bytes).await?;
+
+    let mut blob = BlobObject::create_and_deduplicate(t, &avatar_src, &avatar_src)?;
+    // TODO: Trying 1280 also makes sense, but currently such a test fails: the image still has 900
+    // px and is huge. It should be downscaled.
+    let img_wh = 640;
+    let viewtype = &mut Viewtype::Image;
+    let strict_limits = false;
+    blob.check_or_recode_to_size(t, None, viewtype, img_wh, 27_000, strict_limits)?;
+    tokio::task::block_in_place(move || {
+        let (_, exif) = image_metadata(&std::fs::File::open(blob.to_abs_path())?)?;
+        assert!(exif.is_none());
+        let img = ImageReader::open(blob.to_abs_path())?
+            .with_guessed_format()?
+            .decode()?;
+        assert_eq!(img.width(), img_wh);
+        assert_eq!(img.height(), img_wh);
+        Ok(())
+    })
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_selfavatar_outside_blobdir() {
     async fn file_size(path_buf: &Path) -> u64 {
@@ -462,6 +494,26 @@ async fn test_recode_image_balanced_png() {
     .unwrap();
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_recode_image_balanced_png_huge() {
+    let bytes = include_bytes!("../../test-data/image/screenshot-huge.png");
+
+    SendImageCheckMediaquality {
+        viewtype: Viewtype::Image,
+        media_quality_config: "0",
+        bytes,
+        extension: "jpg",
+        original_width: 1618,
+        original_height: 949,
+        compressed_width: 1618,
+        compressed_height: 949,
+        ..Default::default()
+    }
+    .test()
+    .await
+    .unwrap();
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_sticker_with_exif() {
     let bytes = include_bytes!("../../test-data/image/logo-exif.png");

src/tests/pre_messages/additional_text.rs

@@ -34,7 +34,8 @@ async fn test_additional_text_on_different_viewtypes() -> Result<()> {
     let (pre_message, _, _) = send_large_image_message(alice, a_group_id).await?;
     let msg = bob.recv_msg(&pre_message).await;
     assert_eq!(msg.text, "test".to_owned());
-    assert_eq!(msg.get_text(), "test [Image – 228.45 KiB]".to_owned());
+    assert!(msg.get_text().starts_with("test [Image – "));
+    assert!(msg.get_text().ends_with(" KiB]"));
 
     Ok(())
 }

src/tests/pre_messages/receiving.rs

@@ -6,6 +6,7 @@ use crate::EventType;
 use crate::chat;
 use crate::chat::send_msg;
 use crate::config::Config;
+use crate::constants;
 use crate::contact;
 use crate::download::{DownloadState, PRE_MSG_ATTACHMENT_SIZE_THRESHOLD, PostMsgMetadata};
 use crate::message::{Message, MessageState, Viewtype, delete_msgs, markseen_msgs};
@@ -456,10 +457,11 @@ async fn test_receive_pre_message_image() -> Result<()> {
 
     // test that metadata is correctly returned by methods
     assert_eq!(msg.get_post_message_viewtype(), Some(Viewtype::Image));
-    // recoded image dimensions
-    assert_eq!(msg.get_filebytes(bob).await?, Some(233935));
-    assert_eq!(msg.get_height(), 1704);
-    assert_eq!(msg.get_width(), 959);
+    let n_bytes: usize = msg.get_filebytes(bob).await?.unwrap().try_into().unwrap();
+    assert!(100_000 < n_bytes);
+    assert!(n_bytes <= constants::BALANCED_IMAGE_BYTES);
+    assert_eq!(msg.get_height(), 1920);
+    assert_eq!(msg.get_width(), 1080);
 
     Ok(())
 }