Skip to content

fix!: use Viewtype::File for messages with invalid images, images of unknown size, huge images (#6825) #6949

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

Merged
merged 1 commit into from
Jul 10, 2025

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Jun 29, 2025

Fix #6825
Thanks to @ivan-cx. JSON-RPC tests pass, so this can be merged to main, but i want everyone to agree on the image size limit (~8.3 Mpx).

@iequidoo iequidoo marked this pull request as draft June 29, 2025 19:07
@iequidoo iequidoo marked this pull request as ready for review June 29, 2025 20:06
@ivan-cx
Copy link
Contributor

ivan-cx commented Jun 30, 2025

Thank you @iequidoo for the review and support! And @r10s for setting the label "good first issue" 👍

The limit of 8294400 pixels is chosen from 4k screen resolution (3840 × 2160) and comes to 33177600 bytes of uncompressed memory consumption, given 32 bit per pixel.

@r10s
Copy link
Contributor

r10s commented Jun 30, 2025

8mp is too strict, just checked a random photo from iphone13 (already some years old) having 12mp, probably newer ones have more

idea is to deal with incredibly large images, no image taken from a system camera (panorama aside) should fall in that category - in practise, they also never made problems, as mp scale to age of phone and ram, at least roughly

EDIT: does this affect also recoding of oitgoing images? if not, at least "happy path" of sending images around would work. but even then, i would use a farer limit - chrrently we have none, so there is no need to be too strict

IMG_9740

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jul 1, 2025

I think it'd be better to use some percentage of the RAM size, otherwise photos taken by new devices can't be displayed on old ones. sysinfo crate provides total_memory(), we depend on this crate anyway. OTOH if the user imports a backup from a device with bigger RAM, the same problem happens. So Viewtype reported by Core should be dynamic, not just what is stored in the db. All this sounds complex, so i suggest to just increase the limit to 32 Mpx for now. CC @ivan-cx

@ivan-cx
Copy link
Contributor

ivan-cx commented Jul 1, 2025

I agree with idea to increaste up to 32 megapixels. I think, up to ~50 megapixels should be OK.

This limit does not affect sending pictures, there are other (much stricter) conversion rules applied, if i understand correctly.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jul 2, 2025

I increased the limit to 35 Mpx. Can increase it to 50 Mpx, but then need to regenerate the test data. CC @ivan-cx

@iequidoo iequidoo requested review from r10s and Hocuri July 2, 2025 13:30
@Hocuri
Copy link
Collaborator

Hocuri commented Jul 6, 2025

I don't have an opinion and don't feel the need to form one, I think there are enough ppl discussing already, and I trust that you are making a good decision :)

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jul 8, 2025

Increased the limit to 50 Mpx. Some modern cameras have >= 40 Mpx. And let's merge this, even a high limit is better than no limit at all.

Still need someone's approval because i made some changes.

…unknown size, images > 50 Mpx (#6825)

BREAKING CHANGE: messages with invalid images, images of unknown size,
huge images, will have Viewtype::File

After changing the logic of Viewtype selection, I had to fix 3 old tests
that used invalid Base64 image data.

Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
@iequidoo iequidoo merged commit 22258f7 into main Jul 10, 2025
29 checks passed
@iequidoo iequidoo deleted the pre-main branch July 10, 2025 21:11
@iequidoo iequidoo restored the pre-main branch July 10, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deal with huge incoming image pixelsizes
5 participants