Skip to content
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

recode large PNG #3956

Closed
r10s opened this issue Jan 13, 2023 · 15 comments
Closed

recode large PNG #3956

r10s opened this issue Jan 13, 2023 · 15 comments
Assignees
Labels
enhancement New feature or request

Comments

@r10s
Copy link
Contributor

r10s commented Jan 13, 2023

PNG files may become quite large, eg. when used for screenshots by the system.

therefore, when sent as DC_MSG_IMAGE, we could try to recode these PNG to a smaller bytesize, similar to what we're doing with large JPG (original PNG can still be sent when sending as DC_MSG_FILE).

recoding could be done by:

  1. scale down image width/height and/or palette and keep PNG format
  2. recode to JPG; JPG of the same width/height usually have much smaller bytesize
  3. a combination of 1. and 2.
  4. ?

this needs to be tried out. the destination bytes should be comparable with the result of WORSE_IMAGE_SIZE/BALANCED_IMAGE_SIZE, if that is too worse, a factor of that.

this issue is not about introducing new options in the UI, it is just about respecting exiting media-quality setting also for PNG.

nb: for avatars, we are already recoding PNG to JPG (as 2. above), so, the basic functionality is there.

most work will be testing and checking that things work with a changed file name+type.

ftr: this is a succcessor of #1563

@r10s r10s added the enhancement New feature or request label Jan 13, 2023
@Simon-Laux
Copy link
Contributor

there is also the option for lossless compression/optimization with libs/tools like https://github.com/shssoichiro/oxipng

@r10s
Copy link
Contributor Author

r10s commented Jan 14, 2023

i played a bit around with a random screenshot with lots of text but also some images, 1125 x 2436 px.

original             4.6 mb
oxipng               3.0 mb
oxipng -Z            2.9 mb (recoding took 5 minutes)
jpg 90%              0.5 mb
jpg 90%, 50% scaled  0.2 mb
jpg 70%              0.3 mb

as oxipng is lossless, there is nothing to compare visually, below the original and the three jpg:

Click me

original:
device-messages


jpg 90%:
device-messages


jpg 90%, 50% scaled:
device-messages

jpg 70%:
device-messages

not sure about scaling, just added that by interest. by 1/4th of pixels and potential details, the bytesize is down only by ~ 1/2, probably as by antialiasing things are harder to compress, esp. lines and fonts.

other typical images to try out would be diagrams, a statistics etc.

@Septias
Copy link
Collaborator

Septias commented Jan 16, 2023

Do you want to add recoding for all pngs sent? Because I know a lot of people who are annoyed with e.g. whatsapp for downscaling images when sent.
Maybe an option before sending out images where you can decide whetr they should be recoded is a nice option. Would need ui adoption though which is another burden...

@r10s
Copy link
Contributor Author

r10s commented Jan 16, 2023

Do you want to add recoding for all pngs sent?

all png that appear too large in bytes, yes.

for now, this is not about adding an option, that would be another effort.

so, as for jpg, this issue is about finding a good compromise, eg. leaving things alone that are not too large (already well created png). that also whatsapp is doing downscaling by default amplifies this approach, even if there are some who do no like that :)

finally, even if not obvious, you can send things unchanged when attaching as files (this is true also for JPG already today). but we should add that to the faq.

@Septias
Copy link
Collaborator

Septias commented Jan 16, 2023

ah okay, if it is still possible to send images uncompressed, then I'm fine

@Simon-Laux
Copy link
Contributor

but we should add that to the faq.

that has not really much effect, adding to the UI like telegram does it will be the real solution.

@r10s
Copy link
Contributor Author

r10s commented Jan 26, 2023

maybe, but that is another issue/effort.

@iequidoo
Copy link
Collaborator

Btw, we also should remove EXIF from PNGs and other images because it can contain sensitive info like your location. Here i do that for JPEG: #4037. And now i'm thinking about doing that for all images.

