Refactor mms compression to use a binary search for images, and a one pass algorithm for GIFs - #826
Refactor mms compression to use a binary search for images, and a one pass algorithm for GIFs#826octoshrimpy wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the MMS attachment compression path in MessageRepositoryImpl, introducing a binary-search-based image downscaling approach plus a quality-reduction fallback, and adds an ImageUtils helper to support the fallback compression flow.
Changes:
- Add
ImageUtils.getScaledImageWithQuality(...)to iteratively reduce JPEG quality toward a target byte size. - Refactor MMS attachment building to route attachment byte generation through a new
compressedAttachmentBytes(...)helper. - Replace the prior image compression loop with a binary search over image dimensions, with logging around compression attempts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
data/src/main/java/com/moez/QKSMS/util/ImageUtils.kt |
Adds a helper to retry JPEG compression at decreasing quality levels until a target byte size is met (best-effort). |
data/src/main/java/com/moez/QKSMS/repository/MessageRepositoryImpl.kt |
Refactors MMS attachment processing and introduces compressedAttachmentBytes(...) implementing binary-search dimension compression with fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1074e1f to
a4c6c54
Compare
Inhishonor
left a comment
Inhishonor
left a comment
There was a problem hiding this comment.
We need to improve the architecture here, right now it is too performance heavy, but the compression also runs before the message is even marked as sending, so nothing appears in the UI. We should run the compression after marking it as sending. But we also need to fix some of the performance bottlenecks.
Scale factor algorithm became <= 0 after 4-5 attempts regardless of image size, silently bailing and leaving images potentially over the carrier size limit. Replace with binary search on dimensions (100px to carrier max) plus a quality-reduction fallback (90→40) for images that still don't fit. Adds compressAttachment dispatch for extensibility (audio/* logged, image/* compressed, else raw). Fixes InputStream leak in BitmapFactory dimension probe. Queries MMS_CONFIG_ALLOW_ATTACH_AUDIO for future audio support groundwork. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a4c6c54 to
1486596
Compare
Inhishonor
changed the title
1486596 to
96fcb88
Compare
Addresses the review feedback on #826. Compression searches for a size that fits under the carrier limit, so it encodes the same image many times: a binary search over dimensions, then a quality ladder from 90 down to 40 if nothing fit. Every one of those attempts went through ImageUtils.getScaledImage, which re-opened the source URI and re-decoded it from scratch. Decoding is the expensive part and it was the only part being repeated unnecessarily; a large photo could be decoded a dozen or more times to send once. ScaledImageEncoder decodes the source a single time, sampled down to no smaller than the largest width the search will ask for, and re-encodes that bitmap from memory for each attempt. The search itself is unchanged. The decode is guarded: with no carrier cap the target size is the original image, which on a modern camera sensor is large enough to matter, so an OutOfMemoryError returns null and the caller falls back to Glide, which streams instead of holding the whole bitmap. Also fixes three edge cases raised in review: - A bounds decode that fails leaves outWidth/outHeight at zero, making the aspect ratio 0/0 and every derived dimension garbage. Detect it and fall back rather than divide by it. - searchHi was floored at 100, which raised it above the original width, and above the carrier maximum when that was under 100. Both upscale the image and inflate the payload. The floor is now 1 and the search starts at 1 rather than 100. - The last-resort fallback hard-coded a width of 100, upscaling anything narrower. It is now bounded by searchHi, so it can only shrink.
|
Pushed 2bd62f2. You and the bot were pointing at the same thing: every attempt went through
Copilot's three are fixed too: bounds decode failing left aspect ratio at 0/0, Haven't touched the ordering yet, want your read first. Fixing it properly means creating a provider record before its parts exist, which risks half formed messages in telephony. If decode-once makes the stall imperceptible I'd rather not take that. |
Closes: #825
Closes: #116
Closes: #657
Closes: #759