From b7864f232bbac4c585e75f1a3073d1175727e2f9 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 19:54:13 +0200 Subject: [PATCH] Compress avatar to below 20k (#2384) - Currently, group images are compressed as well because it was easier to implement that way. - Currently, in the unlikely case that the avatar is compressed down to 20x20 pixels but still bigger than 20KB, the user doesn't get any indication of this, the avatar simply isn't changed (at least on Android). If we want to change this, the easiest way is probably to let `dc_set_config()` in the ffi call `error!()` if `Selfavatar` can't be set. The same might make sense for some or all other configs. BUUUUUT: At least Android doesn't show error!() toasts anymore, probably because they were used too often and too spammy. - The factor by which we scale down if the file is too big is 1.5. --- deltachat-ffi/src/lib.rs | 11 +- src/blob.rs | 235 ++++++++++++++++++++++++++++++++++++--- src/chat.rs | 2 +- src/config.rs | 82 +------------- src/sql.rs | 20 +++- src/sql/migrations.rs | 14 ++- 6 files changed, 258 insertions(+), 106 deletions(-) diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index 6de519c10..3506c7891 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -21,6 +21,7 @@ use std::ptr; use std::str::FromStr; use std::time::{Duration, SystemTime}; +use anyhow::Context as _; use async_std::task::{block_on, spawn}; use num_traits::{FromPrimitive, ToPrimitive}; @@ -130,12 +131,14 @@ pub unsafe extern "C" fn dc_set_config( return 0; } let ctx = &*context; - match config::Config::from_str(&to_string_lossy(key)) { - // When ctx.set_config() fails it already logged the error. - // TODO: Context::set_config() should not log this + let key = to_string_lossy(key); + match config::Config::from_str(&key) { Ok(key) => block_on(async move { - ctx.set_config(key, to_opt_string_lossy(value).as_deref()) + let value = to_opt_string_lossy(value); + ctx.set_config(key, value.as_deref()) .await + .with_context(|| format!("Can't set {} to {:?}", key, value)) + .log_err(ctx, "dc_set_config() failed") .is_ok() as libc::c_int }), Err(_) => { diff --git a/src/blob.rs b/src/blob.rs index e52152fc0..7892d17f2 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -1,5 +1,6 @@ //! # Blob directory management +use core::cmp::max; use std::ffi::OsStr; use std::fmt; @@ -7,8 +8,12 @@ use async_std::path::{Path, PathBuf}; use async_std::prelude::*; use async_std::{fs, io}; +use anyhow::format_err; +use anyhow::Context as _; use anyhow::Error; +use image::DynamicImage; use image::GenericImageView; +use image::ImageFormat; use num_traits::FromPrimitive; use thiserror::Error; @@ -380,7 +385,7 @@ impl<'a> BlobObject<'a> { true } - pub async fn recode_to_avatar_size(&self, context: &Context) -> Result<(), BlobError> { + pub async fn recode_to_avatar_size(&mut self, context: &Context) -> Result<(), BlobError> { let blob_abs = self.to_abs_path(); let img_wh = @@ -391,7 +396,15 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => WORSE_AVATAR_SIZE, }; - self.recode_to_size(context, blob_abs, img_wh).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. + if let Some(new_name) = self + .recode_to_size(context, blob_abs, img_wh, Some(20_000)) + .await? + { + self.name = new_name; + } + Ok(()) } pub async fn recode_to_image_size(&self, context: &Context) -> Result<(), BlobError> { @@ -410,30 +423,69 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => WORSE_IMAGE_SIZE, }; - self.recode_to_size(context, blob_abs, img_wh).await + if self + .recode_to_size(context, blob_abs, img_wh, None) + .await? + .is_some() + { + return Err(format_err!( + "Internal error: recode_to_size(..., None) shouldn't change the name of the image" + ) + .into()); + } + Ok(()) } async fn recode_to_size( &self, context: &Context, - blob_abs: PathBuf, - img_wh: u32, - ) -> Result<(), BlobError> { + mut blob_abs: PathBuf, + mut img_wh: u32, + max_bytes: Option, + ) -> Result, BlobError> { let mut img = image::open(&blob_abs).map_err(|err| BlobError::RecodeFailure { blobdir: context.get_blobdir().to_path_buf(), blobname: blob_abs.to_str().unwrap_or_default().to_string(), cause: err, })?; let orientation = self.get_exif_orientation(context); + let mut encoded = Vec::new(); + let mut changed_name = None; - let do_scale = img.width() > img_wh || img.height() > img_wh; + fn encode_img(img: &DynamicImage, encoded: &mut Vec) -> anyhow::Result<()> { + encoded.clear(); + img.write_to(encoded, image::ImageFormat::Jpeg)?; + Ok(()) + } + fn encode_img_exceeds_bytes( + context: &Context, + img: &DynamicImage, + max_bytes: Option, + encoded: &mut Vec, + ) -> anyhow::Result { + if let Some(max_bytes) = max_bytes { + encode_img(img, encoded)?; + if encoded.len() > max_bytes { + info!( + context, + "image size {}B ({}x{}px) exceeds {}B, need to scale down", + encoded.len(), + img.width(), + img.height(), + max_bytes, + ); + return Ok(true); + } + } + Ok(false) + } + let exceeds_width = img.width() > img_wh || img.height() > img_wh; + + let do_scale = + exceeds_width || encode_img_exceeds_bytes(context, &img, max_bytes, &mut encoded)?; let do_rotate = matches!(orientation, Ok(90) | Ok(180) | Ok(270)); if do_scale || do_rotate { - if do_scale { - img = img.thumbnail(img_wh, img_wh); - } - if do_rotate { img = match orientation { Ok(90) => img.rotate90(), @@ -443,14 +495,60 @@ impl<'a> BlobObject<'a> { } } - img.save(&blob_abs).map_err(|err| BlobError::WriteFailure { - blobdir: context.get_blobdir().to_path_buf(), - blobname: blob_abs.to_str().unwrap_or_default().to_string(), - cause: err.into(), - })?; + if do_scale { + if !exceeds_width { + // The image is already smaller than img_wh, but exceeds max_bytes + // We can directly start with trying to scale down to 2/3 of its current width + img_wh = max(img.width(), img.height()) * 2 / 3 + } + + loop { + let new_img = img.thumbnail(img_wh, img_wh); + + if encode_img_exceeds_bytes(context, &new_img, max_bytes, &mut encoded)? { + if img_wh < 20 { + return Err(format_err!( + "Failed to scale image to below {}B", + max_bytes.unwrap_or_default() + ) + .into()); + } + + img_wh = img_wh * 2 / 3; + } else { + info!( + context, + "Final scaled-down image size: {}B ({}px)", + encoded.len(), + img_wh + ); + break; + } + } + } + + // The file format is JPEG now, we may have to change the file extension + if !matches!(ImageFormat::from_path(&blob_abs), Ok(ImageFormat::Jpeg)) { + blob_abs = blob_abs.with_extension("jpg"); + let file_name = blob_abs.file_name().context("No avatar file name (???)")?; + let file_name = file_name.to_str().context("Filename is no UTF-8 (???)")?; + changed_name = Some(format!("$BLOBDIR/{}", file_name)); + } + + if encoded.is_empty() { + encode_img(&img, &mut encoded)?; + } + + fs::write(&blob_abs, &encoded) + .await + .map_err(|err| BlobError::WriteFailure { + blobdir: context.get_blobdir().to_path_buf(), + blobname: blob_abs.to_str().unwrap_or_default().to_string(), + cause: err.into(), + })?; } - Ok(()) + Ok(changed_name) } pub fn get_exif_orientation(&self, context: &Context) -> Result { @@ -522,6 +620,8 @@ pub enum BlobError { #[cfg(test)] mod tests { + use fs::File; + use super::*; use crate::test_utils::TestContext; @@ -715,4 +815,105 @@ mod tests { assert!(!stem.contains('*')); assert!(!stem.contains('?')); } + + #[async_std::test] + async fn test_selfavatar_outside_blobdir() { + let t = TestContext::new().await; + let avatar_src = t.dir.path().join("avatar.jpg"); + let avatar_bytes = include_bytes!("../test-data/image/avatar1000x1000.jpg"); + File::create(&avatar_src) + .await + .unwrap() + .write_all(avatar_bytes) + .await + .unwrap(); + let avatar_blob = t.get_blobdir().join("avatar.jpg"); + assert!(!avatar_blob.exists().await); + t.set_config(Config::Selfavatar, Some(avatar_src.to_str().unwrap())) + .await + .unwrap(); + assert!(avatar_blob.exists().await); + assert!(std::fs::metadata(&avatar_blob).unwrap().len() < avatar_bytes.len() as u64); + let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap(); + assert_eq!(avatar_cfg, avatar_blob.to_str().map(|s| s.to_string())); + + let img = image::open(avatar_src).unwrap(); + assert_eq!(img.width(), 1000); + assert_eq!(img.height(), 1000); + + let img = image::open(&avatar_blob).unwrap(); + assert_eq!(img.width(), BALANCED_AVATAR_SIZE); + assert_eq!(img.height(), BALANCED_AVATAR_SIZE); + + async fn file_size(path_buf: &PathBuf) -> u64 { + let file = File::open(path_buf).await.unwrap(); + file.metadata().await.unwrap().len() + } + + let blob = BlobObject::new_from_path(&t, &avatar_blob).await.unwrap(); + + blob.recode_to_size(&t, blob.to_abs_path(), 1000, Some(3000)) + .await + .unwrap(); + assert!(file_size(&avatar_blob).await <= 3000); + assert!(file_size(&avatar_blob).await > 2000); + let img = image::open(&avatar_blob).unwrap(); + assert!(img.width() > 130); + assert_eq!(img.width(), img.height()); + } + + #[async_std::test] + async fn test_selfavatar_in_blobdir() { + let t = TestContext::new().await; + let avatar_src = t.get_blobdir().join("avatar.png"); + let avatar_bytes = include_bytes!("../test-data/image/avatar900x900.png"); + File::create(&avatar_src) + .await + .unwrap() + .write_all(avatar_bytes) + .await + .unwrap(); + + let img = image::open(&avatar_src).unwrap(); + assert_eq!(img.width(), 900); + assert_eq!(img.height(), 900); + + t.set_config(Config::Selfavatar, Some(avatar_src.to_str().unwrap())) + .await + .unwrap(); + let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap().unwrap(); + assert_eq!( + avatar_cfg, + avatar_src.with_extension("jpg").to_str().unwrap() + ); + + let img = image::open(avatar_cfg).unwrap(); + assert_eq!(img.width(), BALANCED_AVATAR_SIZE); + assert_eq!(img.height(), BALANCED_AVATAR_SIZE); + } + + #[async_std::test] + async fn test_selfavatar_copy_without_recode() { + let t = TestContext::new().await; + let avatar_src = t.dir.path().join("avatar.png"); + let avatar_bytes = include_bytes!("../test-data/image/avatar64x64.png"); + File::create(&avatar_src) + .await + .unwrap() + .write_all(avatar_bytes) + .await + .unwrap(); + let avatar_blob = t.get_blobdir().join("avatar.png"); + assert!(!avatar_blob.exists().await); + t.set_config(Config::Selfavatar, Some(avatar_src.to_str().unwrap())) + .await + .unwrap(); + assert!(avatar_blob.exists().await); + assert_eq!( + std::fs::metadata(&avatar_blob).unwrap().len(), + avatar_bytes.len() as u64 + ); + let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap(); + assert_eq!(avatar_cfg, avatar_blob.to_str().map(|s| s.to_string())); + } } diff --git a/src/chat.rs b/src/chat.rs index 0908eac9c..75aaf638c 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2660,7 +2660,7 @@ pub async fn set_chat_profile_image( msg.param.remove(Param::Arg); msg.text = Some(stock_str::msg_grp_img_deleted(context, DC_CONTACT_ID_SELF as u32).await); } else { - let image_blob = match BlobObject::from_path(context, Path::new(new_image.as_ref())) { + let mut image_blob = match BlobObject::from_path(context, Path::new(new_image.as_ref())) { Ok(blob) => Ok(blob), Err(err) => match err { BlobError::WrongBlobdir { .. } => { diff --git a/src/config.rs b/src/config.rs index 67bcf4ff7..9e16ef387 100644 --- a/src/config.rs +++ b/src/config.rs @@ -249,7 +249,7 @@ impl Context { .await?; match value { Some(value) => { - let blob = BlobObject::new_from_path(self, value).await?; + let mut blob = BlobObject::new_from_path(self, value).await?; blob.recode_to_avatar_size(self).await?; self.sql.set_raw_config(key, Some(blob.as_name())).await?; Ok(()) @@ -331,12 +331,8 @@ mod tests { use std::string::ToString; use crate::constants; - use crate::constants::BALANCED_AVATAR_SIZE; use crate::test_utils::TestContext; - use image::GenericImageView; use num_traits::FromPrimitive; - use std::fs::File; - use std::io::Write; #[test] fn test_to_string() { @@ -350,82 +346,6 @@ mod tests { ); } - #[async_std::test] - async fn test_selfavatar_outside_blobdir() { - let t = TestContext::new().await; - let avatar_src = t.dir.path().join("avatar.jpg"); - let avatar_bytes = include_bytes!("../test-data/image/avatar1000x1000.jpg"); - File::create(&avatar_src) - .unwrap() - .write_all(avatar_bytes) - .unwrap(); - let avatar_blob = t.get_blobdir().join("avatar.jpg"); - assert!(!avatar_blob.exists().await); - t.set_config(Config::Selfavatar, Some(avatar_src.to_str().unwrap())) - .await - .unwrap(); - assert!(avatar_blob.exists().await); - assert!(std::fs::metadata(&avatar_blob).unwrap().len() < avatar_bytes.len() as u64); - let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap(); - assert_eq!(avatar_cfg, avatar_blob.to_str().map(|s| s.to_string())); - - let img = image::open(avatar_src).unwrap(); - assert_eq!(img.width(), 1000); - assert_eq!(img.height(), 1000); - - let img = image::open(avatar_blob).unwrap(); - assert_eq!(img.width(), BALANCED_AVATAR_SIZE); - assert_eq!(img.height(), BALANCED_AVATAR_SIZE); - } - - #[async_std::test] - async fn test_selfavatar_in_blobdir() { - let t = TestContext::new().await; - let avatar_src = t.get_blobdir().join("avatar.png"); - let avatar_bytes = include_bytes!("../test-data/image/avatar900x900.png"); - File::create(&avatar_src) - .unwrap() - .write_all(avatar_bytes) - .unwrap(); - - let img = image::open(&avatar_src).unwrap(); - assert_eq!(img.width(), 900); - assert_eq!(img.height(), 900); - - t.set_config(Config::Selfavatar, Some(avatar_src.to_str().unwrap())) - .await - .unwrap(); - let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap(); - assert_eq!(avatar_cfg, avatar_src.to_str().map(|s| s.to_string())); - - let img = image::open(avatar_src).unwrap(); - assert_eq!(img.width(), BALANCED_AVATAR_SIZE); - assert_eq!(img.height(), BALANCED_AVATAR_SIZE); - } - - #[async_std::test] - async fn test_selfavatar_copy_without_recode() { - let t = TestContext::new().await; - let avatar_src = t.dir.path().join("avatar.png"); - let avatar_bytes = include_bytes!("../test-data/image/avatar64x64.png"); - File::create(&avatar_src) - .unwrap() - .write_all(avatar_bytes) - .unwrap(); - let avatar_blob = t.get_blobdir().join("avatar.png"); - assert!(!avatar_blob.exists().await); - t.set_config(Config::Selfavatar, Some(avatar_src.to_str().unwrap())) - .await - .unwrap(); - assert!(avatar_blob.exists().await); - assert_eq!( - std::fs::metadata(&avatar_blob).unwrap().len(), - avatar_bytes.len() as u64 - ); - let avatar_cfg = t.get_config(Config::Selfavatar).await.unwrap(); - assert_eq!(avatar_cfg, avatar_blob.to_str().map(|s| s.to_string())); - } - #[async_std::test] async fn test_media_quality_config_option() { let t = TestContext::new().await; diff --git a/src/sql.rs b/src/sql.rs index 98b2596c2..4b67b3183 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -11,6 +11,7 @@ use anyhow::Context as _; use async_std::prelude::*; use rusqlite::OpenFlags; +use crate::blob::BlobObject; use crate::chat::{add_device_msg, update_device_icon, update_saved_messages_icon}; use crate::config::Config; use crate::constants::{Viewtype, DC_CHAT_ID_TRASH}; @@ -138,7 +139,7 @@ impl Sql { // this should be done before updates that use high-level objects that // rely themselves on the low-level structure. - let (recalc_fingerprints, update_icons, disable_server_delete) = + let (recalc_fingerprints, update_icons, disable_server_delete, recode_avatar) = migrations::run(context, self).await?; // (2) updates that require high-level objects @@ -183,6 +184,23 @@ impl Sql { .await?; } } + + if recode_avatar { + if let Some(avatar) = context.get_config(Config::Selfavatar).await? { + let mut blob = BlobObject::new_from_path(context, &avatar).await?; + match blob.recode_to_avatar_size(context).await { + Ok(()) => { + context + .set_config(Config::Selfavatar, Some(&avatar)) + .await? + } + Err(e) => { + warn!(context, "Migrations can't recode avatar, removing. {:#}", e); + context.set_config(Config::Selfavatar, None).await? + } + } + } + } } info!(context, "Opened {:?}.", dbfile.as_ref()); diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index c26a0bfac..ae40bc38a 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -10,7 +10,7 @@ const DBVERSION: i32 = 68; const VERSION_CFG: &str = "dbversion"; const TABLES: &str = include_str!("./tables.sql"); -pub async fn run(context: &Context, sql: &Sql) -> Result<(bool, bool, bool)> { +pub async fn run(context: &Context, sql: &Sql) -> Result<(bool, bool, bool, bool)> { let mut recalc_fingerprints = false; let mut exists_before_update = false; let mut dbversion_before_update = DBVERSION; @@ -39,6 +39,7 @@ pub async fn run(context: &Context, sql: &Sql) -> Result<(bool, bool, bool)> { let dbversion = dbversion_before_update; let mut update_icons = !exists_before_update; let mut disable_server_delete = false; + let mut recode_avatar = false; if dbversion < 1 { info!(context, "[migration] v1"); @@ -460,8 +461,17 @@ paramsv![] sql.execute_migration("ALTER TABLE msgs ADD COLUMN subject TEXT DEFAULT '';", 76) .await?; } + if dbversion < 77 { + info!(context, "[migration] v77"); + recode_avatar = true; + } - Ok((recalc_fingerprints, update_icons, disable_server_delete)) + Ok(( + recalc_fingerprints, + update_icons, + disable_server_delete, + recode_avatar, + )) } impl Sql {