So, when i send images as images :), anyway a part of information is lost. At least there's no guarantee to the contrary. So why not also recode to JPEG to save traffic and storage and preserve code simplicity? If i want, i can send as file anyway. All gradations in between are o(all_use_cases) i think. If there are no objections, i can do because already touching this code

@iequidoo iequidoo self-assigned this Feb 20, 2023
@r10s
Copy link
Contributor Author

r10s commented Feb 20, 2023

i think, there are still reasons to leave a png as is, eg if it is already small (a diagram in a png with few colors, already optimised, can be pretty small).

maybe we should think about PNG also more as of PDF or DOCX - yes there can be any meta data in both, however, they are not added "just so" in masses by the device when doing eg. a photo with the camera. at least, that could be our thinking until we have concrete counterexamples where PNG are filled with metabdata at scale without knowledge of the user.

so, i would handle this issue with focussing on PNG bytesize mainly.

maybe sth. like the following

// oxipng(img) // EDIT: maybe for some future, see comments below
if (img.bytesize > 500_000) {
   force_jpg(img)
   recode_to_image_size(img) // do some max. here,
                             // however, be more generous as WORSE|BALANCED_IMAGE_SIZE
                             // to not make screenshots look too bad, maybe double the max. allowed size;
                             // also take care not to scale up
}

as a result, most screenshots and other PNG should not be larger than 500k
cc @adbenitez

we could maybe expand the condition to !img.is_transparent however, that information may not be available "just so" - and i am also not sure about that - if there is a huge png that is not scaled down - that may be much worse than the removed transparency (which may not even be noticed - or is even better in some cases - and if in doubt, it may still be better to send a scaled-down jpg with 500k as a worldmap of the world in 20mb.

but i think, if the PNG pipeline is bootstrapped once, we can easily tweaked and optimized anyway.

@r10s
Copy link
Contributor Author

r10s commented Feb 21, 2023

i just came over sending screenshots on ios:

when drawing something inside a screenshot, apple also saves the result as JPG, keeping the resolution, just so, without asking the user. this underlines our approach of using JPG instead of PNG as outlined above, it will probably not even be noticed in many cases (i was not aware of the apple behaviour as well :)

i would definitely go for the PNG->JPG step and trying that out in practise before trying out oxipng - maybe the dependency and the effort for oxipng is just not needed for the vast majority of cases then.

@iequidoo
Copy link
Collaborator

iequidoo commented Feb 21, 2023

@r10s, the nuance is also that now we have only limits on image width and height:

// max. width/height of images                                                                                                                                                                                       
pub const BALANCED_IMAGE_SIZE: u32 = 1280;
pub const WORSE_IMAGE_SIZE: u32 = 640;

So, i'm thinking about applying them for PNGs and other formats also (now it's done only for JPEG, but if we start doing it also for PNG then why not for all formats). But 1280x1280 PNG may be too big, so we should introduce such constants for the byte size also. Any suggestions? Please note however that screenshots will be likely always recoded because of this 1280 px limit. So, maybe increase limits on width/height (or remove at all) if we introduce limits on a byte size?

Also since i'm going to recode all EXIF-containing JPEGs (it is the easiest way to remove EXIF now), better to do that for all image formats. I checked on my Android that screenshots don't contain EXIF so they won't be recoded because of that.

EDIT: Maybe 500k for "balanced" and 150k for "worse"?

iequidoo added a commit that referenced this issue Feb 21, 2023
- Remove Exif if present.
- Reduce width/height if exceeded.
And encode to JPEG if any of the above is applied.
iequidoo added a commit that referenced this issue Feb 21, 2023
- Remove Exif if present.
- Reduce width/height if exceeded.
And encode to JPEG if any of the above is applied.
@r10s
Copy link
Contributor Author

r10s commented Feb 22, 2023

thanks for pushing this forward, @iequidoo !

But 1280x1280 PNG may be too big, so we should introduce such constants for the byte size also.

