Skip to content

Fix downscaling images #2469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 7, 2021
Merged

Fix downscaling images #2469

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
428d855
Add failing test
Hocuri Jun 5, 2021
b1974b8
typo
Hocuri Jun 5, 2021
4ca41fe
Fix the bug
Hocuri Jun 5, 2021
c183203
Remove TODO for now
Hocuri Jun 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 53 additions & 4 deletions src/blob.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ impl<'a> BlobObject<'a> {
img.write_to(encoded, image::ImageFormat::Jpeg)?;
Ok(())
}
fn encode_img_exceeds_bytes(
fn encoded_img_exceeds_bytes(
Copy link
Contributor

@r10s r10s Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if the new name is better. the function still encodes an image and does not only check if an encoded image exceeds some bytes as the new name implies imho.

maybe sth. as encode_image_and_check_byte_size would describe the function better.

just some thoughts. otoh, it is only an inner function used as a shortcut - so, at the end, i would be fine with all variations, this is not a blocker in any way :)

Copy link
Collaborator Author

@Hocuri Hocuri Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for changing was: I tried to make it clear that the fn does not always encode the image, because that's the wrong assumption that caused the bug in the first place. It just encodes it if necessary, and in this case it writes the result into encoded for performance reasons (so that it doesn't have to be encoded again later).

OTOH, you are right that the fn still sometimes does encode the image.

Copy link
Contributor

@r10s r10s Jun 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, as said, just leave it as is :)

context: &Context,
img: &DynamicImage,
max_bytes: Option<usize>,
Expand All @@ -477,7 +477,7 @@ impl<'a> BlobObject<'a> {
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)?;
exceeds_width || encoded_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 {
Expand All @@ -500,7 +500,7 @@ impl<'a> BlobObject<'a> {
loop {
let new_img = img.thumbnail(img_wh, img_wh);

if encode_img_exceeds_bytes(context, &new_img, max_bytes, &mut encoded)? {
if encoded_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",
Expand All @@ -511,6 +511,10 @@ impl<'a> BlobObject<'a> {

img_wh = img_wh * 2 / 3;
} else {
if encoded.is_empty() {
encode_img(&new_img, &mut encoded)?;
}

info!(
context,
"Final scaled-down image size: {}B ({}px)",
Expand Down Expand Up @@ -617,7 +621,7 @@ mod tests {

use super::*;

use crate::test_utils::TestContext;
use crate::{message::Message, test_utils::TestContext};

#[async_std::test]
async fn test_create() {
Expand Down Expand Up @@ -915,4 +919,49 @@ mod tests {
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() -> anyhow::Result<()> {
for (media_quality_config, image_size) in vec![
(Some("0"), 1000), // BALANCED_IMAGE_SIZE > 1000, the original image size, so the image is not scaled down
Copy link
Contributor

@r10s r10s Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a subsequent pr, consider adding an image with eg. 2000x2000 px, so that scaling can be testing in more variations (if the jpg only contains few information, even these huge images won't blow up the repo too much)

(Some("1"), WORSE_IMAGE_SIZE),
]
.into_iter()
{
let alice = TestContext::new_alice().await;
let bob = TestContext::new_bob().await;
alice
.set_config(Config::MediaQuality, media_quality_config)
.await?;

let file = alice.get_blobdir().join("file.jpg");
let bytes = include_bytes!("../test-data/image/avatar1000x1000.jpg");
File::create(&file).await?.write_all(bytes).await?;

let img = image::open(&file)?;
assert_eq!(img.width(), 1000);
assert_eq!(img.height(), 1000);

let mut msg = Message::new(Viewtype::Image);
msg.set_file(file.to_str().unwrap(), None);
let chat = alice.create_chat(&bob).await;
let sent = alice.send_msg(chat.id, &mut msg).await;

let alice_msg = alice.get_last_msg().await;
assert_eq!(alice_msg.get_width() as u32, image_size);
assert_eq!(alice_msg.get_height() as u32, image_size);
let img = image::open(alice_msg.get_file(&alice).unwrap())?;
assert_eq!(img.width() as u32, image_size);
assert_eq!(img.height() as u32, image_size);

bob.recv_msg(&sent).await;
let bob_msg = bob.get_last_msg().await;
assert_eq!(bob_msg.get_width() as u32, image_size);
assert_eq!(bob_msg.get_height() as u32, image_size);
let img = image::open(bob_msg.get_file(&bob).unwrap())?;
assert_eq!(img.width() as u32, image_size);
assert_eq!(img.height() as u32, image_size);
}
Ok(())
}
}