diff --git a/src/blob.rs b/src/blob.rs index a60ebe65c7..8489cef64c 100644 --- a/src/blob.rs +++ b/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 { .. }) diff --git a/src/blob/blob_tests.rs b/src/blob/blob_tests.rs index 0b765d72a9..cb4c722bee 100644 --- a/src/blob/blob_tests.rs +++ b/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, 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"); diff --git a/src/tests/pre_messages/additional_text.rs b/src/tests/pre_messages/additional_text.rs index f37baa4e4e..d27291fd3f 100644 --- a/src/tests/pre_messages/additional_text.rs +++ b/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(()) } diff --git a/src/tests/pre_messages/receiving.rs b/src/tests/pre_messages/receiving.rs index 7a2586e2cb..1bb44c579c 100644 --- a/src/tests/pre_messages/receiving.rs +++ b/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(()) } diff --git a/test-data/image/avatar900x900-q35.jpg b/test-data/image/avatar900x900-q35.jpg new file mode 100644 index 0000000000..afb118ce99 Binary files /dev/null and b/test-data/image/avatar900x900-q35.jpg differ diff --git a/test-data/image/screenshot-huge.png b/test-data/image/screenshot-huge.png new file mode 100644 index 0000000000..4b83ad994e Binary files /dev/null and b/test-data/image/screenshot-huge.png differ