From ed13b9b27fbd9f8d63ab599dff19ffe9e44bafb1 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Wed, 7 Aug 2024 18:42:22 -0300 Subject: [PATCH 1/2] feat: Send avatars as large as usual images when possible - Remove limits on the avatar width and height, having a limit on its weight is sufficient. - Send avatars as large as usual images in protected and self- chats, Outlook servers can't see avatars there anyway. --- src/blob.rs | 104 +++++++++++++++--------------------- src/chat.rs | 4 +- src/config.rs | 18 ++++++- src/constants.rs | 4 -- src/sql.rs | 3 +- src/tests/verified_chats.rs | 32 +++++++++++ 6 files changed, 97 insertions(+), 68 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index 320cc011e9..6d1bbb139d 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -1,6 +1,6 @@ //! # Blob directory management. -use core::cmp::max; +use std::cmp::{max, min}; use std::ffi::OsStr; use std::fmt; use std::io::{Cursor, Seek}; @@ -14,13 +14,10 @@ use futures::StreamExt; use image::codecs::jpeg::JpegEncoder; use image::ImageReader; use image::{DynamicImage, GenericImage, GenericImageView, ImageFormat, Pixel, Rgba}; -use num_traits::FromPrimitive; use tokio::io::AsyncWriteExt; use tokio::{fs, io}; use tokio_stream::wrappers::ReadDirStream; -use crate::config::Config; -use crate::constants::{self, MediaQuality}; use crate::context::Context; use crate::events::EventType; use crate::log::LogExt; @@ -346,28 +343,33 @@ impl<'a> BlobObject<'a> { Ok(blob.as_name().to_string()) } - pub async fn recode_to_avatar_size(&mut self, context: &Context) -> Result<()> { + /// Recodes an avatar pointed by a [BlobObject] so that it fits into limits on the image width, + /// height and file size specified by the config. + /// + /// * `protected`: Whether the resulting avatar is going to be used in a protected context, + /// i.e. in a protected chat or stored locally, and therefore may be larger. + pub async fn recode_to_avatar_size( + &mut self, + context: &Context, + protected: bool, + ) -> Result<()> { let blob_abs = self.to_abs_path(); - - let img_wh = - match MediaQuality::from_i32(context.get_config_int(Config::MediaQuality).await?) - .unwrap_or_default() - { - MediaQuality::Balanced => constants::BALANCED_AVATAR_SIZE, - MediaQuality::Worse => constants::WORSE_AVATAR_SIZE, - }; - - let maybe_sticker = &mut false; - let strict_limits = true; + let (img_wh, max_bytes) = context.max_image_wh_and_size().await?; // max_bytes is 20_000 bytes: Outlook servers don't allow headers larger than 32k. // 32 / 4 * 3 = 24k if you account for base64 encoding. To be safe, we reduced this to 20k. + let max_bytes = match protected { + false => min(max_bytes, 20_000), + true => max_bytes, + }; + let maybe_sticker = &mut false; + let is_avatar = true; if let Some(new_name) = self.recode_to_size( context, blob_abs, maybe_sticker, img_wh, - 20_000, - strict_limits, + max_bytes, + is_avatar, )? { self.name = new_name; } @@ -387,32 +389,21 @@ impl<'a> BlobObject<'a> { maybe_sticker: &mut bool, ) -> Result<()> { let blob_abs = self.to_abs_path(); - let (img_wh, max_bytes) = - match MediaQuality::from_i32(context.get_config_int(Config::MediaQuality).await?) - .unwrap_or_default() - { - MediaQuality::Balanced => ( - constants::BALANCED_IMAGE_SIZE, - constants::BALANCED_IMAGE_BYTES, - ), - MediaQuality::Worse => (constants::WORSE_IMAGE_SIZE, constants::WORSE_IMAGE_BYTES), - }; - let strict_limits = false; + let (img_wh, max_bytes) = context.max_image_wh_and_size().await?; + let is_avatar = false; if let Some(new_name) = self.recode_to_size( context, blob_abs, maybe_sticker, img_wh, max_bytes, - strict_limits, + is_avatar, )? { self.name = new_name; } Ok(()) } - /// If `!strict_limits`, then if `max_bytes` is exceeded, reduce the image to `img_wh` and just - /// proceed with the result. fn recode_to_size( &mut self, context: &Context, @@ -420,10 +411,10 @@ impl<'a> BlobObject<'a> { maybe_sticker: &mut bool, mut img_wh: u32, max_bytes: usize, - strict_limits: bool, + is_avatar: bool, ) -> Result> { // Add white background only to avatars to spare the CPU. - let mut add_white_bg = img_wh <= constants::BALANCED_AVATAR_SIZE; + let mut add_white_bg = is_avatar; let mut no_exif = false; let no_exif_ref = &mut no_exif; let res = tokio::task::block_in_place(move || { @@ -493,7 +484,7 @@ impl<'a> BlobObject<'a> { // also `Viewtype::Gif` (maybe renamed to `Animation`) should be used for animated // images. let do_scale = exceeds_max_bytes - || strict_limits + || is_avatar && (exceeds_wh || exif.is_some() && { if mem::take(&mut add_white_bg) { @@ -530,7 +521,7 @@ impl<'a> BlobObject<'a> { ofmt.clone(), max_bytes, &mut encoded, - )? && strict_limits + )? && is_avatar { if img_wh < 20 { return Err(format_err!( @@ -579,7 +570,7 @@ impl<'a> BlobObject<'a> { match res { Ok(_) => res, Err(err) => { - if !strict_limits && no_exif { + if !is_avatar && no_exif { warn!( context, "Cannot recode image, using original data: {err:#}.", @@ -761,6 +752,8 @@ mod tests { use super::*; use crate::chat::{self, create_group_chat, ProtectionStatus}; + use crate::config::Config; + use crate::constants::{self, MediaQuality}; use crate::message::{Message, Viewtype}; use crate::test_utils::{self, TestContext}; @@ -984,14 +977,14 @@ mod tests { let mut blob = BlobObject::new_from_path(&t, &avatar_src).await.unwrap(); let img_wh = 128; let maybe_sticker = &mut false; - let strict_limits = true; + let is_avatar = true; blob.recode_to_size( &t, blob.to_abs_path(), maybe_sticker, img_wh, 20_000, - strict_limits, + is_avatar, ) .unwrap(); tokio::task::block_in_place(move || { @@ -1015,16 +1008,18 @@ mod tests { .await .unwrap(); assert!(avatar_blob.exists()); - assert!(fs::metadata(&avatar_blob).await.unwrap().len() < avatar_bytes.len() as u64); + { + let avatar_blob = &avatar_blob; + tokio::task::block_in_place(move || { + let (_, exif) = image_metadata(&std::fs::File::open(avatar_blob).unwrap()).unwrap(); + assert!(exif.is_none()); + }); + } let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap(); assert_eq!(avatar_cfg, avatar_blob.to_str().map(|s| s.to_string())); check_image_size(avatar_src, 1000, 1000); - check_image_size( - &avatar_blob, - constants::BALANCED_AVATAR_SIZE, - constants::BALANCED_AVATAR_SIZE, - ); + check_image_size(&avatar_blob, 1000, 1000); async fn file_size(path_buf: &Path) -> u64 { let file = File::open(path_buf).await.unwrap(); @@ -1033,16 +1028,9 @@ mod tests { let mut blob = BlobObject::new_from_path(&t, &avatar_blob).await.unwrap(); let maybe_sticker = &mut false; - let strict_limits = true; - blob.recode_to_size( - &t, - blob.to_abs_path(), - maybe_sticker, - 1000, - 3000, - strict_limits, - ) - .unwrap(); + let is_avatar = true; + blob.recode_to_size(&t, blob.to_abs_path(), maybe_sticker, 1000, 3000, is_avatar) + .unwrap(); assert!(file_size(&avatar_blob).await <= 3000); assert!(file_size(&avatar_blob).await > 2000); tokio::task::block_in_place(move || { @@ -1071,11 +1059,7 @@ mod tests { avatar_src.with_extension("png").to_str().unwrap() ); - check_image_size( - avatar_cfg, - constants::BALANCED_AVATAR_SIZE, - constants::BALANCED_AVATAR_SIZE, - ); + check_image_size(avatar_cfg, 900, 900); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/src/chat.rs b/src/chat.rs index 92f4902fb1..37dc642933 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -4106,7 +4106,9 @@ pub async fn set_chat_profile_image( msg.text = stock_str::msg_grp_img_deleted(context, ContactId::SELF).await; } else { let mut image_blob = BlobObject::new_from_path(context, Path::new(new_image)).await?; - image_blob.recode_to_avatar_size(context).await?; + image_blob + .recode_to_avatar_size(context, chat.is_protected() || chat.is_self_talk()) + .await?; chat.param.set(Param::ProfileImage, image_blob.as_name()); msg.param.set(Param::Arg, image_blob.as_name()); msg.text = stock_str::msg_grp_img_changed(context, ContactId::SELF).await; diff --git a/src/config.rs b/src/config.rs index 3806cc8026..e82a8f0c97 100644 --- a/src/config.rs +++ b/src/config.rs @@ -7,13 +7,14 @@ use std::str::FromStr; use anyhow::{ensure, Context as _, Result}; use base64::Engine as _; use deltachat_contact_tools::{addr_cmp, sanitize_single_line}; +use num_traits::FromPrimitive; use serde::{Deserialize, Serialize}; use strum::{EnumProperty, IntoEnumIterator}; use strum_macros::{AsRefStr, Display, EnumIter, EnumString}; use tokio::fs; use crate::blob::BlobObject; -use crate::constants; +use crate::constants::{self, MediaQuality}; use crate::context::Context; use crate::events::EventType; use crate::log::LogExt; @@ -537,6 +538,18 @@ impl Context { } } + pub(crate) async fn max_image_wh_and_size(&self) -> Result<(u32, usize)> { + match MediaQuality::from_i32(self.get_config_int(Config::MediaQuality).await?) + .unwrap_or_default() + { + MediaQuality::Balanced => Ok(( + constants::BALANCED_IMAGE_SIZE, + constants::BALANCED_IMAGE_BYTES, + )), + MediaQuality::Worse => Ok((constants::WORSE_IMAGE_SIZE, constants::WORSE_IMAGE_BYTES)), + } + } + /// Executes [`SyncData::Config`] item sent by other device. pub(crate) async fn sync_config(&self, key: &Config, value: &str) -> Result<()> { let config_value; @@ -621,7 +634,8 @@ impl Context { match value { Some(path) => { let mut blob = BlobObject::new_from_path(self, path.as_ref()).await?; - blob.recode_to_avatar_size(self).await?; + let protected = true; + blob.recode_to_avatar_size(self, protected).await?; self.sql .set_raw_config(key.as_ref(), Some(blob.as_name())) .await?; diff --git a/src/constants.rs b/src/constants.rs index 72cda9b159..062f89a873 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -198,10 +198,6 @@ pub(crate) const DC_FETCH_EXISTING_MSGS_COUNT: i64 = 100; pub const BALANCED_IMAGE_BYTES: usize = 500_000; pub const WORSE_IMAGE_BYTES: usize = 130_000; -// max. width/height of an avatar -pub(crate) const BALANCED_AVATAR_SIZE: u32 = 256; -pub(crate) const WORSE_AVATAR_SIZE: u32 = 128; - // max. width/height of images scaled down because of being too huge pub const BALANCED_IMAGE_SIZE: u32 = 1280; pub const WORSE_IMAGE_SIZE: u32 = 640; diff --git a/src/sql.rs b/src/sql.rs index c1f37b48ae..bdedc266ed 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -264,7 +264,8 @@ impl Sql { if recode_avatar { if let Some(avatar) = context.get_config(Config::Selfavatar).await? { let mut blob = BlobObject::new_from_path(context, avatar.as_ref()).await?; - match blob.recode_to_avatar_size(context).await { + let protected = true; + match blob.recode_to_avatar_size(context, protected).await { Ok(()) => { context .set_config_internal(Config::Selfavatar, Some(&avatar)) diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index 732047eb01..932d69804b 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -1,6 +1,7 @@ use anyhow::Result; use pretty_assertions::assert_eq; +use crate::blob; use crate::chat::{self, add_contact_to_chat, Chat, ProtectionStatus}; use crate::chatlist::Chatlist; use crate::config::Config; @@ -909,6 +910,37 @@ async fn test_no_unencrypted_name_if_verified() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_contact_avatar() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + enable_verified_oneonone_chats(&[alice]).await; + let bob = &tcm.bob().await; + mark_as_verified(alice, bob).await; + let alice_bob_chat = alice.create_chat(bob).await; + let file = alice.dir.path().join("avatar.jpg"); + let bytes = include_bytes!("../../test-data/image/avatar1000x1000.jpg"); + tokio::fs::write(&file, bytes).await?; + alice + .set_config(Config::Selfavatar, Some(file.to_str().unwrap())) + .await?; + let sent_msg = alice + .send_text(alice_bob_chat.id, "hello, I have a new avatar") + .await; + bob.recv_msg(&sent_msg).await; + let bob_alice_contact = bob.add_or_lookup_contact(alice).await; + let avatar_path = bob_alice_contact.get_profile_image(bob).await?.unwrap(); + tokio::task::block_in_place(move || { + let (_, exif) = blob::image_metadata(&std::fs::File::open(&avatar_path)?)?; + assert!(exif.is_none()); + let img = image::open(&avatar_path)?; + assert_eq!(img.width(), 1000); + assert_eq!(img.height(), 1000); + Result::<()>::Ok(()) + })?; + Ok(()) +} + // ============== Helper Functions ============== async fn assert_verified(this: &TestContext, other: &TestContext, protected: ProtectionStatus) { From 1e7d3d4d1d15d58f318eb0ca9d729ef1cf88348f Mon Sep 17 00:00:00 2001 From: iequidoo Date: Wed, 7 Aug 2024 19:09:28 -0300 Subject: [PATCH 2/2] test: that self-chat isn't protected, to record the current behaviour --- src/tests/verified_chats.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index 932d69804b..a01514db52 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -941,6 +941,20 @@ async fn test_contact_avatar() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_self_chat() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + enable_verified_oneonone_chats(&[alice]).await; + let chat = alice.get_self_chat().await; + assert!(!chat.is_protected()); + assert_eq!( + chat.id.is_protected(alice).await?, + ProtectionStatus::Unprotected + ); + Ok(()) +} + // ============== Helper Functions ============== async fn assert_verified(this: &TestContext, other: &TestContext, protected: ProtectionStatus) {