-
-
Notifications
You must be signed in to change notification settings - Fork 118
fix: Correct condition for re-encoding avatar-images that include exif-data #7772
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.
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
Conversation
| if mem::take(&mut add_white_bg) { | ||
| self::add_white_bg(&mut img); | ||
| } | ||
| encoded_img_exceeds_bytes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encoded_img_exceeds_bytes() already recodes the image, that's why it takes &mut encoded, this encoded is then passed to BlobObject::create_and_deduplicate_from_bytes(). Maybe rename it to just encode_img_ex() to avoid any wrong assumptions about what it does? Or, if i'm wrong, adding a test for the scenario being fixed will help, w/o tests we always get regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function does add a file-size-check to encode_img, that returns false or true.
Lines 653 to 673 in f43b3b9
| fn encoded_img_exceeds_bytes( | |
| context: &Context, | |
| img: &DynamicImage, | |
| fmt: ImageOutputFormat, | |
| max_bytes: usize, | |
| encoded: &mut Vec<u8>, | |
| ) -> anyhow::Result<bool> { | |
| encode_img(img, fmt, 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) | |
| } |
So, i think the name makes sense as it is.
I added a comment that does explain that it actually changes the image (in the resizing-loop, where it is primarily used) to make that easier to notice, and added another comment explaining why the image will be encoded here.
It's good that
It's good that Unless I misunderstood something, this PR here is not needed. |
"contains exif-data = true" means that there is exif-data.
This part is when a file is not too large in the beginning, but will be too large after a white background is added. |
|
Ah, okay, the exif-data will also be removed further below, so it is not necessary to do that before the scaling-loop. I will close this PR. |
When exif-data is found in the avatar-image, it should be re-encoded to remove it.
When the avatar-image will be too large with a white background added, it should be re-encoded to fit within the file-size-limit.
The check for exif-data and for wether or not the image would be too large if encoded with a white background, currently depend on each other.
Both of these would set
do_scaletofalse:file-size is too large = false || is an avatar = true && (too high resolution = false || contains exif-data = true && is too large with a white background = false)
file-size is too large = false || is an avatar = true && (too high resolution = false || contains exif-data = false && is too large with a white background = true)