From dd7b3aa9c70c4755ca34fcd1f0d45d7ed057a7df Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 29 Apr 2021 15:18:47 +0200 Subject: [PATCH 01/14] First try compressing avatar to below 20KB --- src/blob.rs | 32 ++++++++++++++++++++++++++++---- src/sql.rs | 13 ++++++++++++- src/sql/migrations.rs | 14 ++++++++++++-- 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index e52152fc08..ca16cc5918 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -7,6 +7,7 @@ use async_std::path::{Path, PathBuf}; use async_std::prelude::*; use async_std::{fs, io}; +use anyhow::format_err; use anyhow::Error; use image::GenericImageView; use num_traits::FromPrimitive; @@ -381,6 +382,7 @@ impl<'a> BlobObject<'a> { } pub async fn recode_to_avatar_size(&self, context: &Context) -> Result<(), BlobError> { + // TODO can fail now!!!! let blob_abs = self.to_abs_path(); let img_wh = @@ -391,7 +393,9 @@ 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 as that's the max header size of some servers + self.recode_to_size(context, blob_abs, img_wh, Some(20_000)) + .await } pub async fn recode_to_image_size(&self, context: &Context) -> Result<(), BlobError> { @@ -410,14 +414,15 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => WORSE_IMAGE_SIZE, }; - self.recode_to_size(context, blob_abs, img_wh).await + self.recode_to_size(context, blob_abs, img_wh, None).await } async fn recode_to_size( &self, context: &Context, blob_abs: PathBuf, - img_wh: u32, + 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(), @@ -428,11 +433,30 @@ impl<'a> BlobObject<'a> { let do_scale = img.width() > img_wh || img.height() > img_wh; let do_rotate = matches!(orientation, Ok(90) | Ok(180) | Ok(270)); + let exceeds_bytes = if let Some(max_bytes) = max_bytes { + img.as_bytes().len() > max_bytes + } else { + false + }; - if do_scale || do_rotate { + if do_scale || do_rotate || exceeds_bytes { + // TODO if do_scale { img = img.thumbnail(img_wh, img_wh); } + if let Some(max_bytes) = max_bytes { + while img.as_bytes().len() > max_bytes { + img_wh = img_wh / 2; + if img_wh < 50 { + return Err(format_err!( + "Image witdh is 50, but size is still {}", + img.as_bytes().len() + ) + .into()); + } + img = img.thumbnail(img_wh, img_wh); + } + } if do_rotate { img = match orientation { diff --git a/src/sql.rs b/src/sql.rs index f2c4782019..a16c1ff005 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,16 @@ impl Sql { .await?; } } + + if recode_avatar { + if let Some(avatar) = context.get_config(Config::Selfavatar).await? { + let blob = BlobObject::new_from_path(context, avatar).await?; + if let Err(e) = blob.recode_to_avatar_size(context).await { + 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 c26a0bfac1..ae40bc38a4 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 { From 2b1f68b0efacd6098a3fa98f8df71f3934408061 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 29 Apr 2021 19:49:44 +0200 Subject: [PATCH 02/14] Let dc_set_config() log again --- deltachat-ffi/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index 6de519c105..3506c7891e 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(_) => { From 4d9dba1a1ac3ae51062dcce0278f592ae8dbeb78 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 29 Apr 2021 20:20:00 +0200 Subject: [PATCH 03/14] Set min width down to 20px --- src/blob.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index ca16cc5918..71b265d4b2 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -382,7 +382,6 @@ impl<'a> BlobObject<'a> { } pub async fn recode_to_avatar_size(&self, context: &Context) -> Result<(), BlobError> { - // TODO can fail now!!!! let blob_abs = self.to_abs_path(); let img_wh = @@ -447,12 +446,11 @@ impl<'a> BlobObject<'a> { if let Some(max_bytes) = max_bytes { while img.as_bytes().len() > max_bytes { img_wh = img_wh / 2; - if img_wh < 50 { - return Err(format_err!( - "Image witdh is 50, but size is still {}", + if img_wh < 20 { + Err(format_err!( + "Image width is 20, but size is still {}", img.as_bytes().len() - ) - .into()); + ))? } img = img.thumbnail(img_wh, img_wh); } From 1713d25f94b3ae2ac029ec326325e07918bb545d Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 29 Apr 2021 20:30:18 +0200 Subject: [PATCH 04/14] Divide image by 1.5, not by 2 in each compression step --- src/blob.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index 71b265d4b2..fa5664f9c2 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -430,7 +430,7 @@ impl<'a> BlobObject<'a> { })?; let orientation = self.get_exif_orientation(context); - let do_scale = img.width() > img_wh || img.height() > img_wh; + let exceeds_width = img.width() > img_wh || img.height() > img_wh; let do_rotate = matches!(orientation, Ok(90) | Ok(180) | Ok(270)); let exceeds_bytes = if let Some(max_bytes) = max_bytes { img.as_bytes().len() > max_bytes @@ -438,23 +438,26 @@ impl<'a> BlobObject<'a> { false }; - if do_scale || do_rotate || exceeds_bytes { - // TODO - if do_scale { - img = img.thumbnail(img_wh, img_wh); + if exceeds_width || do_rotate || exceeds_bytes { + let mut scaled_img = None; + if exceeds_width { + scaled_img = Some(img.thumbnail(img_wh, img_wh)); } if let Some(max_bytes) = max_bytes { - while img.as_bytes().len() > max_bytes { - img_wh = img_wh / 2; + while scaled_img.as_ref().unwrap_or(&img).as_bytes().len() > max_bytes { + img_wh = img_wh * 2 / 3; if img_wh < 20 { Err(format_err!( - "Image width is 20, but size is still {}", + "Image width is <20, but size is still {}", img.as_bytes().len() ))? } - img = img.thumbnail(img_wh, img_wh); + scaled_img = Some(img.thumbnail(img_wh, img_wh)); } } + if let Some(scaled) = scaled_img { + img = scaled; + } if do_rotate { img = match orientation { From 4b862db868c5286f6296ba15638cb612eb1bd0d5 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 29 Apr 2021 21:24:40 +0200 Subject: [PATCH 05/14] Move selfavatar tests to blob.rs --- src/blob.rs | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/config.rs | 80 ------------------------------------------------ 2 files changed, 84 insertions(+), 80 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index fa5664f9c2..2d9ab5de79 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -547,6 +547,8 @@ pub enum BlobError { #[cfg(test)] mod tests { + use fs::File; + use super::*; use crate::test_utils::TestContext; @@ -740,4 +742,86 @@ 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_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(); + 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) + .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/config.rs b/src/config.rs index 67bcf4ff70..2677fca014 100644 --- a/src/config.rs +++ b/src/config.rs @@ -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; From f79c652c5dc6eac38c65fb29fd385dc50acad55d Mon Sep 17 00:00:00 2001 From: Hocuri Date: Thu, 29 Apr 2021 23:42:46 +0200 Subject: [PATCH 06/14] Correctly compute size of avatars, add simple test --- src/blob.rs | 94 +++++++++++++++++++++++++++++++++++++------------ src/dc_tools.rs | 28 +++++++++++++++ 2 files changed, 99 insertions(+), 23 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index 2d9ab5de79..aa7f86ad78 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -9,6 +9,7 @@ use async_std::{fs, io}; use anyhow::format_err; use anyhow::Error; +use image::DynamicImage; use image::GenericImageView; use num_traits::FromPrimitive; use thiserror::Error; @@ -19,6 +20,7 @@ use crate::constants::{ WORSE_IMAGE_SIZE, }; use crate::context::Context; +use crate::dc_tools::count_bytes; use crate::events::EventType; use crate::message; @@ -430,34 +432,64 @@ impl<'a> BlobObject<'a> { })?; let orientation = self.get_exif_orientation(context); + fn exceeds_bytes( + context: &Context, + img: &DynamicImage, + max_bytes: Option, + ) -> anyhow::Result { + if let Some(max_bytes) = max_bytes { + let size = count_bytes(img)?; + if size > max_bytes { + info!( + context, + "image size {}B ({}x{}px) exceeds {}B, need to scale down", + size, + 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 || exceeds_bytes(context, &img, max_bytes)?; let do_rotate = matches!(orientation, Ok(90) | Ok(180) | Ok(270)); - let exceeds_bytes = if let Some(max_bytes) = max_bytes { - img.as_bytes().len() > max_bytes - } else { - false - }; - if exceeds_width || do_rotate || exceeds_bytes { - let mut scaled_img = None; - if exceeds_width { - scaled_img = Some(img.thumbnail(img_wh, img_wh)); - } - if let Some(max_bytes) = max_bytes { - while scaled_img.as_ref().unwrap_or(&img).as_bytes().len() > max_bytes { - img_wh = img_wh * 2 / 3; - if img_wh < 20 { - Err(format_err!( - "Image width is <20, but size is still {}", - img.as_bytes().len() - ))? + if do_scale || do_rotate { + 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 = img.width() * 2 / 3 // TODO should be {max or min}(img.width(), img.height()) + } + + img = loop { + let new_img = img.thumbnail(img_wh, img_wh); + + if exceeds_bytes(context, &new_img, max_bytes)? { + 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)", + count_bytes(&new_img)?, + img_wh + ); // TODO dbg (?) + break new_img; } - scaled_img = Some(img.thumbnail(img_wh, img_wh)); } } - if let Some(scaled) = scaled_img { - img = scaled; - } if do_rotate { img = match orientation { @@ -768,9 +800,25 @@ mod tests { assert_eq!(img.width(), 1000); assert_eq!(img.height(), 1000); - let img = image::open(avatar_blob).unwrap(); + 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] diff --git a/src/dc_tools.rs b/src/dc_tools.rs index 9176e8dbf1..a4550c5d2f 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -14,6 +14,7 @@ use async_std::{fs, io}; use anyhow::{bail, Error}; use chrono::{Local, TimeZone}; +use image::DynamicImage; use rand::{thread_rng, Rng}; use crate::chat::{add_device_msg, add_device_msg_with_importance}; @@ -682,6 +683,33 @@ pub fn remove_subject_prefix(last_subject: &str) -> String { .to_string() } +struct ByteCounter { + count: usize, +} + +impl ByteCounter { + fn new() -> Self { + ByteCounter { count: 0 } + } +} + +impl std::io::Write for ByteCounter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.count += buf.len(); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +pub(crate) fn count_bytes(img: &DynamicImage) -> anyhow::Result { + let mut writer = ByteCounter::new(); + img.write_to(&mut writer, image::ImageFormat::Jpeg)?; + Ok(writer.count) +} + #[cfg(test)] mod tests { #![allow(clippy::indexing_slicing)] From a315711503b0f0fa5fc3516bce660fe5ea17fc97 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 13:18:36 +0200 Subject: [PATCH 07/14] Quick-and-dirty: Save encoded file to memory before writing it, we have the problem that JPEGs are saved as PNGs now though --- src/blob.rs | 53 +++++++++++++++++++++++++++++-------------------- src/dc_tools.rs | 28 -------------------------- 2 files changed, 32 insertions(+), 49 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index aa7f86ad78..baf21b5b83 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -20,7 +20,6 @@ use crate::constants::{ WORSE_IMAGE_SIZE, }; use crate::context::Context; -use crate::dc_tools::count_bytes; use crate::events::EventType; use crate::message; @@ -431,19 +430,22 @@ impl<'a> BlobObject<'a> { cause: err, })?; let orientation = self.get_exif_orientation(context); + let mut encoded = Vec::new(); fn exceeds_bytes( context: &Context, img: &DynamicImage, max_bytes: Option, + encoded: &mut Vec, ) -> anyhow::Result { if let Some(max_bytes) = max_bytes { - let size = count_bytes(img)?; - if size > max_bytes { + encoded.clear(); + img.write_to(encoded, image::ImageFormat::Png)?; + if encoded.len() > max_bytes { info!( context, "image size {}B ({}x{}px) exceeds {}B, need to scale down", - size, + encoded.len(), img.width(), img.height(), max_bytes, @@ -455,10 +457,19 @@ impl<'a> BlobObject<'a> { }; let exceeds_width = img.width() > img_wh || img.height() > img_wh; - let do_scale = exceeds_width || exceeds_bytes(context, &img, max_bytes)?; + let do_scale = exceeds_width || 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_rotate { + img = match orientation { + Ok(90) => img.rotate90(), + Ok(180) => img.rotate180(), + Ok(270) => img.rotate270(), + _ => img, + } + } + if do_scale { if !exceeds_width { // The image is already smaller than img_wh, but exceeds max_bytes @@ -469,7 +480,7 @@ impl<'a> BlobObject<'a> { img = loop { let new_img = img.thumbnail(img_wh, img_wh); - if exceeds_bytes(context, &new_img, max_bytes)? { + if exceeds_bytes(context, &new_img, max_bytes, &mut encoded)? { if img_wh < 20 { return Err(format_err!( "Failed to scale image to below {}B", @@ -483,28 +494,28 @@ impl<'a> BlobObject<'a> { info!( context, "Final scaled-down image size: {}B ({}px)", - count_bytes(&new_img)?, + encoded.len(), img_wh ); // TODO dbg (?) break new_img; } } + } else { + img.write_to(&mut encoded, image::ImageFormat::Jpeg) + .map_err(|err| BlobError::RecodeFailure { + blobdir: context.get_blobdir().to_path_buf(), + blobname: blob_abs.to_str().unwrap_or_default().to_string(), + cause: err.into(), + })?; } - if do_rotate { - img = match orientation { - Ok(90) => img.rotate90(), - Ok(180) => img.rotate180(), - Ok(270) => img.rotate270(), - _ => img, - } - } - - 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(), - })?; + 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(()) diff --git a/src/dc_tools.rs b/src/dc_tools.rs index a4550c5d2f..9176e8dbf1 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -14,7 +14,6 @@ use async_std::{fs, io}; use anyhow::{bail, Error}; use chrono::{Local, TimeZone}; -use image::DynamicImage; use rand::{thread_rng, Rng}; use crate::chat::{add_device_msg, add_device_msg_with_importance}; @@ -683,33 +682,6 @@ pub fn remove_subject_prefix(last_subject: &str) -> String { .to_string() } -struct ByteCounter { - count: usize, -} - -impl ByteCounter { - fn new() -> Self { - ByteCounter { count: 0 } - } -} - -impl std::io::Write for ByteCounter { - fn write(&mut self, buf: &[u8]) -> io::Result { - self.count += buf.len(); - Ok(buf.len()) - } - - fn flush(&mut self) -> io::Result<()> { - Ok(()) - } -} - -pub(crate) fn count_bytes(img: &DynamicImage) -> anyhow::Result { - let mut writer = ByteCounter::new(); - img.write_to(&mut writer, image::ImageFormat::Jpeg)?; - Ok(writer.count) -} - #[cfg(test)] mod tests { #![allow(clippy::indexing_slicing)] From 37c48d241c52af6d59c9ed8a5c05a8bc96863f62 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 14:53:52 +0200 Subject: [PATCH 08/14] Tests pass again --- src/blob.rs | 53 +++++++++++++++++++++++++++++++++++++-------------- src/chat.rs | 2 +- src/config.rs | 2 +- src/sql.rs | 15 +++++++++++---- 4 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index baf21b5b83..58b7a46be4 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -8,9 +8,11 @@ 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; @@ -382,7 +384,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 = @@ -394,8 +396,13 @@ impl<'a> BlobObject<'a> { }; // max_bytes is 20_000 bytes as that's the max header size of some servers - self.recode_to_size(context, blob_abs, img_wh, Some(20_000)) - .await + let new_name = self + .recode_to_size(context, blob_abs, img_wh, Some(20_000)) + .await?; + if new_name != "" { + self.name = new_name; + } + Ok(()) } pub async fn recode_to_image_size(&self, context: &Context) -> Result<(), BlobError> { @@ -414,16 +421,23 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => WORSE_IMAGE_SIZE, }; - self.recode_to_size(context, blob_abs, img_wh, None).await + let new_name = self.recode_to_size(context, blob_abs, img_wh, None).await?; + if new_name != "" { + 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, + mut blob_abs: PathBuf, mut img_wh: u32, max_bytes: Option, - ) -> Result<(), BlobError> { + ) -> Result { 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(), @@ -431,6 +445,7 @@ impl<'a> BlobObject<'a> { })?; let orientation = self.get_exif_orientation(context); let mut encoded = Vec::new(); + let mut changed_name = String::new(); fn exceeds_bytes( context: &Context, @@ -440,7 +455,7 @@ impl<'a> BlobObject<'a> { ) -> anyhow::Result { if let Some(max_bytes) = max_bytes { encoded.clear(); - img.write_to(encoded, image::ImageFormat::Png)?; + img.write_to(encoded, image::ImageFormat::Jpeg)?; if encoded.len() > max_bytes { info!( context, @@ -477,7 +492,7 @@ impl<'a> BlobObject<'a> { img_wh = img.width() * 2 / 3 // TODO should be {max or min}(img.width(), img.height()) } - img = loop { + loop { let new_img = img.thumbnail(img_wh, img_wh); if exceeds_bytes(context, &new_img, max_bytes, &mut encoded)? { @@ -497,11 +512,11 @@ impl<'a> BlobObject<'a> { encoded.len(), img_wh ); // TODO dbg (?) - break new_img; + break; } } } else { - img.write_to(&mut encoded, image::ImageFormat::Jpeg) + img.write_to(&mut encoded, ImageFormat::Jpeg) .map_err(|err| BlobError::RecodeFailure { blobdir: context.get_blobdir().to_path_buf(), blobname: blob_abs.to_str().unwrap_or_default().to_string(), @@ -509,6 +524,13 @@ impl<'a> BlobObject<'a> { })?; } + 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 = format!("$BLOBDIR/{}", file_name); + } + fs::write(&blob_abs, &encoded) .await .map_err(|err| BlobError::WriteFailure { @@ -518,7 +540,7 @@ impl<'a> BlobObject<'a> { })?; } - Ok(()) + Ok(changed_name) } pub fn get_exif_orientation(&self, context: &Context) -> Result { @@ -851,10 +873,13 @@ mod tests { 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 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_src).unwrap(); + let img = image::open(avatar_cfg).unwrap(); assert_eq!(img.width(), BALANCED_AVATAR_SIZE); assert_eq!(img.height(), BALANCED_AVATAR_SIZE); } diff --git a/src/chat.rs b/src/chat.rs index e301556750..023aed7f80 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2710,7 +2710,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 2677fca014..9e16ef3876 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(()) diff --git a/src/sql.rs b/src/sql.rs index a16c1ff005..acf7ded1db 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -187,10 +187,17 @@ impl Sql { if recode_avatar { if let Some(avatar) = context.get_config(Config::Selfavatar).await? { - let blob = BlobObject::new_from_path(context, avatar).await?; - if let Err(e) = blob.recode_to_avatar_size(context).await { - warn!(context, "Migrations can't recode avatar, removing. {:#}", e); - context.set_config(Config::Selfavatar, None).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? + } } } } From 02471f1a283b871ccfa882460b04650c9d1b1d5b Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 15:05:51 +0200 Subject: [PATCH 09/14] Improve code style (I hope) --- src/blob.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index 58b7a46be4..7c088fee7f 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -447,15 +447,19 @@ impl<'a> BlobObject<'a> { let mut encoded = Vec::new(); let mut changed_name = String::new(); - fn exceeds_bytes( + 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 { - encoded.clear(); - img.write_to(encoded, image::ImageFormat::Jpeg)?; + encode_img(&img, encoded)?; if encoded.len() > max_bytes { info!( context, @@ -469,10 +473,11 @@ impl<'a> BlobObject<'a> { } } Ok(false) - }; + } let exceeds_width = img.width() > img_wh || img.height() > img_wh; - let do_scale = exceeds_width || exceeds_bytes(context, &img, max_bytes, &mut encoded)?; + 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 { @@ -495,7 +500,7 @@ impl<'a> BlobObject<'a> { loop { let new_img = img.thumbnail(img_wh, img_wh); - if exceeds_bytes(context, &new_img, max_bytes, &mut encoded)? { + 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", @@ -511,19 +516,13 @@ impl<'a> BlobObject<'a> { "Final scaled-down image size: {}B ({}px)", encoded.len(), img_wh - ); // TODO dbg (?) + ); break; } } - } else { - img.write_to(&mut encoded, ImageFormat::Jpeg) - .map_err(|err| BlobError::RecodeFailure { - blobdir: context.get_blobdir().to_path_buf(), - blobname: blob_abs.to_str().unwrap_or_default().to_string(), - cause: err.into(), - })?; } + // 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 (???)")?; @@ -531,6 +530,10 @@ impl<'a> BlobObject<'a> { changed_name = format!("$BLOBDIR/{}", file_name); } + if encoded.is_empty() { + encode_img(&img, &mut encoded)?; + } + fs::write(&blob_abs, &encoded) .await .map_err(|err| BlobError::WriteFailure { From 63be3591efd9b144cd09980370801503bf35a315 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 15:14:00 +0200 Subject: [PATCH 10/14] comment --- src/blob.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/blob.rs b/src/blob.rs index 7c088fee7f..b6d8c0d0ec 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -395,7 +395,8 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => WORSE_AVATAR_SIZE, }; - // max_bytes is 20_000 bytes as that's the max header size of some servers + // 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 new_name = self .recode_to_size(context, blob_abs, img_wh, Some(20_000)) .await?; From b8fc08548d8dd129b3ef647cc5d48b1aec701fb0 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 15:30:07 +0200 Subject: [PATCH 11/14] clippy --- src/blob.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index b6d8c0d0ec..adccbb9eaf 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -423,7 +423,7 @@ impl<'a> BlobObject<'a> { }; let new_name = self.recode_to_size(context, blob_abs, img_wh, None).await?; - if new_name != "" { + if !new_name.is_empty() { return Err(format_err!( "Internal error: recode_to_size(..., None) shouldn't change the name of the image" ) @@ -460,7 +460,7 @@ impl<'a> BlobObject<'a> { encoded: &mut Vec, ) -> anyhow::Result { if let Some(max_bytes) = max_bytes { - encode_img(&img, encoded)?; + encode_img(img, encoded)?; if encoded.len() > max_bytes { info!( context, From a4d52234ae111bbf54f807d801b7853b2b82b100 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 16:00:42 +0200 Subject: [PATCH 12/14] complete TODO --- src/blob.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/blob.rs b/src/blob.rs index adccbb9eaf..d9feaf7df2 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; @@ -495,7 +496,7 @@ impl<'a> BlobObject<'a> { 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 = img.width() * 2 / 3 // TODO should be {max or min}(img.width(), img.height()) + img_wh = max(img.width(), img.height()) * 2 / 3 } loop { From 1f8aaa0e0e5703a04be7e73075299b41e7832413 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sun, 2 May 2021 16:35:21 +0200 Subject: [PATCH 13/14] clippy --- src/blob.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blob.rs b/src/blob.rs index d9feaf7df2..adda0a7f8d 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -401,7 +401,7 @@ impl<'a> BlobObject<'a> { let new_name = self .recode_to_size(context, blob_abs, img_wh, Some(20_000)) .await?; - if new_name != "" { + if !new_name.is_empty() { self.name = new_name; } Ok(()) From 52fb73acda43d8fbd560408310ef23fd464d9f6b Mon Sep 17 00:00:00 2001 From: link2xt Date: Sun, 2 May 2021 20:09:15 +0300 Subject: [PATCH 14/14] Return `None` instead of empty string from `recode_to_size` --- src/blob.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/blob.rs b/src/blob.rs index adda0a7f8d..7892d17f2b 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -398,10 +398,10 @@ impl<'a> BlobObject<'a> { // 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 new_name = self + if let Some(new_name) = self .recode_to_size(context, blob_abs, img_wh, Some(20_000)) - .await?; - if !new_name.is_empty() { + .await? + { self.name = new_name; } Ok(()) @@ -423,8 +423,11 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => WORSE_IMAGE_SIZE, }; - let new_name = self.recode_to_size(context, blob_abs, img_wh, None).await?; - if !new_name.is_empty() { + 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" ) @@ -439,7 +442,7 @@ impl<'a> BlobObject<'a> { mut blob_abs: PathBuf, mut img_wh: u32, max_bytes: Option, - ) -> Result { + ) -> 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(), @@ -447,7 +450,7 @@ impl<'a> BlobObject<'a> { })?; let orientation = self.get_exif_orientation(context); let mut encoded = Vec::new(); - let mut changed_name = String::new(); + let mut changed_name = None; fn encode_img(img: &DynamicImage, encoded: &mut Vec) -> anyhow::Result<()> { encoded.clear(); @@ -529,7 +532,7 @@ impl<'a> BlobObject<'a> { 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 = format!("$BLOBDIR/{}", file_name); + changed_name = Some(format!("$BLOBDIR/{}", file_name)); } if encoded.is_empty() {