Fix downscaling images #2469
Fix downscaling images #2469
Conversation
r10s
left a comment
r10s
left a comment
There was a problem hiding this comment.
lgtm, esp. as for a bugfix, it is not wise to refactor too much and to introduce new bugs :)
+100 for adding more tests in a subsequent pr, some more images in test test-folder are totally fine.
| Ok(()) | ||
| } | ||
| fn encode_img_exceeds_bytes( | ||
| fn encoded_img_exceeds_bytes( |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i see, as said, just leave it as is :)
| #[async_std::test] | ||
| async fn test_media_quality() -> anyhow::Result<()> { | ||
| for (media_quality_config, image_size) in vec
Fix #2468
The problem was:
If
max_bytesisNone,encoded_img_exceeds_bytesdoes nothing, esp. it doesn't encode the image intoencoded.Then,
https://github.com/deltachat/deltachat-core-rust/blob/c183203577d8c0b397a3c6216d80c1f2d3112d06/src/blob.rs#L537-L539
encoded the image into
encoded, but without scaling it down.(these three lines are needed in case the image does not need to be scaled at all but just rotated)
I wrote a test, but it doesn't include the case that an image has to be rotated; I'm wondering whether this would be worth the effort as we would probably need to put an image with the rotation-exif-flag into the
test-datafolder, sending it out to get the "actual" image, manually rotating it inside the test to get the "expected" image, and then somehow comparing "actual" and "expected". Also, I'm currently running a little low on time, lots of other things to do.What speaks in favor of an extra test is that the whole
recode_to_size()code feels like there could easily be more hidden bugs, there are just so many cases to consider. When implementing #2384, I already rewrote the code some number of times as I was completely un-satisfied or hadn't considered some case.We could also merge this PR and I write some tests later (and we hope that this time, I actually get to do it, I already planned to do this when merging #2384)