Image attachments improvements #4037

iequidoo · 2023-02-14T16:21:35Z

Remove metadata from avatars and JPEG images before sending (#4027)

If there's an Exif, rewrite the file to remove it. This implies recoding now though.

Reduce + recode images to JPEG if they are too huge (#3956)

I.e. > 500K for the balanced media quality and 130K for the worse one. This can remove animation and
transparency from PNG/WebP, but then a user always can send an image as a file.

Also don't reduce wide/high images if they aren't huge. Among other benefits, this way most of PNG
screenshots aren't touched.

Also remove Exif from all images, not from JPEGs only.

iequidoo · 2023-02-14T16:28:39Z

Btw, we can easily remove Exif from all images if it's ok to recode them to JPEG @link2xt
EDIT: Decided to remove only from JPEG for now

link2xt · 2023-02-14T17:20:30Z

Why test images are changed in the commit?

iequidoo · 2023-02-14T19:01:59Z

Why test images are changed in the commit?

Yes, only avatar1000x1000.jpg needs to be changed, added some Exif there -- there was no one. So, the tests check that there's an Exif in a source image and no Exif in a received one

src/blob.rs

link2xt · 2023-02-22T14:56:50Z

CHANGELOG.md

@@ -10,6 +10,8 @@
 - Use transaction in `update_blocked_mailinglist_contacts`. #4058
 - Remove `Sql.get_conn()` interface in favor of `.call()` and `.transaction()`. #4055
 - Updated provider database.
+- Remove metadata from avatars and images before sending #4037
+- Reduce and recode images to JPEG if they exceed limits on width or height #4037


This second change is not as simple, I suggest moving it to another PR for now and doing separately.

JPEG does not support transparency, so if the PNG image is some sort of sticker, it will get a solid color background when converted to JPEG.

Maybe, but let's give it another try ^)
And as for transparancy, in #3956 afaiu we agreed with @r10s that sacrificing transparency is better than sending huge images. Anyway a user can send an image as a file. And also most of PNGs (incl. screenshots) won't be recoded because 500K looks quite enough for them

images with viewtype sticker should't be recoded IMHO, they can be animated webp or APNG, but then they also have small width and height so probably don't get affected by this?

ah, it got changed to >500k instead of width and height, I think an animated webp or APNG (even bigger sizes than webp) could be even 1MB I don't think it is good to mess with stickers, and you can't send them as files

For stickers we use Viewtype::Sticker and for GIFs ::Gif. So, they aren't affected, this code is for Viewtype::Image and avatars. But as for animated webp and APNG, u're right, i need to fix it. Thanks!

link2xt · 2023-03-01T16:56:41Z

src/blob.rs

+                == Some((Viewtype::Image, "image/jpeg"));
+            // We need to rewrite JPEGs with Exif to remove metadata such as location,
+            // camera model, etc.
+            let rm_exif = is_jpeg && exif.is_some();


Why check is_jpeg here? If we managed to extract exif from png, we also want to rewrite it.

I checked that on Android and in KDE there is no Exif in PNG screenshots. And we discussed with @r10s in #3956 that currently we can leave Exif in PNGs and other non-JPEGs while there's no evidence that they can carry a sensetive info. But i think removing it is also ok (because, again, screenshots likely have no Exif).

To be honest, i'd rather remove it. Let's vote

I vote for also removing from PNG if it is cheap/easy to do as for jpeg and if it won't affect the PNG image

The only problem is that now we're not able to save PNGs, we maybe need some crate for this. On the other hand, removing Exif is just removing some bytes from the container. Will try to do

As discussed with @link2xt in #4027 (comment) , let's recode images to JPEG to remove Exif for now. Most of PNGs, incl. screenshots, don't contain Exif. As for animated images, IDK what to do with them, i tried to save animated PNGs as PNGs, but the image crate we use don't preserve animation. Maybe we should send them in original size for now, the only question is how to guess if an image is animated

I even think that for all animated images ViewType::Gif must be used (maybe rename it) so that this code won't run for them

So, we can use the image crate to detect animated images, but this needs separate handling of PNGs and WebPs, there is nothing like is_animated() function, we have to decode an image using image::codecs stuff and check then. Ugly. And if it's animated, handle it as ViewType::Gif (renamed to e.g. ViewType::Animation). Should be done in a separate commit of course. @adbenitez, @link2xt, do u have better ideas?

There is also image_utils crate having Info::frames: u32 thing that is > 1 for animated images, but it doesn't support PNGs and WebPs, only Gifs :) And also requires decoding first.

Another option is to go simple for now and recode all images with Exif or big ones to JPEG. A user still can send big animated images as files.

So, big animated images will be sent as JPEGs with their first frame. I think this won't happen usually because animated images are often small, i.e. < 500K. Screenshots are big, but they aren't animated. But if a user set MediaQuality::Worse, they may have problems, 130K as in the PR may be not enough for animated images.

UPD: Looks like i'm too optimistic. APNGs rather need 1M, 500K is not sufficient.

UPD: Also there's an option to increase the weight limit for PNGs and WebPs to 1M.
UPD: Finally i think that 750K is sufficient, this one -- https://upload.wikimedia.org/wikipedia/commons/7/7e/Brain_MRI_apng_105px_100ms.png -- is big enough and it's not easy to find bigger sticker-sized animated PNGs or WebPs.

CHANGELOG.md

adbenitez

looking forward to use this! great improvement! you are my personal hero! for real! 🆒

r10s · 2023-03-07T21:10:09Z

the current state of the pr does not recode PNG -> JPG any longer but does scale down PNG and leave it as PNG.

this wastes much space:

eg. the sample screenshot of of #3956 (comment) is 1.8 MB when scaled down - while only 0.5 MB as JPG/90% (70% even less and still fine).

but also, it looks worse, the scaling of the image crate adds quite some noise (look at the bolder font and the even bolder "t" of "locally generated") and overall pretty pixelated:

(1.8 MB)

here is the same as JPG (as the previous state of the pr was) - some artefacts, but much better:

(0.5 MB)

r10s

i suggest to revert the last changes and to stay with recoding PNG -> JPG.

there are very few disadvantages with that, see discussion above, that outweight the advantages by far.

for transparency and animation, i would for now do no "extra things" - stickers are not affected, small PNG are not recoded, animations usually have a different extension - so, there are few cases with issues only. detection also seems to be hard - but we can still iterate over that in a subsequent pr, but i would wait if that turns out to be really needed.

iequidoo · 2023-03-13T00:32:47Z

@r10s what's is the correct workflow here to tell github that the requested changes are done?

missytake · 2023-03-28T11:51:37Z

@r10s what's is the correct workflow here to tell github that the requested changes are done?

by re-requesting a review I think :) which you already did. If there is no reaction after some time it can make sense to ask a reviewer per DM about the state of the review/PR. In this case I'd ask r10s after the 1.36 releases are out.

iequidoo · 2023-04-03T17:30:47Z

@r10s, can we merge this now as 1.36 is out?

gerryfrancis · 2023-04-04T20:56:51Z

@iequidoo I think you have to rerequest a review from @r10s .

r10s · 2023-04-06T11:28:08Z

sorry, i did not manage yet to dive into this issue again. but i also do not think that i am the bottleneck here?

as the general problems (here and here) seems to be targeted, i removed my "changes requested" (or i tried :) someone needs to look closer to the code again, probably

iequidoo · 2023-04-14T02:31:26Z

@link2xt, i think the final decision for merging this is yours :)