recoding to a given bytesize is harder, one has to recode multiple times etc. - and it is also not not adaptive, resulting easier in poor quality. i am pretty sure that this is not worth the effort - in practise, eg. the mentioned 500k for "balanced" and 150k for "worse" are approximated also by the width/height limits in the vast majority of cases. this is good enough.

let's leave things simple. and as said, leave EXIF outside of this issue, see above for reasoning.

so, for PNG->JPG, let's leave the width/height mostly as is, introducing a limit that is large enough that most typical PNG are not scaled, sth. as pub const PNG_IMAGE_SIZE: u32 = 2560;

the resulting JPG will have a larger bytesize than WORSE/BALANCED images - but that is okay, the bytesize will still be a magnitude lower than currently, probably even in worse case.

@iequidoo
Copy link
Collaborator

recoding to a given bytesize is harder, one has to recode multiple times etc. - and it is also not not adaptive, resulting easier in poor quality. i am pretty sure that this is not worth the effort - in practise, eg. the mentioned 500k for "balanced" and 150k for "worse" are approximated also by the width/height limits in the vast majority of cases. this is good enough.

We already have the code that does this multiple-time recoding for avatars, and it's used for attached images also but just w/o the byte size limit for them, so there's almost nothing to code. But... i agree, it's just an excessive work, lets go with the proper limits on width/height instead. But i'd then improve it -- if PNG (or any other image) doesn't exceed 500k bytes, why recode it at all? Just send in original size. But if it exceeds, then simply apply the width/height limits and recode to JPEG. So, most of PNGs won't be recoded. And yes, we can can make width/height limits for PNG (and other non-JPEG formats) higher. Generally i'd prefer not to add several code branches for JPEG/PNG/etc., but just two -- for JPEG and all other. We're not writing GIMP :)

let's leave things simple. and as said, leave EXIF outside of this issue, see above for reasoning.

We with @link2xt decided to remove Exif at least from all JPEGs, so it's not a question of code complexity. Anyway usually there's no Exif in PNGs (checked on my Android and KDE screenshots). But ok, will do this check only for JPEG for now.

@r10s
Copy link
Contributor Author

r10s commented Feb 23, 2023

if PNG (or any other image) doesn't exceed 500k bytes, why recode it at all? Just send in original size

agree

But if it exceeds, then simply apply the width/height limits and recode to JPEG.

maybe this is good enough, we can start with that, agree

So, most of PNGs won't be recoded.

i do not think, this is true. the majority of screenshots will be recoded as they are usually larger than 1280 in width or height. and ppl nowadays use a lot screenshotting, it is not only us devs when writing some docs. i assume most PNG send out are screenshots (or sticker, but they're handled differently)

And yes, we can can make width/height limits for PNG (and other non-JPEG formats) higher

we can consider this later, but as said above, we can start without. maybe it is just good enough and screenshots scaled down to 1280 are just fine.

so, to be clear: you convinced me, i totally agree with applying the same width/height to PNG, there are good chances that it is good enough and we avoid premature optimisation this way :)

decided to remove Exif at least from all JPEG

totally fine - i am not against removing Exif, i just think, that removal from PNG that are not recoded is out of scope of this issue.

iequidoo added a commit that referenced this issue Feb 24, 2023
I.e. > 500K for the balanced media quality and 130K for the worse one. This can remove transparancy
from PNG f.e., but then 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 won't be recoded.
iequidoo added a commit that referenced this issue Mar 7, 2023
I.e. > 500K for the balanced media quality and 130K for the worse one. This can remove animation
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 added a commit that referenced this issue Mar 8, 2023
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 added a commit that referenced this issue Mar 8, 2023
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 added a commit that referenced this issue Mar 11, 2023
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 added a commit that referenced this issue Mar 23, 2023
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 added a commit that referenced this issue Mar 25, 2023
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 added a commit that referenced this issue Apr 13, 2023
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 added a commit that referenced this issue Apr 16, 2023
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 added a commit that referenced this issue Apr 16, 2023
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
Copy link
Collaborator

Done in #4037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants