fix: prevent PNG to JPG conversion on image crop #17840

Scapesfear · 2025-01-17T17:56:35Z

Purpose / Description

Fixes PNG to JPG conversion issue during image cropping, preserving the original format (PNG) for transparency and quality retention. Ensures the cropping process doesn't alter the image format unintentionally.

Fixes

Image Cropping: PNG is converted to JPG #17817

Approach

This change checks the original image format (PNG) before cropping and preserves it during the process. It ensures that PNG images are cropped without being converted to JPG, maintaining transparency and quality.

How Has This Been Tested?

Samsung F23 5G

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

mikehardy

should handle webp while we're in here I think. If we're going from "one to many", and the maximum number of "many" is 3, going from 1->3 cases instead of just 1->2 isn't too much of a stretch

mikehardy · 2025-01-17T18:25:03Z

AnkiDroid/src/main/java/com/ichi2/imagecropper/ImageCropper.kt

-                cropImageView.croppedImageAsync()
+                if (isImagePng()) {
+                    cropImageView.croppedImageAsync(
+                        saveCompressFormat = Bitmap.CompressFormat.PNG,


as long as we are in here, there is also Bitmap.CompressFormat.WEBP (it is deprecated but the WEBP_LOSSY / WEBP_LOSSLESS symbols are only available in API30+ and WEBP is since 14 - so we could start by at least just supporting the WEBP variant and accepting deprecation fix in another PR

I think it would be instructive to see if a WEBP with transparency had the transparency preserved if you altered the method below from isImagePng() to getImageCompressFormat() and had it return the Bitmap.CompressFormat type or similar, so it could handle jpg, png, and webp at the same time.

Ideally that would preserve transparency in WEBP as well

learning:

https://developer.android.com/reference/android/graphics/Bitmap.CompressFormat#WEBP

appears that our cropper is aware of it https://github.com/search?q=repo%3ACanHub%2FAndroid-Image-Cropper%20webp&type=code

mikehardy

The support for output compression types appears fine, but the testing seems completely inadequate.

I used pngs and webps from this gallery here to test transparency preservation https://developers.google.com/speed/webp/gallery2

After crop they appear to come back as jpegs

I must admit - and this is some very pointed criticism I need to make: the original comment where you duplicated the research effort, and this PR that you requested my time to review but failed a basic "I looked at it at least" check prior to requesting my review - they both look very low quality to me and they have wasted my time

untitled.mp4

In combination with the 45 back-and-forths on the auto-scroll PR where QA was repeatedly lacking, this is a strong low-quality impression I've got now. I'm very hesitant to spend more time reviewing anything

mikehardy · 2025-01-17T19:09:16Z

AnkiDroid/src/main/java/com/ichi2/imagecropper/ImageCropper.kt

+            "image/jpeg" -> Bitmap.CompressFormat.JPEG
+            "image/webp" -> {
+                if (Build.VERSION.SDK_INT >= 30) {
+                    Bitmap.CompressFormat.WEBP_LOSSLESS


having read all the associated discussion and docs just a bit ago, I think this (using WEBP_LOSSLESS alone vs WEBP_LOSSY or trying to use both) is the safest choice.

there is no good way to easily determine if a webp is lossless or lossy, so choosing one is best

there is not a huge expansion in size taken if a webp is lossless vs lossy, so choosing LOSSLESS as the single one to use does not have much of a downside but conservatively preserves quality, which respects the user

so I think this is a reasonable choice despite it seeming like there should be support for both of the new API30+ symbols (LOSSLESS and LOSSY)

Scapesfear · 2025-01-18T01:39:45Z

The support for output compression types appears fine, but the testing seems completely inadequate.

I used pngs and webps from this gallery here to test transparency preservation https://developers.google.com/speed/webp/gallery2

After crop they appear to come back as jpegs

I must admit - and this is some very pointed criticism I need to make: the original comment where you duplicated the research effort, and this PR that you requested my time to review but failed a basic "I looked at it at least" check prior to requesting my review - they both look very low quality to me and they have wasted my time

untitled.mp4
In combination with the 45 back-and-forths on the auto-scroll PR where QA was repeatedly lacking, this is a strong low-quality impression I've got now. I'm very hesitant to spend more time reviewing anything

Thank you for the feedback, but I’d like to address a few points:

Testing and Results: I did thorough testing on my side, and the changes were working fine, including transparency preservation. I’ll attach a video to demonstrate the results shortly. However, I request you to ensure that the latest changes from my branch were pulled into your local environment before testing, as I built upon them and confirmed their functionality.

On QA and Effort: Your feedback about the auto-scroll PR and the recycler view crash issue is noted. For the auto-scroll, I iterated based on DavidA's suggestions and put in significant effort to address all concerns. Regarding the recycler view crash, I accept that I couldn’t reproduce the error, but i tested and tried as you said , i did followed the constraints and all . I’ll keep working to improve my QA process and testing rigor.

Regarding Tone: I must admit that your message felt very hurtful and unnecessarily harsh. While I fully accept constructive criticism and want to learn from mistakes, such language can discourage collaboration. I genuinely value your time and feedback and have no intention of wasting it.

WhatsApp.Video.2025-01-18.at.07.03.53_8114ff52.mp4

mikehardy · 2025-01-18T01:48:11Z

I request you to ensure that the latest changes from my branch were pulled into your local environment before testing

I did, I compared the commit hash carefully, because I couldn't believe it and immediately suspected a problem with my testing methodology or tools. That definitely happens.

I'm sorry if my comment had a harsh tone, but I was quite shocked to pull the branch, run it and immediately see that it failed to solve the issue. That's a very worrisome thing. The trust we have between ourselves as developers is built on the quality of our code. I'm not seeing that and you need to know it.

Scapesfear · 2025-01-18T01:53:40Z

I request you to ensure that the latest changes from my branch were pulled into your local environment before testing

I did, I compared the commit hash carefully, because I couldn't believe it and immediately suspected a problem with my testing methodology or tools. That definitely happens.

I'm sorry if my comment had a harsh tone, but I was quite shocked to pull the branch, run it and immediately see that it failed to solve the issue. That's a very worrisome thing. The trust we have between ourselves as developers is built on the quality of our code. I'm not seeing that and you need to know it.

my debug info

AnkiDroid Version = 2.21alpha7-debug (483576b6dd5907a6d6b746e39bca9a11f1db7592)
Backend Version = 0.1.48-anki24.11 (24.11 c47638ca36f99dd4f3b81ae82d964aec66e392e0)
Android Version = 14 (SDK 34)
ProductFlavor = amazon
Device Info = samsung | samsung | m23xq | m23xqdd | SM-E236B | qcom
Webview User Agent = Mozilla/5.0 (Linux; Android 14; SM-E236B Build/UP1A.231005.007; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/131.0.6778.260 Mobile Safari/537.36
ACRA UUID = d232da32-d98a-4fd7-8988-e690381798f3
FSRS = 1.4.3 (Enabled: false)
Crash Reports Enabled = false

and also tested on
Pixel 8a API 28 emulator for below API 30 support test

idk why it is not working in your device , please share your debug info

mikehardy · 2025-01-18T02:12:44Z

Same git SHA running. It's the API35 emulator for 16k stack pages (I test that occasionally as it is the future for a lot of devices, it doesn't have any known problems with AnkiDroid and hasn't for months though...)

AnkiDroid Version = 2.21alpha7-debug (483576b6dd5907a6d6b746e39bca9a11f1db7592)
Backend Version = 0.1.48-anki24.11 (24.11 c47638ca36f99dd4f3b81ae82d964aec66e392e0)
Android Version = 15 (SDK 35)
ProductFlavor = play
Device Info = Google | google | emu64a16k | sdk_gphone16k_arm64 | sdk_gphone16k_arm64 | ranchu
Webview User Agent = Mozilla/5.0 (Linux; Android 15; sdk_gphone16k_arm64 Build/AE3A.240806.041; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/124.0.6367.219 Mobile Safari/537.36
ACRA UUID = 142f7ca4-d081-4520-a78c-60a54191197c
FSRS = 1.4.3 (Enabled: false)
Crash Reports Enabled = false

You can see in my video the steps I took, I thought it was telling that the image kept coming back after crop as jpg

Is it a different pathway perhaps? Android files app - select image - share to ankidroid as "add image" ?

Scapesfear · 2025-01-18T02:15:08Z

@mikehardy
When Adding/Editing in the App:
If you add a PNG file from the Add/Edit screen and crop it using the AnkiDroid feature, the cropping works perfectly as expected. The PNG format is preserved if applicable.

This is what issue raiser wanted in #17817

"What are the steps to reproduce this bug?
Add a PNG file from the Add/Edit screen.
Crop the file using the AnkiDroid feature."

When Sharing from the Gallery:
When an image is shared from the gallery to AnkiDroid, the behavior is different. My hypothesis is that the image is already converted to JPEG during the sharing process before it reaches the cropping feature.
This means the ImageCropper is receiving a JPEG image, not a PNG, so the expected PNG preservation doesn’t apply in this scenario.
The issue lies somewhere else.

Appologies if i am misundertanding anything, thanks.

mikehardy · 2025-01-18T02:23:31Z

deductions

Don't deduce and make assertions based on the deduction. Make a hypothesis and test it

Hypothesis: the image is converted to JPEG during the share and AnkiDroid only ever sees it as JPEG

Test: find the code pathway that accepts the image when shared to AnkIDroid and log it's type. Follow that same path through to where the cropper is called on it and log it's type

Report findings

That's the process I use, except I usually don't "report findings" exactly, I just include them in the PR description / commit messages that contain the fix(es), along with all steps taken to reproduce my findings while testing and that justify the various changes I made

Scapesfear · 2025-01-18T04:07:46Z

PNG.test.mp4

this has been tested.

I am sorry for earlier inconvinience, i was testing by the steps user produced the bug and did not thought of the case when image is shared form android file apps, so when the tests were sucessful i thought it was good to push, did not wanted to waste your time.

mikehardy · 2025-01-18T13:12:37Z

one more case I forgot about until now, "paste image from clipboard":

can be reproduced on emulator
open chrome search for "google lossless and lossy webp gallery"
underneath the images there are links to the actual webp files, click on one that has transparency
long-press on that one and copy image

At this point you should have a webp with transparency saved in the clipboard

open AnkiDroid
click FAB and click add card
paste the image into the field

It appears to paste as a JPEG?

There may be nothing we can do to handle this case, I'm not sure how the paste code works - I don't know if it can only paste JPGs or if it can paste PNGs and WEBPs as well, but it seems like it should paste the image without modification into the field, which then allows cropping without losing transparency

At this point if it is getting into the paste code, it is starting to get a bit far away from the original bug - but the point of software that really tries to help users vs just makes money is to solve the problem the user faced plus all related problems if you "generalize" the user request. And the general user request is "AnkiDroid should respect image transparency" - so...pasting a transparent image really should work too?

mikehardy

possible issue getting type from content resolver - left a comment pointing at the utility to use in place of the type resolution proposed here
does not appear to handle pasting transparent images, unsure if that can be handled, needs investigation
there was some mention of tests? not sure if they were automated or not, however if ContentResolverUtil is used, then the rest of the automated tests would be about picking or pasting or sharing transparent images and I'm not sure how to do that myself, so manual testing may be the only way

This does handle both picking and sharing transparent images in the basic cases which is good. Complex sharing cases (like from google photos when content is in the cloud) will probably fail with ContentResolver problems until the util code is swapped in here

mikehardy · 2025-01-18T13:18:36Z

AnkiDroid/src/main/java/com/ichi2/imagecropper/ImageCropper.kt

+    private fun getImageCompressFormat(uri: Uri?): Bitmap.CompressFormat {
+        Timber.d("Original image URI: $uri")
+        val mimeType = uri?.let { requireContext().contentResolver.getType(it) }
+
+        val fileExtension =
+            if (uri?.scheme == "file") {
+                uri.path?.let { getFileExtensionFromPath(it) }
+            } else {
+                mimeType
+            }


Before adding code, definitely ask "has anyone done this before? if so, is there existing code already?"

The answer in this case is yes and yes! You would not believe how hard it is to get the extension from a content resolver. We have had crashes in this area, and bugs, it is very subtle, and you only see the problems enough to even know there are problems when you have millions of users.

This is the method to use - ContentResolverUtil.getFileName, then grab the extension off that. Note that it can throw an exception so it should be in a try/catch with some sort of "I give up but I won't crash" handling and a Timber.w

mikehardy

seems minimal and complete now
appears to work in my testing when using the picker to select a webp with transparency, or to send a webp with transparency to ankidroid from android files app

mikehardy requested changes Jan 17, 2025

View reviewed changes

Scapesfear requested a review from mikehardy January 17, 2025 18:59

mikehardy requested changes Jan 17, 2025

View reviewed changes

Scapesfear requested a review from mikehardy January 18, 2025 03:58

fix: prevent PNG,WEBP to JPG conversion on image crop

cb7791e

Scapesfear force-pushed the PNG branch from 4b36353 to cb7791e Compare January 18, 2025 07:26

mikehardy requested changes Jan 18, 2025

View reviewed changes

Using ContentResolverUtil.getFileName() .

32f548d

Scapesfear requested a review from mikehardy January 18, 2025 14:30

mikehardy approved these changes Jan 18, 2025

View reviewed changes

mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Jan 18, 2025

This comment was marked as off-topic.

Sign in to view

mikehardy mentioned this pull request Jan 18, 2025

If you copy / paste an image via clipboard into Android, it does not seem to result in anything but JPG #17841

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent PNG to JPG conversion on image crop #17840

fix: prevent PNG to JPG conversion on image crop #17840

Scapesfear commented Jan 17, 2025 •

edited

Loading

mikehardy left a comment

mikehardy Jan 17, 2025

mikehardy left a comment •

edited

Loading

mikehardy Jan 17, 2025 •

edited

Loading

Scapesfear commented Jan 18, 2025

mikehardy commented Jan 18, 2025

Scapesfear commented Jan 18, 2025 •

edited by mikehardy

Loading

mikehardy commented Jan 18, 2025

Scapesfear commented Jan 18, 2025 •

edited

Loading

mikehardy commented Jan 18, 2025

Scapesfear commented Jan 18, 2025 •

edited

Loading

mikehardy commented Jan 18, 2025

mikehardy left a comment

mikehardy Jan 18, 2025

mikehardy left a comment

This comment was marked as off-topic.

fix: prevent PNG to JPG conversion on image crop #17840

Are you sure you want to change the base?

fix: prevent PNG to JPG conversion on image crop #17840

Conversation

Scapesfear commented Jan 17, 2025 • edited Loading

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Jan 17, 2025

Choose a reason for hiding this comment

mikehardy left a comment • edited Loading

Choose a reason for hiding this comment

mikehardy Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Scapesfear commented Jan 18, 2025

mikehardy commented Jan 18, 2025

Scapesfear commented Jan 18, 2025 • edited by mikehardy Loading

mikehardy commented Jan 18, 2025

Scapesfear commented Jan 18, 2025 • edited Loading

mikehardy commented Jan 18, 2025

Scapesfear commented Jan 18, 2025 • edited Loading

mikehardy commented Jan 18, 2025

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Jan 18, 2025

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Scapesfear commented Jan 17, 2025 •

edited

Loading

mikehardy left a comment •

edited

Loading

mikehardy Jan 17, 2025 •

edited

Loading

Scapesfear commented Jan 18, 2025 •

edited by mikehardy

Loading

Scapesfear commented Jan 18, 2025 •

edited

Loading

Scapesfear commented Jan 18, 2025 •

edited

Loading