Simon-Laux · 2023-04-15T12:20:25Z

but then a user always can send an image as a file.

not really convinient/usable as normal user on iOS yet: deltachat/deltachat-ios#1840

also this only affects images, not stickers right?

link2xt · 2023-04-15T12:43:17Z

src/blob.rs

            let mut encoded = Vec::new();
            let mut changed_name = None;

-            let exceeds_width = img.width() > img_wh || img.height() > img_wh;
+            if matches!(orientation, Some(90) | Some(180) | Some(270)) {


This if seems unnecessary, match inside checks the same already.

link2xt · 2023-04-15T12:57:26Z

src/blob.rs

+            let fmt = ImageFormat::from_path(&blob_abs);
+            let ofmt = match fmt {
+                Ok(ImageFormat::Png) if !exceeds_max_bytes => ImageOutputFormat::Png,
+                _ => ImageOutputFormat::Jpeg(75),


It is not clear what 75 means, had to look up the documentation. I would add a let jpeg_quality = 75; line.

link2xt · 2023-04-15T13:06:08Z

src/blob.rs

+            let do_scale = exceeds_max_bytes
+                || strict_limits
+                    && (exceeds_wh
+                        || exif.is_some()


I don't get this condition, why exif.is_some() is only checked if strict_limts = true and encoded_img_exceeds_bytes(...) = true?

This expr is only for whether we should scale the image. So, if there's an Exif, we're going to recode the image (currently we can't just remove it using the image crate) and that's why if strict_limits is set, we're looking at encoded_img_exceeds_bytes(...).

link2xt · 2023-04-15T13:06:39Z

src/blob.rs


-                // The file format is JPEG now, we may have to change the file extension
-                if !matches!(ImageFormat::from_path(&blob_abs), Ok(ImageFormat::Jpeg)) {
+            if do_scale || exif.is_some() {


Why exif.is_some() is checked here if we only rewrite the image if do_scale is true?

To remove Exif. So, actually we rewrite the image (recode it) if there's an Exif also

r10s · 2023-04-15T14:32:07Z

but then a user always can send an image as a file.

not really convinient/usable as normal user on iOS yet: deltachat/deltachat-ios#1840

thanks for the pointer, let's continue discussion about that aspect at deltachat/deltachat-ios#1840 , not here. this pr is unrelated and ios should not block things here (it is already complicated enough here :)

also this only affects images, not stickers right?

this pr? this pr should not affect stickers, see discussion above

If there's an Exif, rewrite the file to remove it. This implies recoding now though.

I.e. > 500K for the balanced media quality and 130K for the worse one. This can remove animation and transparency from PNG/WebP, but then a user always can send an image as a file. Also don't reduce wide/high images if they aren't huge. Among other benefits, this way most of PNG screenshots aren't touched. Also remove Exif from all images, not from JPEGs only.

r10s

was also playing along, re-testing things i critisized before - much better!

r10s · 2023-04-16T20:38:24Z

\o/ thanks for merging. i think, this is a really great improvement! looking forward to test that out in practise

iequidoo force-pushed the iequidoo/no-exif branch from 681c1c4 to dbfa144 Compare February 14, 2023 16:22

iequidoo marked this pull request as ready for review February 14, 2023 17:06

iequidoo requested review from link2xt and r10s February 14, 2023 17:06

iequidoo force-pushed the iequidoo/no-exif branch from dbfa144 to 12dceca Compare February 14, 2023 18:58

link2xt reviewed Feb 15, 2023

View reviewed changes

src/blob.rs Outdated Show resolved Hide resolved

src/blob.rs Outdated Show resolved Hide resolved

iequidoo force-pushed the iequidoo/no-exif branch from 12dceca to 7f3e6e5 Compare February 16, 2023 13:59

iequidoo requested a review from link2xt February 16, 2023 14:03

iequidoo mentioned this pull request Feb 20, 2023

recode large PNG #3956

Closed

iequidoo force-pushed the iequidoo/no-exif branch 2 times, most recently from a8dca19 to 20a5328 Compare February 21, 2023 22:35

link2xt reviewed Feb 22, 2023

View reviewed changes

iequidoo marked this pull request as draft February 23, 2023 13:48

iequidoo force-pushed the iequidoo/no-exif branch from 20a5328 to eb508c0 Compare February 24, 2023 14:09

iequidoo changed the title ~~Remove metadata from avatars and JPEG images before sending (#4027)~~ Image attachments improvements Feb 24, 2023

iequidoo marked this pull request as ready for review February 24, 2023 14:32

iequidoo requested review from link2xt, Hocuri and adbenitez February 24, 2023 14:32

link2xt reviewed Mar 1, 2023

View reviewed changes

iequidoo requested a review from link2xt March 2, 2023 11:45

adbenitez reviewed Mar 5, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

adbenitez approved these changes Mar 5, 2023

View reviewed changes

iequidoo marked this pull request as draft March 6, 2023 04:42

iequidoo force-pushed the iequidoo/no-exif branch from eb508c0 to 6fefbc0 Compare March 7, 2023 19:36

r10s requested changes Mar 7, 2023

View reviewed changes

iequidoo force-pushed the iequidoo/no-exif branch 2 times, most recently from e445357 to 039a21f Compare March 25, 2023 19:13

r10s mentioned this pull request Mar 27, 2023

Document how to send original images deltachat/deltachat-pages#625

Merged

iequidoo requested review from r10s and removed request for r10s April 5, 2023 21:58

r10s approved these changes Apr 6, 2023

View reviewed changes

r10s self-requested a review April 6, 2023 11:35

iequidoo force-pushed the iequidoo/no-exif branch from 039a21f to d891640 Compare April 13, 2023 20:33

link2xt reviewed Apr 15, 2023

View reviewed changes

iequidoo added 2 commits April 15, 2023 21:05

Remove metadata from avatars and JPEG images before sending (#4027)

b514bed

If there's an Exif, rewrite the file to remove it. This implies recoding now though.

iequidoo force-pushed the iequidoo/no-exif branch from d891640 to dfbe5ea Compare April 16, 2023 01:07

fix review comments

5269673

iequidoo force-pushed the iequidoo/no-exif branch from dfbe5ea to 5269673 Compare April 16, 2023 01:11

link2xt approved these changes Apr 16, 2023

View reviewed changes

r10s approved these changes Apr 16, 2023

View reviewed changes

iequidoo merged commit 4dfe34e into master Apr 16, 2023

iequidoo deleted the iequidoo/no-exif branch April 16, 2023 19:32

iequidoo mentioned this pull request Apr 16, 2023

Remove metadata from JPEG images before sending #4027

Closed

r10s mentioned this pull request May 2, 2023

Limit Avatar-size (or header-size in general) to 32K to prevent rejection of mails #4370

Closed

r10s mentioned this pull request Jan 21, 2025

Large stickers lose their transparency when sent #6447

Open

Image attachments improvements #4037

Image attachments improvements #4037

Conversation

iequidoo commented Feb 14, 2023 • edited Loading

iequidoo commented Feb 14, 2023 • edited Loading

link2xt commented Feb 14, 2023

iequidoo commented Feb 14, 2023

Choose a reason for hiding this comment

iequidoo Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iequidoo Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iequidoo Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iequidoo Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

adbenitez left a comment

Choose a reason for hiding this comment

r10s commented Mar 7, 2023 • edited Loading

r10s left a comment • edited Loading

Choose a reason for hiding this comment

iequidoo commented Mar 13, 2023

missytake commented Mar 28, 2023

iequidoo commented Apr 3, 2023

gerryfrancis commented Apr 4, 2023

r10s commented Apr 6, 2023 • edited Loading

iequidoo commented Apr 14, 2023

Simon-Laux commented Apr 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r10s commented Apr 15, 2023 • edited Loading

r10s left a comment

Choose a reason for hiding this comment

r10s commented Apr 16, 2023

iequidoo commented Feb 14, 2023 •

edited

Loading

iequidoo commented Feb 14, 2023 •

edited

Loading

iequidoo Feb 24, 2023 •

edited

Loading

iequidoo Mar 1, 2023 •

edited

Loading

iequidoo Mar 6, 2023 •

edited

Loading

iequidoo Mar 6, 2023 •

edited

Loading

r10s commented Mar 7, 2023 •

edited

Loading

r10s left a comment •

edited

Loading

r10s commented Apr 6, 2023 •

edited

Loading

r10s commented Apr 15, 2023 •

edited

Loading