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

Improve image compression #589

Merged
merged 8 commits into from
Mar 4, 2023

Conversation

naveensingh
Copy link
Member

@naveensingh naveensingh commented Feb 15, 2023

Addressing PRs #585, #571

Apparently, there are no other popular open-source image compression libraries other than the one existing code was based on.

Changes:

  • Approximate quality and compress in one go instead of doing it recursively. For larger images, as per my testing, iterating over different qualities takes more time than iterating over different resolutions.
  • Increase minimum quality to 30 (see code comments)
  • If compressing doesn't help achieve the required file size limit, we'll keep reducing image resolution until the file size is smaller than the max limit.
  • Convert PNGs to JPEG for lossy compression when the max MMS limit is less than 1MB. As PNGs are lossless, the quality parameter is ignored. The only way to reduce size is to reduce the resolution of the image or convert it to JPEG and compress using the quality parameter. This helps avoid tiny pixelated PNG images.
  • Removed the abstraction from the library (didn't think it was necessary)

Most images around 10MB are often compressed in 4 seconds or less. We can add back iterating-over-quality if we want more fine control over the file size.

Results:

This ⬆️ was a 12.5MB, 8k JPEG compressed to 149kB, 1080P in 7.8 seconds.

This one ⬆️ was a 100MB, 16k JPEG compressed to 225kB, 1000x1000 in 19.8 seconds.

Compression logs:
Click to expand!

8k JPEG

16kx16k JPEG

image

The following was a PNG but was converted to JPEG to avoid tiny resolution:
23MB PNG (lossy)

Tested with JPEGs and PNGS ranging from 250Kb to 100MB in size. The limit was set to 300kB and all testing was done on a Snapdragon 680 running near stock Android 12

- Approximate quality and compress in one go instead of iterating.
- If compressing doesn't help achieve the required file size limit, keep reducing resolution until the file size is smaller than the max limit.
- Convert PNGs to JPEG for lossy compression when max MMS limit is less than 1MB. This helps avoid tiny pixelated PNG images.
- Removed the abstraction (didn't think it was necessary)
@tibbi
Copy link
Member

tibbi commented Feb 17, 2023

it seems to work just fine to 1 image, but if I try sending 2, it doesnt work. The sending device shows 2 ticks as delivered, it doesnt even show any error. But the receiving device shows nothing at all. Are you aware of that or can you reproduce it?
Edit: actually, guess you cannot send MMS at all, right?

@tibbi
Copy link
Member

tibbi commented Feb 17, 2023

the master branch version at least shows error 5 when I try sending two casual sized photos. But the new version behaves like it was sent successfully, while it wasnt.

@naveensingh
Copy link
Member Author

naveensingh commented Feb 17, 2023

guess you cannot send MMS at all, right?

that is correct.

it seems to work just fine to 1 image, but if I try sending 2, it doesnt work.

I suspect it's because I didn't account for compressing multiple attachments so that the whole MMS size is less than the limit. It could be that the MMS limit applies to the whole MMS message and not just per attachment, right? Are you able to send two very small images so that they both are below the provider limit?

If that is the case, we'll have to change some of the attachment logic to divide the available MMS bandwidth so that all attachments combined are still within the max MMS limit. Come to think of it, we should also show an error toast if the attachment size exceeds the max limit set in settings.

the new version behaves like it was sent successfully, while it wasn't

that is strange. We always mark the message as failed unless we get resultCode == RESULT_OK from the system.

@naveensingh
Copy link
Member Author

It could be that the MMS limit applies to the whole MMS message

I don't have any info to prove it but that seems like the obvious case. I'll try to resize all the attached images before they are sent to make sure the whole MMS size is less than the max limit.

@tibbi
Copy link
Member

tibbi commented Feb 17, 2023

yeah, sending 2 small images works, if no resizing was needed there

@naveensingh
Copy link
Member Author

okay.

we should also show an error toast if the attachment size exceeds the max limit set in settings

if you want to add it, #591 is the PR for that string

naveensingh and others added 2 commits February 19, 2023 03:01
If the message contains text, it is sent with the last attachment.
@naveensingh
Copy link
Member Author

I'll be testing the latest changes, this should reduce error 5 message seen by users a lot but the other cases described by the author of #571 are not related to the image compression. I guess we'll address those issues separately.

(Maybe we should use some delay when sending MMS attachments one by one, not sure yet.)

@tibbi
Copy link
Member

tibbi commented Feb 21, 2023

if I send 2 images now, the top one is stuck at "Sending..." status even if I relaunch the conversation. All the previous images have their status removed, only the latest one has 2 ticks shown. But actual sending works now, so we are getting there.

@naveensingh
Copy link
Member Author

naveensingh commented Feb 28, 2023

Hi!

the top one is stuck at "Sending..." status even if I relaunch the conversation.

Does it stay like that forever? It's probably waiting for a result from the system. On my device, when I send attachments, they are stuck with 'Sending...' but are later updated with an error message as soon as the system returns a result.
Edit: I tested this by manually triggering the result pending intent with success/failure result codes and the message status was updated properly no matter whether I do it with or without any delay.

a27790e fixes updating UI in case of these failed MMS messages so you don't have to relaunch.

All the previous images have their status removed, only the latest one has 2 ticks shown.

isn't that by design? we only show the status ticks for the last message in the recycler view. If you want, we could do that click-to-show-status thing like Google messages

@tibbi
Copy link
Member

tibbi commented Mar 3, 2023

isn't that by design? we only show the status ticks for the last message in the recycler view. If you want, we could do that click-to-show-status thing like Google messages

yes, it is the expected behaviour, I just wrote it to highlight the stuck "Sending..." message

@tibbi
Copy link
Member

tibbi commented Mar 3, 2023

getting a crash at using "Choose photo", then picking a system file and returning to the app

java.lang.NoSuchMethodError: No virtual method compressImage(Landroid/net/Uri;JLkotlin/jvm/functions/Function1;)V in class Lcom/simplemobiletools/smsmessenger/helpers/ImageCompressor; or its super classes (declaration of 'com.simplemobiletools.smsmessenger.helpers.ImageCompressor' appears in /data/app/~~9cVhWOa-J6nqlWEwFieU2A==/com.simplemobiletools.smsmessenger.debug-EVYexjBB91p7-9I5XPXu4Q==/base.apk!classes6.dex)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter.setupMediaPreview(AttachmentsAdapter.kt:135)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter.access$setupMediaPreview(AttachmentsAdapter.kt:30)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter$onBindViewHolder$1.invoke(AttachmentsAdapter.kt:88)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter$onBindViewHolder$1.invoke(AttachmentsAdapter.kt:62)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter$ViewHolder.bindView(AttachmentsAdapter.kt:193)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter.onBindViewHolder(AttachmentsAdapter.kt:62)
at com.simplemobiletools.smsmessenger.adapters.AttachmentsAdapter.onBindViewHolder(AttachmentsAdapter.kt:30)

@naveensingh
Copy link
Member Author

java.lang.NoSuchMethodError: No virtual method

whenever that happens, it's almost always a build issue. I just uninstall the app, clean the project and install it again.

@tibbi
Copy link
Member

tibbi commented Mar 4, 2023

ok, should be good, thanks. Will see how it works.

@tibbi tibbi merged commit 758d538 into SimpleMobileTools:master Mar 4, 2023
@tibbi
Copy link
Member

tibbi commented Mar 4, 2023

if I send 2 images at once, they are sent properly, but there is still "Sending..." stuck under the top image

@tibbi
Copy link
Member

tibbi commented Mar 4, 2023

it is stuck even after restarting the app

images

@naveensingh
Copy link
Member Author

I guess I can't test this properly on my own, can you check if MmsSentReceiver is triggered only once or twice when sending two images? (the updateAndroidDatabase method, maybe check the resultCode too)

It should get called twice with unique message Uris.

@naveensingh
Copy link
Member Author

What happens when you send 3 or 4 images? Is it still only the top one that gets stuck?

@tibbi
Copy link
Member

tibbi commented Mar 4, 2023

if I send 3 images, the top 2 have "Sending..." stuck

@tibbi
Copy link
Member

tibbi commented Mar 4, 2023

if I send multiple images, updateAndroidDatabase is triggered once only

@naveensingh
Copy link
Member Author

ok, I'm investigating it. Will create another PR for it.

I believe it has something to do with sending MMS messages frequently without waiting for results.

naveensingh added a commit to FossifyOrg/Messages that referenced this pull request Mar 6, 2023
@camporter camporter mentioned this pull request Mar 16, 2023
@naveensingh naveensingh deleted the improve_compressor branch May 18, 2023 18:13
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.

None yet

2 participants